-
Notifications
You must be signed in to change notification settings - Fork 17
Lambdaify function composition #73
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
jfmengels
wants to merge
91
commits into
mdgriffith:master
Choose a base branch
from
jfmengels:lambdaify-function-composition
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Lambdaify function composition #73
jfmengels
wants to merge
91
commits into
mdgriffith:master
from
jfmengels:lambdaify-function-composition
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
PedroHLC
pushed a commit
to PedroHLC/elm-gobrrrrr
that referenced
this pull request
Jan 21, 2022
mdgriffith#73 Copy other transformer Get rid of unnecessary things Apply replacement for composeL Extract variable Start count from 0 Inverse operations for composeL Split tests Fix replacement Enable transformation Add comments Add test Recurse through children first Extract information Give less confusing names Move comment Compose multiple functions Remove dollar signs f Add passing test Add TODOs Give unique names to introduced params to avoid conflicts Add failing test Add blocks Wrap test code in IIFE Insert new variable Create insertion mechanism Extract function calls to variables Fix original source code Remove TODO Add explanation Add test Change test title Support extracting variable from the first argument Fix examples Simplify test Add failing test Extract function Remove unnecessary imports Also extract when it's a function expression Extract variable Don't replace the functions that we just introduced Change name of param Change name of declarations Formatting Remove TODO Add failing test Make inserted variables start with a _ Call mergeFunctionCalls Merge functions Insert new declarations in the right block Add TODOs Move the declaration next to the function that uses it Declare statements next to the return statement when possible Make test code more real Add test Make condition more solid Extract function to top-level Rename tests Create unique names manually Support extracting function calls on property accesses Remove ideas that don't seem to improve performance Add failing tests Move condition Support A3 calls Change conditions Change conditions Add TODO Add failing test Update test Inline call to createCompositionCall in createLambda Don't extract to variables for A3 Support wrapping in A2 Use AX functions for the second function as well Add failing test Extract to function Re-use function Move TODOs Reverse conditions Add invocationRegex Increment the AX wrapper count Add test for second function Update source explanation Don't increment AX calls when it already has the perfect number Remove composeL and composeR definitions Extract regexes to util module Improve documentation Don't replace calls to functions that only have a single argument Add passing test Make test clearer Documentation improvements Remove unnecessary import Use mutable clone to update body
b3e4b7e
to
20c3382
Compare
20c3382
to
5605e04
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is based off of #63 (and until it gets merged, contains those same commits).
This transformer turns function composition into an anonymous function with a named parameter.
It makes #65 redundant because this transformation leads that function to have the exact same code as the transformation in that PR. I think that PR does help see the performance increases this can give.
Asset-size wise: I measured it on a single small application, and the size of the compiled JS code is reduced compare to without the transformation. But the gain gets reduced after minification and is reduced to 0 after gzip. But at least it didn't get worse!
There were a lot of cases where I try to make sure the performance doesn't get worse than what we currently have because of how the components get called, but in some cases I made it even faster.
I hopefully wrote a clear documentation of the transformation in the code, but I think the tests go a bit further detail, so do check out both.
Further work
There are still some improvements that can be done but I'll get to that later. For instance improving the following:
I already do this when encountering A3 expressions, so it's do-able, but a bit more work. The current solution is already an improvement in all cases (because it's pretty conservative) but we could go a step further when we know better.
Another one would be to change
not
calls into!
, which would remove the cost of a function call. That said, I tried benchmarking the same idea for field accesses, and that didn't improve anything, so maybe this would not change much either.I am welcome to suggestions and improvements (including but not limited to the name of the transform 😁 )
Also, please let me know how I can properly benchmark the results of this transformation and what the next steps should be before we merge it in.