feat: persistent lists #11767
feat: persistent lists #11767
Conversation
github-actions
bot
added
new-cli
Memory management bugs are tricky to debug, because their effects tend to be discovered only later on, and in rather uninformative ways. The warnings in https://sinusoid.es/immer/memory.html#garbage-collected-heap hold true, but I wouldn't be surprised if another property is involved in our crash here. Does Does this work if GC is disabled? If so, we might be dealing with a missing GC root. If it also fails with GC disabled, we might be looking at some sort of memory corruption, such as the lack of initialization you suggested. |
| @@ -133,34 +146,6 @@ class ExternalValueBase | |||
| std::ostream & operator << (std::ostream & str, const ExternalValueBase & v); | |||
|
|
|||
|
|
|||
| class ListBuilder | |||
There was a problem hiding this comment.
We'll want to keep this around for the existing C API.
It should be easy to implement with immer types, I suppose.
Whether we continue to use ListBuilder internally is another question, and I could see "no" as a likely answer to it.
| tList1, | ||
| tList2, |
There was a problem hiding this comment.
These two are an optimization that lets the evaluator produce small lists without any allocations besides e.g. the thunk that already existed. I think we'll want to keep tList1 and tList2 for this purpose.
| // declare a memory policy for using a tracing garbage collector | ||
| using gc_policy = immer::memory_policy<immer::heap_policy<immer::gc_heap>, | ||
| immer::no_refcount_policy, | ||
| immer::default_lock_policy, |
There was a problem hiding this comment.
I wonder what the default lock policy does.
I suppose clever locking might save allocations in a few cases, but I'm inclined to think that just allocating all the time is more efficient, because we tend to keep the "old version" around because purely functional operations are non-destructive and our scopes/closures (Env) tend to stick around fairly often in practice, and if they don't, we don't really know locally.
There was a problem hiding this comment.
The locking is not used for allocation actually, but it's used by immer::atom so no need to worry about it :)
|
Thanks for doing this experiment. Maybe to see if this goes in the right direction you could try to do some micro benchmark before having to fix all builtins etc? Than we could already get a performance preview. |
|
also cc @arximboldi |
|
I can eval my NixOS config! I tried re-adding uses of |
ConnorBaker
force-pushed
the
persistent-lists
branch
from
0251ac3
to
2ee7758
Compare
github-actions
bot
added
the
with-tests
|
Rebased to fix merge conflicts. |
No reading list unfortunately; except perhaps for the bdwgc docs, which are mostly in its headers. It's reference documentation, not really learning material. The symptoms you describe suggest that something is not getting marked and then collected by accident, or in other words, some heap objects aren't reachable from the garbage collection roots such as the (main) stack or C++ heap objects allocated with |
|
Adding |
|
@Mic92 I added stats to the numbers section. @roberth I know that with Haskell, one can use allocation stats provided by GHC's RTS as a weak proxy for performance -- any idea if that's the case with Nix as well? My understanding was that a non-trivial amount of time is spent in the garbage collector. |
|
@arximboldi I'm having trouble with transient data structures -- do you have any insight? In void ExprList::eval(EvalState & state, Env & env, Value & v)
{
auto list = state.allocList();
for (auto & i : elems)
*list = list->push_back(i->maybeThunk(state, env));
v.mkList(list);
}I've tried to rewrite it to use void ExprList::eval(EvalState & state, Env & env, Value & v)
{
auto list = state.allocList();
auto transient = list->transient();
for (auto & i : elems) {
transient.push_back(i->maybeThunk(state, env));
}
*list = transient.persistent();
v.mkList(list);
}Unfortunately, when I do that I get an exception when calling nix: /nix/store/gkghbi5d6849mwsbcdhnqljz2xnjvnis-immer-0.8.1-unstable-2024-09-18/include/immer/transience/gc_transience_policy.hpp:95: ownee &immer::gc_transience_policy::apply<immer::heap_policy<immer::gc_heap>>::type::ownee::operator=(edit) [HeapPolicy = immer::heap_policy<immer::gc_heap>]: Assertion `e != noone' failed.I believe something is being garbage collected when it shouldn't be, and verified this by checking that running with I know that in typedef std::vector<Value *, traceable_allocator<Value *>> ValueVector;
typedef std::unordered_map<Symbol, Value *, std::hash<Symbol>, std::equal_to<Symbol>, traceable_allocator<std::pair<const Symbol, Value *>>> ValueMap;
typedef std::map<Symbol, ValueVector, std::less<Symbol>, traceable_allocator<std::pair<const Symbol, ValueVector>>> ValueVectorMap;Do I need to do something similar for Do you have any ideas on what I can do to troubleshoot or debug this? From the error, I'm not entirely sure if it's the |
To my understanding, this is a consequence of widespread use of immutable data structures.
Recent versions of Nix will tell you how much time is spent in GC. Evaluating my NixOS config it spends 10% of time in GC, but an individual package might only spend 1.5% in GC. (anecdata) |
|
So currently we are actually increasing memory usage? I wonder what happens if we remove the optimization from nix for 1,2 element lists from the current implementation. Maybe the comparison is than fairer? |
|
Arrays are pretty compact, fwiw. Maybe this optimization would pay off if we had constant folding? IIRC we haven't previously done constant folding because it didn't pay off, but perhaps that was because of all the copying. Another possibility is that the data structure can't be used efficiently because it has to assume that multiple references to it exist, even though those references might be all from the garbage. |
|
Hi @ConnorBaker ! Thanks a lot for this contribution. I am a fan of Nix and use it in all my systems, so this gets me very excited! 🎉 😄 Taking a look at the code, the two things I can see could improve:
As to why are those crashes happening... you mention that the values are being inappropriately GC'ed. You are using the |
|
Ok, just took a look and it seems we are indeed using Boehm's GC here. And the the implementation of I see instances of something that resembles refcounting in the code though (`nix_gc_incref, etc.). That could be integrated via the memory policy. I think it'd be nice for someone with a bit more knowledge about how the lifetimes of C++ values is managed in Nix and how it all interacts with the GC. It's unclear to me: which allocations are should be made with Boehm's GC Also, what's |
|
Looking at it again... I wouldn't fully discard that there may be a bug in Immer's use of |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/is-it-possible-for-us-to-remove-builtins-functionargs/51960/4 |
ConnorBaker
force-pushed
the
persistent-lists
branch
from
d7f712f
to
f742092
Compare
github-actions
bot
added
the
repl
Important
This PR is a work in progress. I may force-push.
Experimental work on switching to persistent lists via immer (thanks for the library suggestion @Mic92!).
Current blockers:
Various places use collections fromstdwithout configuring them to use the GC.A printing test fails (see TODOs insrc/libexpr/print-ambiguous.cc), but only when run throughnix-instantiate --eval--nix evalreturns the correct result. That's odd and makes me think the failing test is symptomatic of a larger problem I introduced.printAmbiguousI'm unable to evaluate my own system closure because an uninitialized value sneaks in. To help debug this, I've aprintErrortoValue::type(seesrc/libexpr/value.hh), which is what was throwing the error. I've also added an assert toEvalState::forceValue(seesrc/libexpr/eval-inline.hh) to ensure an error is thrown if an uninitialized value is forced.transientValueListthrough thenewkeyword.ValueList::push_backusesstd::moveon the argument; I don't know enough CPP to know what that means if the argument is used afterwards.NIX_SHOW_STATSno longer shows helpful things for lists besides concatenations(++)operand is no longer usingconcatLists, it's not updating the number of list concatenations tracked byEvalState.Motivation
As far as I can tell, lists in Nix are structures with raw C arrays and a size. Most list operations involve allocating an array large enough to hold the result and then
memcpy-ing components over.This PR looks at using persistent lists via immer -- my hope is that by leveraging techniques from function programming, we can reduce the amount of allocation and copying we do and improve performance of the interpreter.
Context
TODO
Early numbers
Setup
Run on a NixOS system with an Intel i9-13900K.
The current implementation is about 1.13x faster than the one in this PR. Put another way, the changes in this PR slow down eval by around 12% (using system evaluation times,
3.339-2.924)/3.339=0.124).NOTE: I verified outputs from both versions are the same.
System evaluation
./nix-before/bin/nix eval --no-eval-cache --read-only github:ConnorBaker/nixos-configs/39f01b05c37494627242162863b4725e38600b50#nixosConfigurations.nixos-desktop.config.system.build.toplevel./nix-after/bin/nix eval --no-eval-cache --read-only github:ConnorBaker/nixos-configs/39f01b05c37494627242162863b4725e38600b50#nixosConfigurations.nixos-desktop.config.system.build.toplevel{ "cpuTime": 2.796797037124634, "envs": { "bytes": 147284032, "elements": 10772297, "number": 7638207 }, "gc": { "cycles": 5, "heapSize": 1024122880, "totalBytes": 1482768144 }, "list": { "bytes": 20914368, "concats": 390624, "elements": 2614296 }, "nrAvoided": 8090856, "nrExprs": 1469182, "nrFunctionCalls": 6649159, "nrLookups": 3978975, "nrOpUpdateValuesCopied": 31774690, "nrOpUpdates": 607131, "nrPrimOpCalls": 3261148, "nrThunks": 11004466, "sets": { "bytes": 679170688, "elements": 40539768, "number": 1908400 }, "sizes": { "Attr": 16, "Bindings": 16, "Env": 8, "Value": 24 }, "symbols": { "bytes": 1016494, "number": 85193 }, "time": { "cpu": 2.796797037124634, "gc": 0.973, "gcFraction": 0.3478979658103235 }, "values": { "bytes": 412291368, "number": 17178807 } }{ "cpuTime": 3.241765022277832, "envs": { "bytes": 147284032, "elements": 10772297, "number": 7638207 }, "gc": { "cycles": 6, "heapSize": 1141563392, "totalBytes": 1912521536 }, "list": { "bytes": 0, "concats": 103533, "elements": 0 }, "nrAvoided": 8090856, "nrExprs": 1469182, "nrFunctionCalls": 6649159, "nrLookups": 3978975, "nrOpUpdateValuesCopied": 31774690, "nrOpUpdates": 607131, "nrPrimOpCalls": 3261148, "nrThunks": 11004466, "sets": { "bytes": 679170688, "elements": 40539768, "number": 1908400 }, "sizes": { "Attr": 16, "Bindings": 16, "Env": 8, "Value": 24 }, "symbols": { "bytes": 1016494, "number": 85193 }, "time": { "cpu": 3.241765022277832, "gc": 1.223, "gcFraction": 0.37726361768832245 }, "values": { "bytes": 426071736, "number": 17752989 } }List-concatenation-heavy evaluation
./nix-before/bin/nix eval --no-eval-cache --read-only --json --file ./nixpkgs-91bc7dadf7bf0bf3fb9203e611bd856f40fc2b66/pkgs/top-level/release-attrpaths-superset.nix names./nix-after/bin/nix eval --no-eval-cache --read-only --json --file ./nixpkgs-91bc7dadf7bf0bf3fb9203e611bd856f40fc2b66/pkgs/top-level/release-attrpaths-superset.nix names$ NIX_SHOW_STATS_PATH="$PWD/list-concat-eval-bench-before.json" NIX_SHOW_STATS=1 ./nix-before/bin/nix eval --no-eval-cache --read-only --json --file ./nixpkgs-91bc7dadf7bf0bf3fb9203e611bd856f40fc2b66/pkgs/top-level/release-attrpaths-superset.nix names &> /dev/null{ "cpuTime": 15.477017402648926, "envs": { "bytes": 1025735272, "elements": 83935767, "number": 44281142 }, "gc": { "cycles": 10, "heapSize": 7333933056, "totalBytes": 10287749840 }, "list": { "bytes": 131652904, "concats": 1976668, "elements": 16456613 }, "nrAvoided": 47813368, "nrExprs": 3631225, "nrFunctionCalls": 38051112, "nrLookups": 18372127, "nrOpUpdateValuesCopied": 207325193, "nrOpUpdates": 10128855, "nrPrimOpCalls": 14286327, "nrThunks": 72867737, "sets": { "bytes": 4695210432, "elements": 276587147, "number": 16863505 }, "sizes": { "Attr": 16, "Bindings": 16, "Env": 8, "Value": 24 }, "symbols": { "bytes": 1903646, "number": 157132 }, "time": { "cpu": 15.477017402648926, "gc": 7.973, "gcFraction": 0.5151509359054803 }, "values": { "bytes": 2496420984, "number": 104017541 } }$ NIX_SHOW_STATS_PATH="$PWD/list-concat-eval-bench-after.json" NIX_SHOW_STATS=1 ./nix-after/bin/nix eval --no-eval-cache --read-only --json --file ./nixpkgs-91bc7dadf7bf0bf3fb9203e611bd856f40fc2b66/pkgs/top-level/release-attrpaths-superset.nix names &> /dev/null{ "cpuTime": 18.587953567504883, "envs": { "bytes": 1025735272, "elements": 83935767, "number": 44281142 }, "gc": { "cycles": 12, "heapSize": 7954685952, "totalBytes": 12852417616 }, "list": { "bytes": 0, "concats": 247488, "elements": 0 }, "nrAvoided": 47813368, "nrExprs": 3631225, "nrFunctionCalls": 38051112, "nrLookups": 18372127, "nrOpUpdateValuesCopied": 207325193, "nrOpUpdates": 10128855, "nrPrimOpCalls": 14286327, "nrThunks": 72867737, "sets": { "bytes": 4695210432, "elements": 276587147, "number": 16863505 }, "sizes": { "Attr": 16, "Bindings": 16, "Env": 8, "Value": 24 }, "symbols": { "bytes": 1903646, "number": 157132 }, "time": { "cpu": 18.587953567504883, "gc": 8.347, "gcFraction": 0.4490542743011835 }, "values": { "bytes": 2579421624, "number": 107475901 } }Priorities and Process
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.