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

addMultipleToStore(): Move pathsToCopy #12296

Merged
merged 2 commits into from
Jan 20, 2025

Conversation

edolstra
Copy link
Member

@edolstra edolstra commented Jan 20, 2025

Motivation

This allows RemoteStore::addMultipleToStore() to free the Source objects early (and in particular the associated sinkToSource() buffers). This should fix #7359. For example, memory consumption of

$ nix copy --derivation --to ssh-ng://localhost?remote-store=/tmp/nix --derivation --no-check-sigs \
    /nix/store/4p9xmfgnvclqpii8pxqcwcvl9bxqy2xf-nixos-system-...drv

went from 353 MB to 74 MB.

This fix is an alternative to (or in addition to) #12255.

Context


Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

This allows RemoteStore::addMultipleToStore() to free the Source
objects early (and in particular the associated sinkToSource()
buffers). This should fix NixOS#7359. For example, memory consumption of

  nix copy --derivation --to ssh-ng://localhost?remote-store=/tmp/nix --derivation --no-check-sigs \
    /nix/store/4p9xmfgnvclqpii8pxqcwcvl9bxqy2xf-nixos-system-...drv

went from 353 MB to 74 MB.
@edolstra edolstra requested a review from Ericson2314 as a code owner January 20, 2025 13:26
@github-actions github-actions bot added the store Issues and pull requests concerning the Nix store label Jan 20, 2025
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.

A proper streaming abstraction might be nicer, but this solves the problem adequately.

src/libstore/remote-store.cc Show resolved Hide resolved
Co-authored-by: Robert Hensing <[email protected]>
@edolstra edolstra added backport 2.24-maintenance Automatically creates a PR against the branch backport 2.25-maintenance Automatically creates a PR against the branch labels Jan 20, 2025
@edolstra edolstra enabled auto-merge January 20, 2025 13:58
@edolstra edolstra merged commit 263a818 into NixOS:master Jan 20, 2025
10 checks passed
@edolstra edolstra deleted the release-source-early branch January 20, 2025 14:17
edolstra added a commit that referenced this pull request Jan 20, 2025
…2296

addMultipleToStore(): Move pathsToCopy (backport #12296)
edolstra added a commit that referenced this pull request Jan 20, 2025
…2296

addMultipleToStore(): Move pathsToCopy (backport #12296)
@roberth roberth added automatic backport This PR is a backport produced by automation (does not trigger backporting) and removed automatic backport This PR is a backport produced by automation (does not trigger backporting) labels Jan 20, 2025
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2024-01-20-nix-team-meeting-minutes-209/59119/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.24-maintenance Automatically creates a PR against the branch backport 2.25-maintenance Automatically creates a PR against the branch store Issues and pull requests concerning the Nix store
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Copying lots of paths fails with "too many root sets"
3 participants