nixexpr: introduce arena to hold ExprString strings #14090

Radvendii · 2025-09-26T16:27:33Z

~~⚠️ built on top of #14047~~ (merged)

Motivation

See this tracking issue for more big-picture motivation.

Where does this fall in that picture?

Removes 24/32 btyes per string in the AST (size of an std::string depending on standard library implementation)
- Saves ~10MB (~1%) memory when evaluating a NixOS configuration
Introduces an arena for other Expr-related data to live in
Introduces Exprs struct, which will eventually contain arrays for each of the Expr types
For indentation strings, allocate the memory needed up-front, rather than repeatedly resizing an std::string to accommodate. We may also have been over-allocating as a result (I'm not sure what the reallocation strategy of std::string is)
Once Exprs live in a std::vector, they will need to be trivially moveable. Because of the small string optimization of std::string, moving an ExprString invalidates its Value's pointer.

Note that this does get rid of the small-string optimization built into std::string. That's part of the idea, but it also means for small strings, we now have an extra pointer indirection where there wasn't one before. I didn't find any noticeable slowdown because of this.

I am also opening another PR to add SSO to Values themselves. If that one is accepted, I will modify this one to take advantage of it.

Context

tracking issue

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

Mic92 · 2025-09-26T18:31:27Z

Solved merge conflict and incorperated formatting commit.

Mic92 · 2025-09-26T18:37:50Z

ASAN is not happy.

Mic92 · 2025-09-26T18:38:49Z

also reproducible locally.

src/libexpr/include/nix/expr/parser-state.hh

src/libexpr/include/nix/expr/nixexpr.hh

src/libexpr/include/nix/expr/parser-state.hh

Mic92 · 2025-09-26T19:02:34Z

cc @NaN-git

Radvendii · 2025-09-26T19:50:59Z

Solved merge conflict and incorperated formatting commit.

Did I clobber these again? I pulled before pushing but I don't see your commits 🙈

EDIT: oh never mind, I see you rolled them into the existing commits.

Radvendii · 2025-09-26T19:52:54Z

ASAN is not happy.

Yep.. I fixed the bugs and added asserts to make sure they don't happen sneakily again. (they weren't that sneaky, but I had forgotten to run tests before and was just checking whether a NixOS configuration built)

src/libexpr/include/nix/expr/nixexpr.hh

src/libexpr/include/nix/expr/parser-state.hh

src/libexpr/parser.y

src/libexpr/include/nix/expr/parser-state.hh

NaN-git · 2025-09-28T00:15:08Z

src/libexpr/include/nix/expr/parser-state.hh

+        assert(c > 0);
+        s2[c] = '\0';
+
+        es2->emplace_back(i->first, new ExprString(s2));


I'm wondering whether it's possible to allocate the AST nodes with the buffer allocator.

I think that is supposed to come next? See the main issue.

I'm intending to move the AST nodes into big vectors. How exactly that relates to the buffer allocator isn't clear to me yet since they have to be able to move to accommodate growth.

But indeed I haven't forgotten to consider where the Exprs are going, I'm just trying to do things one small step at a time.

src/libexpr/include/nix/expr/nixexpr.hh

xokdvium · 2025-09-28T12:32:44Z

src/libexpr/parser.y

  std::vector<nix::AttrName> * attrNames;
  std::vector<std::pair<nix::AttrName, nix::PosIdx>> * inheritAttrs;
  std::vector<std::pair<nix::PosIdx, nix::Expr *>> * string_parts;
+  std::variant<nix::Expr *, std::string_view> * to_be_string;


All this makes me wish we could move to a C++ skeleton in bison. Then we'd be able to use proper variant types for values.

I'm not sure what that means "C++ skeleton in bison".

I kind of have an itching to move us to a recursive descent parser, but I'm not sure other people would like that, and in any case that should definitely be after we have a fuzzer that can thoroughly check that they're equivalent

https://www.gnu.org/software/bison/manual/html_node/A-Simple-C_002b_002b-Example.html

Then we wouldn't have to use the %union at all and instead switch to %define api.value.type variant

This was added now in #14105

1. Saves 24-32 bytes per string (size of std::string) 2. Saves additional bytes by not over-allocating strings (in total we save ~1% memory) 3. Sets us up to perform a similar transformation on the other Expr subclasses 4. Makes ExprString trivially moveable (before the string data might move, causing the Value's pointer to become invalid). This is important so we can put ExprStrings in an std::vector and refer to them by index We have introduced a string copy in ParserState::stripIndentation(). This could be removed by pre-allocating the right sized string in the arena, but this adds complexity and doesn't seem to improve performance, so for now we've left the copy in.

Radvendii requested review from edolstra and roberth as code owners September 26, 2025 16:27

github-actions bot added new-cli Relating to the "nix" command repl The Read Eval Print Loop, "nix repl" command and debugger c api Nix as a C library with a stable interface labels Sep 26, 2025

Radvendii mentioned this pull request Sep 26, 2025

Managed Heap tracking issue #14088

Open

42 tasks

Mic92 force-pushed the expr-slim branch from 3877dcb to 3446a46 Compare September 26, 2025 18:31

Mic92 reviewed Sep 26, 2025

View reviewed changes

src/libexpr/include/nix/expr/parser-state.hh Outdated Show resolved Hide resolved

Mic92 reviewed Sep 26, 2025

View reviewed changes

src/libexpr/include/nix/expr/nixexpr.hh Show resolved Hide resolved

Mic92 reviewed Sep 26, 2025

View reviewed changes

src/libexpr/include/nix/expr/parser-state.hh Outdated Show resolved Hide resolved

Radvendii force-pushed the expr-slim branch 3 times, most recently from 0f6732b to 6863c10 Compare September 26, 2025 21:17

Ericson2314 reviewed Sep 27, 2025

View reviewed changes

src/libexpr/include/nix/expr/nixexpr.hh Outdated Show resolved Hide resolved

Radvendii force-pushed the expr-slim branch from ff3d0ba to 966b855 Compare September 27, 2025 23:25

Ericson2314 reviewed Sep 27, 2025

View reviewed changes

src/libexpr/include/nix/expr/nixexpr.hh Outdated Show resolved Hide resolved

Ericson2314 reviewed Sep 28, 2025

View reviewed changes

src/libexpr/include/nix/expr/parser-state.hh Outdated Show resolved Hide resolved

Ericson2314 reviewed Sep 28, 2025

View reviewed changes

src/libexpr/parser.y Show resolved Hide resolved

NaN-git reviewed Sep 28, 2025

View reviewed changes

Radvendii force-pushed the expr-slim branch 2 times, most recently from fa9fdad to aeb391f Compare September 28, 2025 11:48

xokdvium reviewed Sep 28, 2025

View reviewed changes

src/libexpr/include/nix/expr/nixexpr.hh Outdated Show resolved Hide resolved

xokdvium reviewed Sep 28, 2025

View reviewed changes

src/libexpr/include/nix/expr/nixexpr.hh Outdated Show resolved Hide resolved

xokdvium reviewed Sep 28, 2025

View reviewed changes

Radvendii force-pushed the expr-slim branch from c18d9b9 to 1db9336 Compare September 28, 2025 17:23

Radvendii mentioned this pull request Sep 28, 2025

Expr stats #14102

Draft

Radvendii force-pushed the expr-slim branch from ca78783 to 3ef9c79 Compare September 28, 2025 17:56

Radvendii force-pushed the expr-slim branch from 3ef9c79 to eab467e Compare September 28, 2025 18:47

xokdvium mentioned this pull request Sep 28, 2025

libexpr: Switch parser.y to %skeleton lalr1.cc #14105

Merged

Radvendii mentioned this pull request Sep 28, 2025

libexpr: allocate ExprPath strings in the allocator #14106

Merged

Mic92 approved these changes Sep 29, 2025

View reviewed changes

Mic92 merged commit 7cbc0f9 into NixOS:master Sep 29, 2025
16 checks passed

Uh oh!

nixexpr: introduce arena to hold ExprString strings #14090

nixexpr: introduce arena to hold ExprString strings #14090

Uh oh!

Conversation

Radvendii commented Sep 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Context

Uh oh!

Mic92 commented Sep 26, 2025

Uh oh!

Mic92 commented Sep 26, 2025

Uh oh!

Mic92 commented Sep 26, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Mic92 commented Sep 26, 2025

Uh oh!

Radvendii commented Sep 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Radvendii commented Sep 26, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

NaN-git Sep 28, 2025

Choose a reason for hiding this comment

Uh oh!

Ericson2314 Sep 28, 2025

Choose a reason for hiding this comment

Uh oh!

Radvendii Sep 28, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

xokdvium Sep 28, 2025

Choose a reason for hiding this comment

Uh oh!

Radvendii Sep 28, 2025

Choose a reason for hiding this comment

Uh oh!

xokdvium Sep 28, 2025

Choose a reason for hiding this comment

Uh oh!

Mic92 Sep 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Radvendii commented Sep 26, 2025 •

edited

Loading

Radvendii commented Sep 26, 2025 •

edited

Loading

Mic92 Sep 29, 2025 •

edited

Loading