index-pack: retain child bases in delta cache#2131
Conversation
Welcome to GitGitGadgetHi @arijit91, and welcome to GitGitGadget, the GitHub App to send patch series to the Git mailing list from GitHub Pull Requests. Please make sure that either:
You can CC potential reviewers by adding a footer to the PR description with the following syntax: NOTE: DO NOT copy/paste your CC list from a previous GGG PR's description, Also, it is a good idea to review the commit messages one last time, as the Git project expects them in a quite specific form:
It is in general a good idea to await the automated test ("Checks") in this Pull Request before contributing the patches, e.g. to avoid trivial issues such as unportable code. Contributing the patchesBefore you can contribute the patches, your GitHub username needs to be added to the list of permitted users. Any already-permitted user can do that, by adding a comment to your PR of the form Both the person who commented An alternative is the channel Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment If you want to see what email(s) would be sent for a After you submit, GitGitGadget will respond with another comment that contains the link to the cover letter mail in the Git mailing list archive. Please make sure to monitor the discussion in that thread and to address comments and suggestions (while the comments and suggestions will be mirrored into the PR by GitGitGadget, you will still want to reply via mail). If you do not want to subscribe to the Git mailing list just to be able to respond to a mail, you can download the mbox from the Git mailing list archive (click the curl -g --user "<EMailAddress>:<Password>" \
--url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txtTo iterate on your change, i.e. send a revised patch or patch series, you will first want to (force-)push to the same branch. You probably also want to modify your Pull Request description (or title). It is a good idea to summarize the revision by adding something like this to the cover letter (read: by editing the first comment on the PR, i.e. the PR description): To send a new iteration, just add another PR comment with the contents: Need help?New contributors who want advice are encouraged to join git-mentoring@googlegroups.com, where volunteers who regularly contribute to Git are willing to answer newbie questions, give advice, or otherwise provide mentoring to interested contributors. You must join in order to post or view messages, but anyone can join. You may also be able to find help in real time in the developer IRC channel, |
|
/submit |
|
Error: User arijit91 is not yet permitted to use GitGitGadget |
|
/allow |
|
User arijit91 is now allowed to use GitGitGadget. |
|
/submit |
|
Submitted as pull.2131.git.1780070763044.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
This branch is now known as |
|
This patch series was integrated into seen via git@b581127. |
When resolving a delta whose result has children of its own,
index-pack adds the result to work_head, accounts its data in
base_cache_used, and calls prune_base_data(). It then immediately frees
that same data.
This bypasses the existing delta base cache policy and can force later
descendants to reconstruct the queued base again. Let the existing
delta_base_cache_limit pruning policy decide whether to keep or evict
the data instead.
This does not add a new cache or increase the cache limit. The object
data is already accounted in base_cache_used before prune_base_data()
runs, and the existing pruning and base cleanup paths still release it.
On a quiet Ubuntu 24.04 VM with 16 vCPUs, 32 GiB RAM, and local SSD,
direct index-pack timings on single-pack Linux fixtures improved as
follows:
linux blobless: 69.17s -> 57.98s (16.2% faster), RSS flat
linux full: 280.72s -> 236.32s (15.8% faster), RSS +1.9%
Five-repeat medians on public repositories also improved:
git.git: 12.31s -> 10.70s (13.1% faster)
libgit2: 3.35s -> 2.88s (14.0% faster)
redis: 6.52s -> 5.64s (13.5% faster)
cpython: 33.02s -> 31.44s (4.8% faster)
The standard p5302 perf test on a smaller git.git fixture was neutral:
5302.9 index-pack default threads:
11.21(38.07+1.33) -> 11.16(37.90+1.31), -0.4%
t/t5302-pack-index.sh passed, and GitGitGadget's linux-leaks CI also
exercised that test under SANITIZE=leak.
Signed-off-by: Arijit Banerjee <arijit@effectiveailabs.com>
cafb170 to
42eca38
Compare
|
/submit |
|
Submitted as pull.2131.v2.git.1780330402264.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
Derrick Stolee wrote on the Git mailing list (how to reply to this email): On 5/29/2026 12:06 PM, Arijit Banerjee via GitGitGadget wrote:
> From: Arijit Banerjee <arijit@effectiveailabs.com>
>
> When resolving a delta whose result has children of its own,
> index-pack adds the result to work_head, accounts its data in
> base_cache_used, and calls prune_base_data(). It then immediately
> frees that same data.
>
> This bypasses the existing delta base cache policy and can force later
> descendants to reconstruct the queued base again. Let the existing
> delta_base_cache_limit pruning policy decide whether to keep or evict
> the data instead.
>
> Signed-off-by: Arijit Banerjee <arijit@effectiveailabs.com>
> ---
> index-pack: retain child bases in delta cache
>
> Speed up the local pack indexing phase of clone/fetch for large
> delta-compressed packs by keeping reconstructed delta bases available
> for reuse when they are queued for later delta resolution.
>
> When index-pack reconstructs a child base and queues it for resolving
> descendant deltas, it currently frees that data immediately. This can
> force the same base to be reconstructed again. Instead, keep it in the
> existing delta base cache and let the existing delta_base_cache_limit
> policy decide whether to retain or evict it.
>
> This does not add a new cache or increase the cache limit. The object
> data is already accounted in base_cache_used, and prune_base_data() is
> already called at this point.
>
> Correctness:
>
> * t/t5302-pack-index.sh passed all 36 tests.
Is there any chance that you ran this also with SANITIZE=leak to make
sure that we aren't introducing a memory leak? (It's hard to tell just
from the patch context, though your description is convincing.)
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2131%2Farijit91%2Findex-pack-retain-child-base-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2131/arijit91/index-pack-retain-child-base-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/2131
Indeed, this PR has a passing linux-leaks build that exercises this
test script. [1]
[1] https://github.com/gitgitgadget/git/actions/runs/26605615549/job/78399938323?pr=2131#step:9:1405
> Benchmarks on a quiet Ubuntu 24.04 VM, 16 vCPU, 32 GiB RAM, local SSD:
>
> pack baseline patched wall-time change RSS change linux blobless 69.17s
> 57.98s 16.2% faster -0.0% linux full 280.72s 236.32s 15.8% faster +1.9%
>
> Five-repeat public-repo medians also improved: git.git 13.1%, libgit2
> 14.0%, redis 13.5%, cpython 4.8%.
>
> Perf on the linux blobless pack showed the same direction under
> profiling: 76.64s baseline vs 61.09s patched, with similar RSS.
A lot of this information that is in your cover letter would be helpful
to include in your commit message, for posterity.
Also, I prefer to see performance numbers for these repos reflected in
results from our performance test suite. We have a test for this purpose,
so you could try running this from t/perf/ for your local copies of these
repos:
GIT_PERF_LARGE_REPO=<path> ./run HEAD~1 HEAD -- p5302-pack-index.sh
And this should result in a standard comparison table that will help
present your results in a way that is familiar to Git contributors.
> @@ -1212,7 +1212,6 @@ static void *threaded_second_pass(void *data)
> list_add(&child->list, &work_head);
> base_cache_used += child->size;
> prune_base_data(NULL);
> - free_base_data(child);
> } else if (child) {
A nice and simple change. Good find!
Thanks,
-Stolee
|
|
Hi Stolee,
Thanks for the suggestion. I ran the standard p5302-pack-index.sh perf
test using:
GIT_PERF_LARGE_REPO=<repo> ./run HEAD~1 HEAD -- p5302-pack-index.sh
on an Ubuntu 24.04 VM with 16 vCPU, 32 GiB RAM, and local SSD.
The results were:
5302.9: index-pack default number of threads
libgit2:
3.17(11.42+0.61) -> 2.69(10.49+0.32), -15.1%
redis:
5.88(15.77+0.63) -> 4.98(14.08+0.39), -15.3%
git.git:
11.22(38.31+1.29) -> 9.69(35.36+0.64), -13.6%
cpython:
32.64(117.70+4.39) -> 28.70(109.90+1.98), -12.1%
linux:
276.95(793.14+39.51) -> 234.75(722.33+18.01), -15.2%
I also included these results in the PR description.
Thanks,
Arijit
On Mon, Jun 1, 2026 at 11:29 AM gitgitgadget[bot] ***@***.***>
wrote:
… *gitgitgadget[bot]* left a comment (gitgitgadget/git#2131)
<#2131 (comment)>
*Derrick Stolee* wrote on the Git mailing list
***@***.***>
(how to reply to this email <https://gitgitgadget.github.io/reply-to-this>
):
On 5/29/2026 12:06 PM, Arijit Banerjee via GitGitGadget wrote:> From: Arijit Banerjee ***@***.***>> > When resolving a delta whose result has children of its own,> index-pack adds the result to work_head, accounts its data in> base_cache_used, and calls prune_base_data(). It then immediately> frees that same data.> > This bypasses the existing delta base cache policy and can force later> descendants to reconstruct the queued base again. Let the existing> delta_base_cache_limit pruning policy decide whether to keep or evict> the data instead.> > Signed-off-by: Arijit Banerjee ***@***.***>> ---> index-pack: retain child bases in delta cache> > Speed up the local pack indexing phase of clone/fetch for large> delta-compressed packs by keeping reconstructed delta bases available> for reuse when they are queued for later delta resolution.> > When index-pack reconstructs a child base and queues it for resolving> descendant deltas, it currently frees that data immediately. This can> force the same base to be reconstructed again. Instead, keep it in the> existing delta base cache and let the existing delta_base_cache_limit> policy decide whether to retain or evict it.> > This does not add a new cache or increase the cache limit. The object> data is already accounted in base_cache_used, and prune_base_data() is> already called at this point.> > Correctness:> > * t/t5302-pack-index.sh passed all 36 tests.
Is there any chance that you ran this also with SANITIZE=leak to make
sure that we aren't introducing a memory leak? (It's hard to tell just
from the patch context, though your description is convincing.)
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2131%2Farijit91%2Findex-pack-retain-child-base-v1> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2131/arijit91/index-pack-retain-child-base-v1> Pull-Request: #2131
Indeed, this PR has a passing linux-leaks build that exercises this
test script. [1]
[1] https://github.com/gitgitgadget/git/actions/runs/26605615549/job/78399938323?pr=2131#step:9:1405
> Benchmarks on a quiet Ubuntu 24.04 VM, 16 vCPU, 32 GiB RAM, local SSD:> > pack baseline patched wall-time change RSS change linux blobless 69.17s> 57.98s 16.2% faster -0.0% linux full 280.72s 236.32s 15.8% faster +1.9%> > Five-repeat public-repo medians also improved: git.git 13.1%, libgit2> 14.0%, redis 13.5%, cpython 4.8%.> > Perf on the linux blobless pack showed the same direction under> profiling: 76.64s baseline vs 61.09s patched, with similar RSS.
A lot of this information that is in your cover letter would be helpful
to include in your commit message, for posterity.
Also, I prefer to see performance numbers for these repos reflected in
results from our performance test suite. We have a test for this purpose,
so you could try running this from t/perf/ for your local copies of theserepos:
GIT_PERF_LARGE_REPO=<path> ./run HEAD~1 HEAD -- p5302-pack-index.sh
And this should result in a standard comparison table that will help
present your results in a way that is familiar to Git contributors.
> @@ -1212,7 +1212,6 @@ static void *threaded_second_pass(void *data)> list_add(&child->list, &work_head);> base_cache_used += child->size;> prune_base_data(NULL);> - free_base_data(child);> } else if (child) {
A nice and simple change. Good find!
Thanks,
-Stolee
—
Reply to this email directly, view it on GitHub
<#2131?email_source=notifications&email_token=AAGSQWCT2PNPRIFOCY77BTD45XDOZA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTINJZGUZTONBYGY22M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLDGN5XXIZLSL5RWY2LDNM#issuecomment-4595374865>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAGSQWHGUQFLLJ5TU3AKJAD45XDOZAVCNFSM6AAAAACZRSOMM6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DKOJVGM3TIOBWGU>
.
Triage notifications, keep track of coding agent tasks and review pull
requests on the go with GitHub Mobile for iOS
<https://github.com/notifications/mobile/ios/AAGSQWHPPZLLQ6IKMBFHFVT45XDOZA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTINJZGUZTONBYGY22M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJKTGN5XXIZLSL5UW64Y>
and Android
<https://github.com/notifications/mobile/android/AAGSQWDDNQF376CWQEJV4QD45XDOZA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTINJZGUZTONBYGY22M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLTGN5XXIZLSL5QW4ZDSN5UWI>.
Download it today!
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Improvement for the end to end full clone time was closer to 5-10% if I
measured it correctly, added those numbers as well
…On Mon, Jun 1, 2026 at 5:01 PM Arijit Banerjee ***@***.***> wrote:
Hi Stolee,
Thanks for the suggestion. I ran the standard p5302-pack-index.sh perf
test using:
GIT_PERF_LARGE_REPO=<repo> ./run HEAD~1 HEAD -- p5302-pack-index.sh
on an Ubuntu 24.04 VM with 16 vCPU, 32 GiB RAM, and local SSD.
The results were:
5302.9: index-pack default number of threads
libgit2:
3.17(11.42+0.61) -> 2.69(10.49+0.32), -15.1%
redis:
5.88(15.77+0.63) -> 4.98(14.08+0.39), -15.3%
git.git:
11.22(38.31+1.29) -> 9.69(35.36+0.64), -13.6%
cpython:
32.64(117.70+4.39) -> 28.70(109.90+1.98), -12.1%
linux:
276.95(793.14+39.51) -> 234.75(722.33+18.01), -15.2%
I also included these results in the PR description.
Thanks,
Arijit
On Mon, Jun 1, 2026 at 11:29 AM gitgitgadget[bot] <
***@***.***> wrote:
> *gitgitgadget[bot]* left a comment (gitgitgadget/git#2131)
> <#2131 (comment)>
>
> *Derrick Stolee* wrote on the Git mailing list
> ***@***.***>
> (how to reply to this email
> <https://gitgitgadget.github.io/reply-to-this>):
>
> On 5/29/2026 12:06 PM, Arijit Banerjee via GitGitGadget wrote:> From: Arijit Banerjee ***@***.***>> > When resolving a delta whose result has children of its own,> index-pack adds the result to work_head, accounts its data in> base_cache_used, and calls prune_base_data(). It then immediately> frees that same data.> > This bypasses the existing delta base cache policy and can force later> descendants to reconstruct the queued base again. Let the existing> delta_base_cache_limit pruning policy decide whether to keep or evict> the data instead.> > Signed-off-by: Arijit Banerjee ***@***.***>> ---> index-pack: retain child bases in delta cache> > Speed up the local pack indexing phase of clone/fetch for large> delta-compressed packs by keeping reconstructed delta bases available> for reuse when they are queued for later delta resolution.> > When index-pack reconstructs a child base and queues it for resolving> descendant deltas, it currently frees that data immediately. This can> force the same base to be reconstructed again. Instead, keep it in the> existing delta base cache and let the existing delta_base_cache_limit> policy decide whether to retain or evict it.> > This does not add a new cache or increase the cache limit. The object> data is already accounted in base_cache_used, and prune_base_data() is> already called at this point.> > Correctness:> > * t/t5302-pack-index.sh passed all 36 tests.
>
> Is there any chance that you ran this also with SANITIZE=leak to make
> sure that we aren't introducing a memory leak? (It's hard to tell just
> from the patch context, though your description is convincing.)
> > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2131%2Farijit91%2Findex-pack-retain-child-base-v1> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2131/arijit91/index-pack-retain-child-base-v1> Pull-Request: #2131
>
> Indeed, this PR has a passing linux-leaks build that exercises this
> test script. [1]
>
> [1] https://github.com/gitgitgadget/git/actions/runs/26605615549/job/78399938323?pr=2131#step:9:1405
> > Benchmarks on a quiet Ubuntu 24.04 VM, 16 vCPU, 32 GiB RAM, local SSD:> > pack baseline patched wall-time change RSS change linux blobless 69.17s> 57.98s 16.2% faster -0.0% linux full 280.72s 236.32s 15.8% faster +1.9%> > Five-repeat public-repo medians also improved: git.git 13.1%, libgit2> 14.0%, redis 13.5%, cpython 4.8%.> > Perf on the linux blobless pack showed the same direction under> profiling: 76.64s baseline vs 61.09s patched, with similar RSS.
> A lot of this information that is in your cover letter would be helpful
> to include in your commit message, for posterity.
>
> Also, I prefer to see performance numbers for these repos reflected in
> results from our performance test suite. We have a test for this purpose,
> so you could try running this from t/perf/ for your local copies of theserepos:
>
> GIT_PERF_LARGE_REPO=<path> ./run HEAD~1 HEAD -- p5302-pack-index.sh
>
> And this should result in a standard comparison table that will help
> present your results in a way that is familiar to Git contributors.
> > @@ -1212,7 +1212,6 @@ static void *threaded_second_pass(void *data)> list_add(&child->list, &work_head);> base_cache_used += child->size;> prune_base_data(NULL);> - free_base_data(child);> } else if (child) {
> A nice and simple change. Good find!
>
> Thanks,
> -Stolee
>
> —
> Reply to this email directly, view it on GitHub
> <#2131?email_source=notifications&email_token=AAGSQWCT2PNPRIFOCY77BTD45XDOZA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTINJZGUZTONBYGY22M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLDGN5XXIZLSL5RWY2LDNM#issuecomment-4595374865>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAGSQWHGUQFLLJ5TU3AKJAD45XDOZAVCNFSM6AAAAACZRSOMM6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DKOJVGM3TIOBWGU>
> .
> Triage notifications, keep track of coding agent tasks and review pull
> requests on the go with GitHub Mobile for iOS
> <https://github.com/notifications/mobile/ios/AAGSQWHPPZLLQ6IKMBFHFVT45XDOZA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTINJZGUZTONBYGY22M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJKTGN5XXIZLSL5UW64Y>
> and Android
> <https://github.com/notifications/mobile/android/AAGSQWDDNQF376CWQEJV4QD45XDOZA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTINJZGUZTONBYGY22M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLTGN5XXIZLSL5QW4ZDSN5UWI>.
> Download it today!
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>
|
|
This patch series was integrated into seen via git@b0c7b14. |
|
Jeff King wrote on the Git mailing list (how to reply to this email): On Mon, Jun 01, 2026 at 04:13:21PM +0000, Arijit Banerjee via GitGitGadget wrote:
> When resolving a delta whose result has children of its own,
> index-pack adds the result to work_head, accounts its data in
> base_cache_used, and calls prune_base_data(). It then immediately frees
> that same data.
>
> This bypasses the existing delta base cache policy and can force later
> descendants to reconstruct the queued base again. Let the existing
> delta_base_cache_limit pruning policy decide whether to keep or evict
> the data instead.
>
> This does not add a new cache or increase the cache limit. The object
> data is already accounted in base_cache_used before prune_base_data()
> runs, and the existing pruning and base cleanup paths still release it.
That explanation makes sense, but I'm left with one question/concern.
Dropping the data for a base makes sense when we are "done" with it,
because we know we won't need it anymore and it leaves more room in the
cache for things we do care about.
The problem here is that the current notion of "done" is not correct.
Imagine we have delta chains "A -> B -> C" and "A -> D -> F". We are
totally done with A when we have resolved both B and D, but if I
understand correctly, we currently throw it away after just resolving B.
Your patch never throws it away, and just waits for it to get evicted
from the cache due to memory pressure. But could we realize the moment
when B and D have both finished using it, and evict it then? That makes
it more likely for us to keep something useful in the cache when there
is pressure.
I'm not sure how hard that would be in practice, or how much it would
help (the base cache works in list order, so I think it might naturally
be a sort of LRU?).
-Peff |
|
User |
|
There was a status update in the "New Topics" section about the branch "git index-pack" has been optimized by retaining child bases in the delta cache instead of immediately freeing them, letting the existing cache limit policy decide eviction. Comments? cf. <4882be43-9bc5-48cf-b74c-4a05453b2fef@gmail.com> cf. <20260602064519.GD695568@coredump.intra.peff.net> source: <pull.2131.v2.git.1780330402264.gitgitgadget@gmail.com> |
Speed up the local
index-packphase used by clone/fetch for largedelta-compressed packs.
When
index-packreconstructs a child base and queues it for resolvingdescendant deltas, it currently frees that data immediately. This can force
the same base to be reconstructed again. This patch keeps the data in the
existing delta base cache and lets
delta_base_cache_limitdecide whether toretain or evict it.
This does not add a new cache or increase the cache limit. The object data is
already accounted in
base_cache_used, andprune_base_data()is alreadycalled at this point.
Changes since v1:
p5302-pack-index.shperf-suite results.Correctness:
t/t5302-pack-index.shpassed.linux-leaksCI exercisest5302-pack-index.shunderSANITIZE=leak.Benchmarks on a quiet Ubuntu 24.04 VM, 16 vCPU, 32 GiB RAM, local SSD:
Standard
p5302-pack-index.shperf-suite results usingGIT_PERF_LARGE_REPO=<repo> ./run HEAD~1 HEAD -- p5302-pack-index.sh:These timings isolate the
index-packphase affected by this patch.End-to-end local full-clone spot checks also showed wins, though these timings
include both server-side
pack-objectsand client-sideindex-packrunningconcurrently over a local
file://transport, so they are not isolatedindex-packtimings.These runs used
git clone --bare --no-local, dropped page cache before eachclone, and pinned
GIT_EXEC_PATH,PATH, and--upload-packso helperprocesses came from the selected build. Trace2 confirmed that
index-packandpack-objectsresolved to the intended baseline or patched binary.CC: Ævar Arnfjörð Bjarmason avarab@gmail.com, Junio C Hamano gitster@pobox.com, Derrick Stolee stolee@gmail.com
cc: Jeff King peff@peff.net