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

Get rid of Hash::dummy from BinaryCacheStore #3935

Merged

Conversation

@Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Aug 15, 2020

CC @roberth. I think between this BinaryCacheStore::addToStoreCommon and your RemoteStore::addCAToStore we are narrowing in on "the one addToStore to rule them all".

@Ericson2314 Ericson2314 force-pushed the binary-cache-addToStoreFromDump branch from 98d3461 to 9fbc31a Sep 23, 2020
@Ericson2314 Ericson2314 changed the title WIP: Get rid of Hash::dummy from BinaryCacheStore Get rid of Hash::dummy from BinaryCacheStore Sep 23, 2020
Copy link
Member

@roberth roberth left a comment

Implementing addCAToStore might make sense if it's easy to implement, but it will need extra tests because it's not part of "normal" Nix use afaict.

RepairFlag repair, CheckSigsFlag checkSigs)
{
if (!repair && isValidPath(info.path)) {
// FIXME: copyNAR -> null sink
Copy link
Member

@roberth roberth Sep 23, 2020

We don't need to parse the NAR to determine the end if we make the caller responsible for ending narSource. That's what addCAToStore is doing.

Suggested change
// FIXME: copyNAR -> null sink
// FIXME: make sure all callers truncate `narSource`

nix-store --import comes to mind. It will have to parse the NAR because the import/export format doesn't have a way to determine the end by simpler means.

Copy link
Member Author

@Ericson2314 Ericson2314 Sep 23, 2020

I think agree it's better to make the caller responsible, but I'm a bit wary on changing the direction of this FIXME as it and this code already existed, I just moved it here.

I'll let @edolstra decide :).

src/libstore/binary-cache-store.hh Outdated Show resolved Hide resolved
src/libstore/binary-cache-store.cc Outdated Show resolved Hide resolved
Ericson2314 and others added 5 commits Sep 23, 2020
Co-authored-by: Robert Hensing <roberth@users.noreply.github.com>
We don't need it yet, but we could/should in the future, and it's a
cost-free change since we already have the reference. I like it.

Co-authored-by: Robert Hensing <roberth@users.noreply.github.com>
@Ericson2314 Ericson2314 force-pushed the binary-cache-addToStoreFromDump branch from 102063b to 1832436 Sep 26, 2020
Copy link
Member

@roberth roberth left a comment

1× fixme
1× store api and binary cache idea. I'll make an issue for that

src/libstore/binary-cache-store.cc Outdated Show resolved Hide resolved
std::shared_ptr<FSAccessor> narAccessor;
HashSink narHashSink { htSHA256 };
{
FdSink fileSink(fdTemp.get());
Copy link
Member

@roberth roberth Sep 26, 2020

This is beyond the scope of this PR, but since we're revisiting these store API methods it's worth noting that we could optimize away the tmpfile if we know a little bit more in advance. Although optimizing away a tmpfile seems unimpressive, it changes the time taken from O(sum(steps)) to O(max(steps)), which is significant when compression and upload take similar amounts of time.

When the file is known to be small (low hanging fruit)

Add a size parameter to addToStoreCommon or use a fancy sink that only writes to file when it crosses a limit. This is similar to what LocalStore::addToStoreFromDump does.

When we know the nar hash in advance

For http binary caches this does require us to change the binary cache filenames to match uncompressed hashes, which seems to be equivalent and can only result in one-time duplication in existing caches when new paths are uploaded.
I don't know yet how IPFS caches fit into this picture, but if those can compress after hashing, this would be beneficial.
Another reason to do this is so we don't need to compress before we can decide to reuse an available nar file.

In this case it does make sense to have both addToStore(const ValidPathInfo & info, .....) and addToStore(....., std::function<ValidPathInfo(HashResult)) where the prior can have a default implementation in terms of the latter.

Copy link
Member

@edolstra edolstra Sep 28, 2020

I decided not to try to optimize away the temporary file for small NARs because the overhead is likely to be insignificant compared to stuff like HTTP requests.

Co-authored-by: Robert Hensing <roberth@users.noreply.github.com>
src/libstore/binary-cache-store.cc Outdated Show resolved Hide resolved
Co-authored-by: Eelco Dolstra <edolstra@gmail.com>
@Ericson2314
Copy link
Member Author

@Ericson2314 Ericson2314 commented Sep 28, 2020

@edolstra OK this is all ready.

@edolstra edolstra merged commit 51c2992 into NixOS:master Oct 5, 2020
2 checks passed
2 checks passed
@github-actions[bot]
tests (ubuntu-latest)
Details
@github-actions[bot]
tests (macos-latest)
Details
@Ericson2314 Ericson2314 deleted the binary-cache-addToStoreFromDump branch Oct 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants