Conversation
This is a mapping from paths to "resolved" paths (i.e. with `default.nix` added, if appropriate). `fileParseCache` and `fileEvalCache` are now keyed on the resolved path *only*.
Previously, the optimistic concurrency approach in `evalFile()` meant that a `nix search nixpkgs ^` would do hundreds of duplicated parsings/evaluations. Now, we reuse the thunk locking mechanism to ensure it's done only once.
This refactoring allows the symbol table to be stored as something other than std::strings.
This allows symbol IDs to be offsets into an arena whose base offset never moves, and can therefore be dereferenced without any locks.
This makes it less likely that we concurrently execute tasks that would block on a common subtask, e.g. evaluating `libfoo` and `libfoo_variant` are likely to have common dependencies.
Relevant paper with a design that implements this.
It's quite a different architecture, but maybe we need that? Specifically, they check blackholes on the stack during GC. I guess the alternative is to add more bookkeeping so that perhaps another thread could be tasked with periodically checking that no thread is deadlocked, and if they are, insert exception thunks, unblock them, and let them unwind their stacks. Fwiw, they have
It sure would be nice not to regress single-threaded performance, and have a boost in multi-threaded as well. I wouldn't oppose taking control of the stack instead of delegating all of that completely to bdwgc. Doing so would open the door to other benefits, such as more efficient |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
edolstra
force-pushed
the
multithreaded-eval
branch
from
a134740 to
d300e13
Compare
Also restore infinite recursion errors if parallel eval is not enabled.
|
@roberth @Ericson2314 was there more to be done on here? Reading through the history, it looked like there were some concerns about single-threaded performance regression, but was there something else that makes this not ready for prime-time yet? |
Blackholing is still missing, thus in case of infinite recursion nix is just freezing on this branch, instead of throwing an error. |
Is that error condition easy to provoke, or a nice-to-have? |
Easy to provoke and very important for users. |
| const char * c_str() const | ||
| { | ||
| return s->c_str(); | ||
| return s.data(); |
There was a problem hiding this comment.
String views are not guaranteed to be null-terminated, so this might be incorrect in certain cases.
|
Hi! Really looking forward to this. Are there any plans on releasing it soon? |
|
Closing this PR because it's outdated. An up-to-date version based on the new 16-byte |
Why is the PR made against https://github.com/DeterminateSystems/nix-src and not https://github.com/NixOS/nix? |
Is this surprising? Determinate Systems stands to directly monetarily benefit from competing with the upstream Nix implementation. Why then would you expect its employees to act against their interests by prioritizing upstream contributions? This is not obviously an entirely bad thing. The GPL means that any downstream contributions can eventually be upstreamed, so everyone stands to benefit from this work (and contributors have a right to try to monetize their contributions in the meantime). I am just confused as to why everybody seems to expect the DetNix contributors to act against their own self-interest---maybe there is something else here that I am misunderstanding. |
13 participants
Motivation
This PR makes the evaluator thread-safe. Currently, only
nix flake searchandnix flake showmake use of multi-threaded evaluation to achieve a speedup on multicore systems.Unlike the previous attempt at a multi-threaded evaluator, this one locks thunks to prevent them from being evaluated more than once. The life of a thunk is now:
forceValue()call, the thunk type goes fromtThunktotPending.forceValue()on a thunk in thetPendingstate, it acquires a lock to register itself as "awaiting" that value, and sets the type totAwaited.tAwaited, it updates the value and wakes up the threads that are waiting. If the type istPending, it just updates the value normally.Also, there now is a
tFailedvalue type that stores an exception pointer to represent the case where thunk evaluation throws an exception. In that case, every thread that forces the thunk should get the same exception.To enable multi-threaded evaluation, you need to set the
NR_CORESenvironment variable to the number of threads to use. You can also setNIX_SHOW_THREAD_STATS=1to get some debug statistics.Some benchmark results on a Ryzen 5900X with 12 cores and 24 hyper-threads:
GC_INITIAL_HEAP_SIZE=8G nix flake show --eval-cores 12 --no-eval-cache --all-systems --json github:NixOS/nix/afdd12be5e19c0001ff3297dea544301108d298went from 23.70s to 5.77s.GC_INITIAL_HEAP_SIZE=6G nix search --eval-cores 16 --no-eval-cache github:NixOS/nixpkgs/bf8462aeba50cc753971480f613fbae0747cffc0?narHash=sha256-bPyv7hsbtuxyL6LLKtOYL6QsmPeFWP839BZQMd3RoUg%3D ^went from 11.82s to 3.88s.Note: it's good to set
GC_INITIAL_HEAP_SIZEto a high value because stop-the-world garbage collection is expensive.To do:
nix flake check.Executorclass currently executes work items in random order to reduce the probability that we execute a bunch of items at the same time that all depend on the same thunk, causing all but one to be blocked. This can probably be improved.Context
Priorities and Process
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.