Skip to content

Conversation

@xokdvium
Copy link
Contributor

@xokdvium xokdvium commented Dec 2, 2025

…ctSink

Motivation

This leads to incredibly wasteful refreshes (see ^) when oids are not found. Since we are writing the pack files only once per unpacking we should not bother with this refreshing at all.

This brings down the number of syscalls during nix flake metadata "https://releases.nixos.org/nixos/25.05/nixos-25.05.813095.1c8ba8d3f763/nixexprs.tar.xz" --store "dummy://?read-only=false"

Down from 576334 to just 6235 (100x less syscalls):

(Before)

% time     seconds  usecs/call     calls    errors syscall
------ ----------- ----------- --------- --------- ------------------
 32.98    0.625288           3    162898           getdents64
 29.58    0.560686           3    163514     81917 openat
 15.01    0.284509           3     81819       186 newfstatat
 10.99    0.208349           2     81601           close
 10.56    0.200145           2     81552           fstat

All these are coming from 2 and are totally useless.

(After)

% time     seconds  usecs/call     calls    errors syscall
------ ----------- ----------- --------- --------- ------------------
 76.47    0.108558         247       438        20 futex
  6.55    0.009292          18       513           munmap
  3.30    0.004680           7       639       492 openat
  2.68    0.003803          10       359           write
  2.30    0.003268           2      1146           read
  2.26    0.003215           3       870           mmap

Context

Natural follow-up to #14689.


Add 👍 to pull requests you find important.

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

@xokdvium xokdvium requested a review from edolstra as a code owner December 2, 2025 21:29
@github-actions github-actions bot added the fetching Networking with the outside (non-Nix) world, input locking label Dec 2, 2025
@xokdvium xokdvium force-pushed the even-faster-tarball-cache branch from 77473ff to 6fd8f4c Compare December 2, 2025 21:47
@xokdvium
Copy link
Contributor Author

xokdvium commented Dec 2, 2025

Impact on my machine (with the on zfs on an nvme ssd, I expect this to be much more pronounced on systems with slower drives):

nix flake metadata "https://releases.nixos.org/nixos/25.05/nixos-25.05.813095.1c8ba8d3f763/nixexprs.tar.xz" --store "dummy://?read-only=false"

(Before)

        User time (seconds): 21.28
        System time (seconds): 1.25
        Percent of CPU this job got: 160%
        Elapsed (wall clock) time (h:mm:ss or m:ss): 0:13.99

(After)

        User time (seconds): 21.00
        System time (seconds): 0.44
        Percent of CPU this job got: 164%
        Elapsed (wall clock) time (h:mm:ss or m:ss): 0:13.01

…ctSink

This leads to incredibly wasteful refreshes (see [^]) when oids are not found.
Since we are writing the pack files only once per unpacking we should not bother
with this refreshing at all.

This brings down the number of syscalls during `nix flake metadata "https://releases.nixos.org/nixos/25.05/nixos-25.05.813095.1c8ba8d3f763/nixexprs.tar.xz" --store "dummy://?read-only=false"`

Down from 576334 to just 6235 (100x less syscalls):

(Before)

% time     seconds  usecs/call     calls    errors syscall
------ ----------- ----------- --------- --------- ------------------
 32.98    0.625288           3    162898           getdents64
 29.58    0.560686           3    163514     81917 openat
 15.01    0.284509           3     81819       186 newfstatat
 10.99    0.208349           2     81601           close
 10.56    0.200145           2     81552           fstat

All these are coming from [2] and are totally useless.

(After)

% time     seconds  usecs/call     calls    errors syscall
------ ----------- ----------- --------- --------- ------------------
 76.47    0.108558         247       438        20 futex
  6.55    0.009292          18       513           munmap
  3.30    0.004680           7       639       492 openat
  2.68    0.003803          10       359           write
  2.30    0.003268           2      1146           read
  2.26    0.003215           3       870           mmap

[^]: https://github.com/libgit2/libgit2/blob/58d9363f02f1fa39e46d49b604f27008e75b72f2/include/git2/sys/odb_backend.h#L68-L75
[2]: https://github.com/libgit2/libgit2/blob/58d9363f02f1fa39e46d49b604f27008e75b72f2/src/libgit2/odb_pack.c#L517-L546
@xokdvium xokdvium force-pushed the even-faster-tarball-cache branch from 6fd8f4c to d1f9fe9 Compare December 3, 2025 00:33
@xokdvium xokdvium marked this pull request as ready for review December 3, 2025 00:44
@Ericson2314 Ericson2314 added this pull request to the merge queue Dec 3, 2025
unset it in the vtable. libgit2 does nothing if it's a nullptr:
https://github.com/libgit2/libgit2/blob/58d9363f02f1fa39e46d49b604f27008e75b72f2/src/libgit2/odb.c#L1922
*/
packfileOdbRefresh = std::exchange(backend->refresh, nullptr);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this different than

packfileOdbRefresh = backend->refresh;
backend->refresh = nullptr;

?

Comment on lines +1343 to +1344
/* We are done writing blobs, can restore refresh functionality. */
backend->refresh = packfileOdbRefresh;
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume that the status of repo->packBackend never changes for a given repo?

Merged via the queue into master with commit 96d8b54 Dec 3, 2025
20 checks passed
@Ericson2314 Ericson2314 deleted the even-faster-tarball-cache branch December 3, 2025 16:30
@Mic92
Copy link
Member

Mic92 commented Dec 6, 2025

I don't know if it's this or one of the other git optimizations, but I see errors about missing commits from repos. Unfortunately I didn't copy the exact error and now it went away. will do the next time i see it.

@Mic92
Copy link
Member

Mic92 commented Dec 6, 2025

Unclear now if this was the actual root cause or it's an issue maybe fixed by #14620

Will do more testing after I rebased again.

@edolstra edolstra mentioned this pull request Dec 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fetching Networking with the outside (non-Nix) world, input locking

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants