-
Notifications
You must be signed in to change notification settings - Fork 92
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
Get rid of most generated variables #1679
Merged
Merged
Conversation
This file contains 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
yannham
force-pushed
the
optimization/internal-closure
branch
2 times, most recently
from
October 13, 2023 10:59
b052e49
to
a575a75
Compare
yannham
force-pushed
the
optimization/internal-closure
branch
from
October 13, 2023 16:10
a575a75
to
9529727
Compare
jneem
approved these changes
Oct 13, 2023
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm definitely lacking some context here, but I did have a look at least...
Co-authored-by: jneem <[email protected]>
Co-authored-by: jneem <[email protected]>
yannham
force-pushed
the
optimization/internal-closure
branch
from
October 16, 2023 10:00
ac447d6
to
d2557aa
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.
Fixes #1647.
Context
This PR adds a new node to the AST,
Closure
, which contains a "pointer" (more abstractly, a cache index, in practice a thunk) to an allocated closure. This removes the need of closurizing terms in the environment everywhere.The first motivation was performance, as we used to allocate specific variables and put them in the environment to store those closures, with renaming issues, environment propagations, and paying the price of environment insertion and lookup. Now, we basically just use direct pointers.
The price to pay is that
Closure
now leaks in the AST, even at stages where it doesn't make sense. Ideally, having separate ASTs for parsing and for evaluation would make this cleaner, and is the eventual plan - but for now, it's an ok price to pay to fix some big performance issues. Also, this change makes a lot of code much simpler, because we don't have to propagate and think about nested environments for closurization anymore. There's a lot less threading to do.Content
Closure
node to the ASTclosurized
attributes of arrays and records to know if something is a "fresh" array or record - in which case we allocate the closures for each element at first evaluation - or if it's already "transformed". In some sense, this PR factors the share normal form transformation directly into evaluation, removing the need of creating intermediate let-bindings and allocating the corresponding slot in the environment.Closurizable
into its specific module, and rename itClosurize
. We still useclosurize
because it's handy, but the signature is simpler.Effects
On the codebase provided for #1622, this change doesn't gain an order of magnitude but still seems to provide a boost of around 20-30% (it's hard to say more without a proper statistical method, because there is a lot of variance across runs - an effect of Sip hashing?). It's noticeable without being game changing.
However, on other examples, the gain is drastic. On an example reported in the online chat by another user, this:
Was basically taking forever (didn't finish before interrupted, and worked for at least a solid minute). After this change, it takes between 2 and 3 seconds (it's still a bit high, but nowhere near the previous numbers).
Follow-up
One medium-term follow-up would be to split the AST, and have 1. a concrete AST with no mention of closures 2. A runtime AST with versions of
Array
and(Rec)Record
which enforce their elements are stored inside closures.