Share evaluation caches across installables#10570
Conversation
layus
changed the title
|
This may resolve #9724 |
|
CI timeout 😿 |
|
LGTM, however it's a workaround for the underlying issue, which is that we don't memoize evaluation across installable. If I understand it correctly, this PR only fixes it for the case where the flake has a fingerprint (e.g. dirty Git workdirs don't) and pure evaluation and the eval cache are enabled. Ideally we would merge together installables for the same flake somehow so that they share a |
Yes, exactly. I did not want to spend too much time on that feature, and I also did not want to enter bike-shedding area. There are plenty of such unsolved questions:
So yeah, this is an imcomplete workaround, but at least I know this one is safe 😅 . That being said, if there is a clear idea of how this must be implemented, I am ready to do it 💪 |
As per Eelco's review comments Co-authored-by: Eelco Dolstra <[email protected]>
|
@layus Amazing... thanks! |
|
Can we get a backport for this? |
Share evaluation caches across installables Before: $ rm -rf ~/.cache/nix && time -f '%E' nix build --dry-run \ 'nixpkgs#hello' \ 'nixpkgs#clang' \ 'nixpkgs#cargo' \ 'nixpkgs#rustup' \ 'nixpkgs#bear' \ 'nixpkgs#firefox' \ 'nixpkgs#git-revise' \ 'nixpkgs#hyperfine' \ 'nixpkgs#curlie' \ 'nixpkgs#xz' \ 'nixpkgs#ripgrep' 0:03.61 After: $ rm -rf ~/.cache/nix && time -f '%E' nix build --dry-run \ 'nixpkgs#hello' \ 'nixpkgs#clang' \ 'nixpkgs#cargo' \ 'nixpkgs#rustup' \ 'nixpkgs#bear' \ 'nixpkgs#firefox' \ 'nixpkgs#git-revise' \ 'nixpkgs#hyperfine' \ 'nixpkgs#curlie' \ 'nixpkgs#xz' \ 'nixpkgs#ripgrep' 0:01.46 This could probably use a more proper benchmark... Fixes NixOS#313 (cherry picked from commit de51e5c) Change-Id: I9350bebd462b6af12c51db5bf432321abfe84a16
- Fix eval cache not being persisted in `nix develop` (since NixOS#10570) - Fallback to flake path so the eval cache isn't disabled for flakes evaluated by a relative `path:` (since NixOS#10149, NixOS#10176) - Don't attempt to commit cache transaction if there is no active transaction, which will spew errors in edge cases - Drive-by: trivial typo fix
- Fix eval cache not being persisted in `nix develop` (since NixOS#10570) - Don't attempt to commit cache transaction if there is no active transaction, which will spew errors in edge cases - Drive-by: trivial typo fix
6 participants
Motivation
I noticed that building multiple installables from the same flake did not share
the same root
Value. This has significant performance drawbacks that this PRaddresses by reusing said value for all the attribute paths accessed in the
same source.
Nix takes 4 minutes to evaluate (create the .drv files for) 276 packages from
the same flake, which amounts to .9s per installable. With this patch, the
whole evaluation takes 11s. That is twenty times faster. Of course, the overall
speedup is related to the amount of packages and the numbers here are very
specific and imprecise. But still.
This needs a proper review because I have no idea if this is the right approach.
Ideally, I would like the evaluation cache to be shared. Not cached and retrieved.
[edit: moved to #10590]
As this opens the gate to parallel evaluation for even better speedups, I also
discovered that our database transactions span the whole duration of the
evaluation, which became even longer now that the database is kept during the
evaluation of all the installables. I have tried to reduce the scope of the
transactions given that it is a cache. Not sure if it has any impact at all.
[/edit]
This has been extensively tested already, so at least it works as expected.
Note that this has no impact on the racy behavior of
path:installables.Since the code fetches them for every occurrence, they still can end up with
a different hash and a different cache. Arguably, a better implementation like
the once outlined above would address this by loading each source only once.
Fixes #9724
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.