Get rid of Hash::dummy from BinaryCacheStore#3935
Conversation
Ericson2314
force-pushed
the
binary-cache-addToStoreFromDump
branch
from
98d3461 to
9fbc31a
Compare
Ericson2314
changed the title
roberth
left a comment
roberth
left a comment
There was a problem hiding this 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 |
There was a problem hiding this comment.
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.
| // 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.
There was a problem hiding this comment.
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 :).
Co-authored-by: Robert Hensing <[email protected]>
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 <[email protected]>
Ericson2314
force-pushed
the
binary-cache-addToStoreFromDump
branch
from
102063b to
1832436
Compare
roberth
left a comment
roberth
left a comment
There was a problem hiding this comment.
1× fixme
1× store api and binary cache idea. I'll make an issue for that
| std::shared_ptr<FSAccessor> narAccessor; | ||
| HashSink narHashSink { htSHA256 }; | ||
| { | ||
| FdSink fileSink(fdTemp.get()); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 <[email protected]>
Co-authored-by: Eelco Dolstra <[email protected]>
|
@edolstra OK this is all ready. |
3 participants
CC @roberth. I think between this
BinaryCacheStore::addToStoreCommonand yourRemoteStore::addCAToStorewe are narrowing in on "the oneaddToStoreto rule them all".