libexpr: move eval memory allocation to own struct #14047

Radvendii · 2025-09-22T18:41:02Z

Motivation

Separate the concerns of memory management from the rest of EvalState. This will become even more important in the future as we take on more management of the heap.

Context

Co-authored-by: eldritch horrors [email protected]
https://git.lix.systems/lix-project/lix/commit/f5754dc90ae9b1207656d0e29ad2704d3ef1e554

Add 👍 to pull requests you find important.

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

CC @xokdvium

xokdvium

Fixed some outstanding issues with baseEnvP (bad merge probably). Let's wait for @roberth to take a look as well.

edolstra

Can you explain a bit more about the context? What does "take on more management of the heap" mean?

Changing all these state.allocValue() to state.mem.allocValue() seems like gratuitous code churn to me that will cause a lot of merge conflicts.

Mic92 · 2025-09-23T05:42:47Z

Can you explain a bit more about the context? What does "take on more management of the heap" mean?

Different gc implementations, @Radvendii in particular is working on using 32bit indices rather than pointers to reduce memory usage. This would require a new gc however. I also experimented with bumper allocators for nix-eval-jobs.

Changing all these state.allocValue() to state.mem.allocValue() seems like gratuitous code churn to me that will cause a lot of merge conflicts.

Which pull requests do you have in mind? I don't see too much regarding evaluation at the moment.

Note that he ported this patch based on @xokdvium request, discussed during a team meeting.

Ericson2314 · 2025-09-23T18:04:04Z

I was not paying super close attention, but @roberth and @xokdvium I believe also discussed how the evaluator would become a bit more opaque in order to support @roberth's eval cahing work. I think moving the things to mem was supposed to be a precursor for that.

edolstra · 2025-09-23T18:11:55Z

more opaque in order to support @roberth's eval cahing work

That may be premature abstraction though if we don't know what @roberth's eval caching work will require from a memory interface.

@Radvendii in particular is working on using 32bit indices rather than pointers to reduce memory usage.

That sounds great, but unless we anticipate that we would have multiple implementations in the same code base (rather than just replacing the existing one), having an extra abstraction layer may just make such work harder rather than easier.

BTW, one way to reduce the code churn in this PR a lot is to keep EvalState::allocValue() as an inline forwarder to mem.allocValue().

xokdvium · 2025-09-23T18:28:59Z

EvalState::allocValue() as an inline forwarder to mem.allocValue().

This sounds good.

Ericson2314 · 2025-09-23T18:44:06Z

EvalState::allocValue() as an inline forwarder to mem.allocValue()

OK great, if we can unblock this by doing that to make the new mem field low-cost code churn wise, that's great!

roberth · 2025-09-24T09:44:44Z

more opaque in order to support @roberth's eval cahing work

That may be premature abstraction though if we don't know what @roberth's eval caching work will require from a memory interface.

I plan to add a layer that goes around the "outside" of the evaluator, where EvalState is one of the implementations, as well as capturing "IO" like all SourceAccessors and IFD.
I don't think that work is affected by this change.

Radvendii · 2025-09-25T16:44:21Z

Okay, I restored EvalState::allocValue() and it just calls EvalMemory::allocValue().

For the record while learning my way around the code base recently I found the number of functions that just forward to another function somewhat confusing. Asking myself things like Why are they there? Am I missing something? Which one should I be using?

I added a comment to make it hopefully clearer.

src/libexpr/eval.cc

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

Co-authored-by: eldritch horrors <[email protected]> Co-authored-by: Sergei Zimmerman <[email protected]> See original commit on lix: https://git.lix.systems/lix-project/lix/commit/f5754dc90ae9b1207656d0e29ad2704d3ef1e554

xokdvium

Diff is much smaller

Ericson2314 · 2025-09-26T16:46:03Z

src/libexpr/eval.cc

    : size(size)
    , elems(size <= 2 ? inlineElems : (Value **) allocBytes(size * sizeof(Value *)))
 {
-    state.nrListElems += size;


~~I am not sure that every ListBuilder is going through the new mem.listBuilder?~~

OK nevermind, I thought there was some sort of default arg but there is not. This is easy to verify.

When I search the codebase, I don't find any other instances. Neither with LSP goto-references nor with a text search for ListBuilder. But it could be getting initialized in some way that is tripping up both searches.

code churn got removed.

Radvendii requested review from edolstra and roberth as code owners September 22, 2025 18:41

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 22, 2025

Radvendii force-pushed the eval-memory branch 3 times, most recently from 3ca0525 to f124072 Compare September 22, 2025 18:59

xokdvium force-pushed the eval-memory branch 2 times, most recently from 9f6202e to efe955f Compare September 22, 2025 19:50

xokdvium approved these changes Sep 22, 2025

View reviewed changes

edolstra previously requested changes Sep 22, 2025

View reviewed changes

Radvendii force-pushed the eval-memory branch from efe955f to bf3171b Compare September 25, 2025 16:40

Radvendii force-pushed the eval-memory branch 2 times, most recently from ac59260 to e90f6de Compare September 25, 2025 17:26

xokdvium requested changes Sep 25, 2025

View reviewed changes

src/libexpr/eval.cc Show resolved Hide resolved

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

xokdvium reviewed Sep 25, 2025

View reviewed changes

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

Radvendii force-pushed the eval-memory branch from 56d6d23 to d5bc8b9 Compare September 25, 2025 18:37

libexpr: move eval memory allocation to own struct

7b3c193

Co-authored-by: eldritch horrors <[email protected]> Co-authored-by: Sergei Zimmerman <[email protected]> See original commit on lix: https://git.lix.systems/lix-project/lix/commit/f5754dc90ae9b1207656d0e29ad2704d3ef1e554

xokdvium force-pushed the eval-memory branch from d5bc8b9 to 7b3c193 Compare September 25, 2025 21:45

xokdvium requested a review from edolstra September 25, 2025 22:16

xokdvium approved these changes Sep 25, 2025

View reviewed changes

Radvendii mentioned this pull request Sep 26, 2025

Managed Heap tracking issue #14088

Open

41 tasks

Radvendii mentioned this pull request Sep 26, 2025

nixexpr: introduce arena to hold ExprString strings #14090

Merged

Ericson2314 reviewed Sep 26, 2025

View reviewed changes

Ericson2314 approved these changes Sep 26, 2025

View reviewed changes

Mic92 merged commit b5f765b into NixOS:master Sep 26, 2025
15 checks passed

Uh oh!

libexpr: move eval memory allocation to own struct #14047

libexpr: move eval memory allocation to own struct #14047

Uh oh!

Conversation

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

Uh oh!

Motivation

Context

Uh oh!

xokdvium left a comment

Choose a reason for hiding this comment

Uh oh!

edolstra left a comment • 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!

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

Uh oh!

Uh oh!

Ericson2314 commented Sep 23, 2025

Uh oh!

edolstra commented Sep 23, 2025

Uh oh!

xokdvium commented Sep 23, 2025

Uh oh!

Ericson2314 commented Sep 23, 2025

Uh oh!

roberth commented Sep 24, 2025

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

xokdvium left a comment

Choose a reason for hiding this comment

Uh oh!

Ericson2314 Sep 26, 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!

Radvendii Sep 26, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Radvendii commented Sep 22, 2025 •

edited

Loading

edolstra left a comment •

edited

Loading

Mic92 commented Sep 23, 2025 •

edited

Loading

Radvendii commented Sep 25, 2025 •

edited

Loading

Ericson2314 Sep 26, 2025 •

edited

Loading