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

replace in-memory nar caching with on-disk nar caching when >256MB #2981

Closed
wants to merge 15 commits into from

Conversation

yorickvP
Copy link
Contributor

@yorickvP yorickvP commented Jul 7, 2019

Nix runs out of memory when dumping large files on systems without enough ram, and the whole saving and hashing is not as efficient as it could be (multiple passes through files, multiple copies). I'm out of time for now, but this fixes all the main problematic cases. It would be very cool to just pass FD's over to the nix daemon where possible, zero-copying into the nix store on filesystems with reflinks.

The one in RemoteFSAccessor::fetch still remains.

In more detail, this introduces a HashedBufferSink sink that buffers data in memory, then switches to a file when the data gets too big. Afterwards, it can be converted to a Source. It also keeps a hash of the data. It also switches some of the code to std::string_view, so we can use file-backed memory via mmap there.
There also seemed to be an accessor argument to addToStore that was never actually used and complicated the BinaryCacheStore code quite a bit, I've removed that.
These changes do not seem to be covered well by the tests, but I've run some builds and nix add-to-store's to verify the output and behavior is the same (minus the running out of memory).

cc @Pitometsu

@edolstra
Copy link
Member

edolstra commented Jul 8, 2019

Hm, I did something similar a while ago in cf4a7ae. I would prefer that one since it's a lot simpler (it touches only 2 files rather than 31).

@yorickvP
Copy link
Contributor Author

yorickvP commented Jul 8, 2019

Most of the changes to other files are simple refactorings (std::string -> std::string_view, removing the accessor from addToStore). They could be split out of this PR if this helps.
Making the maximum in-memory buffer size configurable is a good idea. While your patch is more elegant when this functionality is only needed in a single function, I think there are quite a few places where in-memory buffering is used and nix could run out of memory.
My patch touches the following locations:

  • BinaryCacheStore::addToStore
  • sandbox creation when hardlinking is impossible
  • Store::importPaths
  • LocalStore::addToStoreFromDump
  • RemoteFSAccessor::addToCache
  • nix-daemon (wopAddToStore)
  • nix add-to-store

@Pitometsu
Copy link

Confirm: it work as expected for me.
Thanks, @yorickvP for hard work!

@Pitometsu
Copy link

@edolstra any progress here/chance to integrate? Nix from this PR just work for me.

@Pitometsu
Copy link

@edolstra any progress on merging/adopting it? AFAIK that fixes long-term nix issue with disability to store big files, which could be pretty useful in nix-as-a-development-environment workflow, where bunch of language-specific libraries yet installed by language-specific tool and reused as artifacts.

@domenkozar
Copy link
Member

I'd suggest openining a separate PR for refactoring and rebasing the rest on top.

@stale
Copy link

stale bot commented Feb 13, 2021

I marked this as stale due to inactivity. → More info

@domenkozar
Copy link
Member

Completely diverged with master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants