Skip to content

Conversation

@pkpbynum
Copy link
Contributor

@pkpbynum pkpbynum commented Nov 14, 2025

Motivation

Copying closures between devices currently requires resolving the full closure on the "source" store. This is usually fast because the path info is cached, but for devices which are infrequently updated (so the cache is expired) this can add a significant amount of unnecessary work.

This PR adds a callback to computeFSClosure that allows it to short-circuit, and then uses it from copyClosure to query only the set of missing paths. I tested this by deleting my narinfo cache & downloading a closure from HTTP cache with 1442 paths, where 1396 of those paths were already available locally.

Before this PR it took 5m 14s, where resolving all the *.narinfo files took ~4 minutes before any download started.

After this PR it took 1m 12s, with downloads starting essentially instantly.

Context

I am working through the C API, but it would be great if this also applied to the nix copy CLI, but it's a pretty different code path that made this PR a lot more complicated.


Add 👍 to pull requests you find important.

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

@github-actions github-actions bot added store Issues and pull requests concerning the Nix store c api Nix as a C library with a stable interface labels Nov 14, 2025
@Radvendii
Copy link
Contributor

Discussed in the nix maintainer coworking call. My summary:

  1. From @Ericson2314: nix code is a mess. There's different versions, it's not systematic, it should be cleaned up with double dynamic dispatch. This isn't built into C++, but we can try to dynamic_cast each store and choose an implementation based on both.

  2. From @Radvendii (me): We should make sure that this doesn't speed up the case of "fast destination, slow source" at the expense of slowing down "slow destination, fast source" (or at least, we should check whether that's the case and make an informed decision)

(1) shouldn't block this PR, but for (2) we should test the opposite case and choose what to do based on the results.

Copy link
Member

Choose a reason for hiding this comment

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

C API could be extended when the need arises. Now seems premature, so this impl-only change looks perfect to me.

@pkpbynum
Copy link
Contributor Author

Discussed in the nix maintainer coworking call. My summary:

  1. From @Ericson2314: nix code is a mess. There's different versions, it's not systematic, it should be cleaned up with double dynamic dispatch. This isn't built into C++, but we can try to dynamic_cast each store and choose an implementation based on both.
  2. From @Radvendii (me): We should make sure that this doesn't speed up the case of "fast destination, slow source" at the expense of slowing down "slow destination, fast source" (or at least, we should check whether that's the case and make an informed decision)

(1) shouldn't block this PR, but for (2) we should test the opposite case and choose what to do based on the results.

re (2): I didn't test it because tbh I don't know how to artificially create a "slow destination." But I'm fairly confident this is a strict improvement. The code path currently is:

  1. Query the full closure (N paths) on the source (computeFSClosure)
  2. Filter all N paths against the destination (queryValidPaths)
  3. Only copy the missing subset

So today it's always doing 2N queries before transferring any NARs. With this PR that is strictly lower. Maybe there is some cost in context switching between the two, but I don't think it's consequential.

As an alternative implementation, I was thinking about implementing a separate Store::queryMissingClosure method which takes an additional "reference store" parameter. Since copying between two stores is a fairly unique operation, it might make sense to just have a separate, optimized utility function rather than extending the already-overloaded computeFSClosure.

Copy link
Contributor

@xokdvium xokdvium left a comment

Choose a reason for hiding this comment

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

One small comment. Also Eelcos concern seems valid, though it only affects the legacy ssh store case.

@pkpbynum pkpbynum force-pushed the pb/short-circuit-copy-fs-closure branch from 293230b to 5d13b86 Compare November 21, 2025 02:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c api Nix as a C library with a stable interface store Issues and pull requests concerning the Nix store

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants