nar-accessor: Fix thread safety of seekableGetNarBytes, use Sink, reduce memory usage of nix store cat#15013
Merged
Ericson2314 merged 4 commits intomasterfrom Jan 19, 2026
Merged
Conversation
xokdvium
force-pushed
the
seekable-nar-bytes-thread-safe-pread
branch
2 times, most recently
from
a4992fe to
f9213a4
Compare
Ericson2314
reviewed
Jan 18, 2026
src/libutil/nar-accessor.cc
Outdated
| while (left) { | ||
| checkInterrupt(); | ||
| auto limit = std::min<decltype(buf)::size_type>(left, buf.size()); | ||
| #ifdef _WIN32 |
Member
There was a problem hiding this comment.
Now that this code is a bit more complex, can a pread/ReadFile wrapper can factored out into file-descriptor.hh? I think we already have the Windows/Unix implementation files for that too.
Ericson2314
approved these changes
Jan 18, 2026
Member
Ericson2314
left a comment
Ericson2314
left a comment
There was a problem hiding this comment.
Good to merge once factor out is done
Instead of mutating the file pointer we can instead safely do preads. That makes the local-nar-info cache once again thread safe without the overhead of reopening the file that we used to have prior to b9b6def which broke the thread safety by persisting the file descriptor.
This reduces the memory overhead of nix store cat down to constant memory with a local-nar-cache.
xokdvium
force-pushed
the
seekable-nar-bytes-thread-safe-pread
branch
from
f9213a4 to
253aec8
Compare
github-actions
bot
added
store
Contributor
Author
|
Fixed, also did some more optimizations and cleanups. Turns out we were unnecessarily buffering files in memory in the evaluator. |
xokdvium
force-pushed
the
seekable-nar-bytes-thread-safe-pread
branch
from
253aec8 to
3a00ce2
Compare
This factors out the helper function from seekableGetNarBytes into copyFdRange and adds some more sanity checks for offset/length truncation/wrapping at that API boundary where we work with NAR-style offsets and convert to native off_t.
…non-virtual This makes all addToStore operations that use these source accessors constant memory regardless of file sizes. Also make the other overload altogether and relegate it to the base class as a non-virtual method to avoid such mistakes.
xokdvium
force-pushed
the
seekable-nar-bytes-thread-safe-pread
branch
from
3a00ce2 to
4db68c2
Compare
Ericson2314
approved these changes
Jan 18, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Instead of mutating the file pointer we can instead safely do
preads. That makes the local-nar-info cache once again thread safe
without the overhead of reopening the file that we used to have prior
to b9b6def which broke the thread safety
by persisting the file descriptor.
The second and third commit reduces the memory usage of the source accessor with a local-nar-cache,
which pairs nicely with the recent refactor to support substituting from it.
Context
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.