libexpr: allocate ExprAttrs data in Exprs::alloc #14167
Draft
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
This change is more involved than moving other data into Exprs::alloc, because ExprAttrs can be recursive, and we have to maintain mutability until the outer one is finished. At that point we can transfer everything into Exprs::alloc and only keep pointers. This saves about ~4% memory
github-actions
bot
added
the
with-tests
Radvendii
commented
Oct 6, 2025
| @@ -1,4 +1,4 @@ | |||
| error: syntax error, unexpected invalid token, expecting end of file | |||
| error: syntax error, unexpected invalid token | |||
Contributor
Author
There was a problem hiding this comment.
I'm not sure why this error message changed, but it seems maybe not important? I'm not sure why we were expecting EOF before, as 123 + 4, for example, is a valid parse.
Comment on lines
+576
to
+577
| // XXX: help I don't know how to C++ I'm making this up | ||
| class iterator |
Contributor
Author
There was a problem hiding this comment.
Someone please help with proper iterator design I was so confused trying to figure this out
src/libexpr/eval.cc
Outdated
Comment on lines
3228
to
3229
| // XXX: we're already crawling the whole AST here. we could use this opportunity to do ExprAttrsBuilder -> ExprAttrs transformation | ||
| result->bindVars(*this, staticEnv); |
Contributor
Author
There was a problem hiding this comment.
If anyone has thoughts about this design decision please weigh in
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
ExprAttrsas they're created and finalizing them all after parsing when we crawl the AST forbindVars().Motivation
For big-picture motivation, see the tracking issue
This PR moves the data in
ExprAttrsnodes into the bump allocator, allowing us to store only immutable data and thereby save space. In the case ofExprAttrs, this is more involved because they can be nested within each other, and the inner ones must remain mutable until the outermost one is complete. e.g.We add
c = 4;into the firstaattrset while we're parsing.This PR saves 40 bytes per
ExprAttrsstd::map(48 bytes) becomes two pointers and auint32_tlength (20 bytes)std::vector(24 bytes) becomes a pointer and auint16_tlength (10 bytes)std::unique_ptr<std::vector>(8 bytes) becomes a pointer and auint16_tlength (10 bytes)saves an additional 24 bytes for the
std::vectorcontained within thestd::unique_ptrwhen it's non-nullThis PR also removes the
Displacementfield ofAttrDef. It's now cheap (O(1)) to calculate on the spot when we need it, which saves us 8 bytes per AttrDef. (4 bytes for the field, 4 bytes for padding)This is ~1% of memory on its own
We also save when we trim the excess capacity off the ends of vectors and maps when we make them immutable, though I think this is negligible.
All told this saves us ~3.7% of memory usage
Alternative design
Rather than finalizing each
ExprAttrsas they're created, we could leave them asExprAttrsBuilderuntil thebindVars()step, and then while we're crawling the AST for that, also finalizeExprAttrsAdd 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.