Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Constant space addToStoreFromDump and deduplicate code #3801

Merged
merged 15 commits into from
Jul 21, 2020

Conversation

Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Jul 11, 2020

addToStoreFromDump now takes a stream just like addToStore from path, and their LocalStore implementations are deduplicated.

I suggest reviewing commit by commit.

(There is a FIXME about removing addToStoreFromDump altogether, but we can't do that just yet. I don't believe this PR will make doing so harder.)

@Ericson2314
Copy link
Member Author

CC @matthewbauer

@edolstra
Copy link
Member

What's the purpose of demux and the wanted argument?

@Ericson2314
Copy link
Member Author

Demux was the way I found to share the code. I would have rather not made addToStoreCommon and just make addToStore use addToStoreFromDump, but I couldn't figure out how to do that except by using two sinkToSource calls and coroutines.

wanted is so for addToStoreFromDump in the sinkToSource call I know how data is being requested from the source. This I can take advantage because the whole addToStoreFromDump is "source to sink to source".

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
Copy link
Member Author

@edolstra In short, if the LambdaSink in local store can become a LambdaSource, then this callback hell can go away.

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.
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
@Ericson2314
Copy link
Member Author

@edolstra Kept on pushing on it, and wanted and demux are both gone! The trick was making ChainSource, after which I could greatly simplify many things.

We use this to simplify `LocalStore::addToStoreFromDump`.

Also, hope I fixed build error with old clang (used in Darwin CI).
Ericson2314 added a commit to obsidiansystems/nix that referenced this pull request Jul 19, 2020
or the original source is empty */
while (dump.size() < settings.narBufferSize) {
auto oldSize = dump.size();
constexpr size_t chunkSize = 1024;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
constexpr size_t chunkSize = 1024;
constexpr size_t chunkSize = 65536;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this is now corrected in #3844

@edolstra
Copy link
Member

There's a merge conflict in daemon.cc, otherwise LGTM.

@edolstra
Copy link
Member

Hm, there's a failure in tests/add.sh.

@Ericson2314
Copy link
Member Author

https://github.com/NixOS/nix/runs/890820640 oh great, it's master.

@edolstra
Copy link
Member

Looks like ac2fc7b caused it.

@edolstra edolstra merged commit 0835447 into NixOS:master Jul 21, 2020
@Ericson2314 Ericson2314 deleted the from-dump-stream branch July 21, 2020 13:40
meditans added a commit to obsidiansystems/nix that referenced this pull request Jul 21, 2020
This was a suggested course of action in a review in one of our earlier
commits, NixOS#3801 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants