[perf] Skip cache check if store path exists locally #2042
Conversation
|
|
||
| outputInCache := map[string]bool{} // key = output name, value = in cache | ||
| for _, output := range outputs { | ||
| // if store path exists locally, then it is equivalent to being in the cache. |
There was a problem hiding this comment.
I think this is correct, assuming the nix order for resolving a store-path is:
- check local /nix/store to see if already built
- check binary cache(s) configured to see if binaries are present
- build derivation locally
I say assuming because looking at the docs for builtins.fetchClosure it doesn't say that it will check local nix-store and skip the binary cache if its present:
https://nixos.org/manual/nix/stable/language/builtins.html#builtins-fetchClosure
There was a problem hiding this comment.
I assume it works this way, because otherwise we would be re-downloading everything every time the flake is evaluated. Currently we build everything first (which stores it int the nix store) and then we evaluate the flake using print-dev-env.
Also, see this note:
fetchClosure is similar to builtins.storePath in that it allows you to use a previously built store path in a Nix expression. However, fetchClosure is more reproducible because it specifies a binary cache from which the path can be fetched
builtins.storePath always checks the local store first before using substituters, so I assume fetchClosure does the same before trying to fetch from remote.
cc: @gcurtis let us know if this is your understanding as well.
There was a problem hiding this comment.
That's also my understanding.
This reverts commit ec0a703.
Reverts #2042 #2042 introduced a bug where adding non-cached packages would fail to add them to the flake on the first attempt. (it would succeed on subsequent attempts after the package is already in the store). The root cause is that `fetchNarInfoStatus` gets called twice per package (even though we cache it and we're not supposed to call it twice). The first call returns `false` because the package is not cached. The second call returns `true` because the package is already in store. This discrepancy essentially causes the package not to appear on the flake at all. When updating state again, the package is already in the nix store so both `fetchNarInfoStatus` calls return true. I think reverting this is best immediate course of action. In follow up we should fix `fetchNarInfoStatus` so it only gets called once (it will improve performance and is more correct). Later on we can think of better way to do #2042. The current implementation is a bit fragile and not 100% consistent with the initial intention of the function, so I'm concerned it can lead to more bugs in the future.
Summary
If store path exists locally, we know we can use that store path in
fetchClosure. This helps speed up almost any operation where state is not up to date and we need to check if some store path is in cache.How was it tested?
devbox run echo 5