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 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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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 September 23, 2020 10:36
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>
Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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>
Co-authored-by: Eelco Dolstra <edolstra@gmail.com>
@Ericson2314
Copy link
Member Author

@edolstra OK this is all ready.

@edolstra edolstra merged commit 51c2992 into NixOS:master Oct 5, 2020
@Ericson2314 Ericson2314 deleted the binary-cache-addToStoreFromDump branch October 5, 2020 14:10
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.

None yet

3 participants