Constant space addToStoreFromDump and deduplicate code
#3801
Conversation
|
What's the purpose of |
|
Demux was the way I found to share the code. I would have rather not made wanted is so for |
I just as little beyond the type as possible, so the implementation changes this enables can be reviewed separately.
The downsides is that the coroutine has byte-by-byte loop transfer. Will fix that next.
Rather than copying byte-by-byte, we let the coroutine know how much data we would like it to send back to us.
Ericson2314
force-pushed
the
from-dump-stream
branch
from
d384c03
to
592851f
Compare
|
@edolstra In short, if the |
We were calculating the nar hash wrong when the file ingestion method was flat. I don't think there's anything we can do in that case but dump the file again, so that's what I do. As an optomization, we again could reuse the original dump for just the recursive and non-sha256 case, but I rather do that after this fix, and after my other PRs which deduplicate this code.
This reverts commit cff2157.
I got it to just become `LocalStore::addToStoreFromDump`, cleanly taking a store and then doing nothing too fancy with it. `LocalStore::addToStore(...Path...)` is now just a simple wrapper with a bare-bones sinkToSource of the right dump command.
This reverts commit 592851f. We don't need this extra feature anymore
|
@edolstra Kept on pushing on it, and |
We use this to simplify `LocalStore::addToStoreFromDump`. Also, hope I fixed build error with old clang (used in Darwin CI).
Merge NixOS#3801 Constant space `addToStoreFromDump`
| or the original source is empty */ | ||
| while (dump.size() < settings.narBufferSize) { | ||
| auto oldSize = dump.size(); | ||
| constexpr size_t chunkSize = 1024; |
There was a problem hiding this comment.
| constexpr size_t chunkSize = 1024; | |
| constexpr size_t chunkSize = 65536; |
There was a problem hiding this comment.
Thanks, this is now corrected in #3844
|
There's a merge conflict in daemon.cc, otherwise LGTM. |
|
Hm, there's a failure in tests/add.sh. |
|
https://github.com/NixOS/nix/runs/890820640 oh great, it's master. |
|
Looks like ac2fc7b caused it. |
…systems/nix into from-dump-stream
This was a suggested course of action in a review in one of our earlier commits, NixOS#3801 (comment)
addToStoreFromDumpnow takes a stream just likeaddToStorefrom path, and theirLocalStoreimplementations are deduplicated.I suggest reviewing commit by commit.
(There is a
FIXMEabout removingaddToStoreFromDumpaltogether, but we can't do that just yet. I don't believe this PR will make doing so harder.)