reftable: fix quadratic behavior when re-creating deleted refs#2166
Open
spkrka wants to merge 2 commits into
Open
reftable: fix quadratic behavior when re-creating deleted refs#2166spkrka wants to merge 2 commits into
spkrka wants to merge 2 commits into
Conversation
Author
|
/cc peff@peff.net, ps@pks.im |
spkrka
changed the title
spkrka
changed the title
spkrka
force-pushed
the
reftable-tombstone-perf
branch
5 times, most recently
from
23410d3 to
aa0e191
Compare
Add a performance test and a correctness test for update-ref when many tombstones are present in a reftable. The performance test (p1401) exercises two scenarios: - All refs are deleted (creating tombstones) and then re-created with the same names, which currently exhibits quadratic behavior. - An asymmetric variant where refs are deleted and then new, differently-named refs are created. When the tombstones sort after the new refs, every create scans all tombstones, making this case even worse than re-creating the same refs. The correctness test (t0610) verifies that refs deleted and then re-created with the same names are visible afterwards. Helped-by: Jeff King <peff@peff.net> Signed-off-by: Kristofer Karlsson <krka@spotify.com>
When many refs are deleted and then re-created, update-ref exhibits quadratic behavior. With 8000 refs deleted and re-created, the runtime is ~15s, quadrupling for each doubling of input size. The root cause is the merged iterator's suppress_deletions flag. When set, merged_iter_next_void() silently consumes tombstone records in a tight internal loop before returning to the caller. This prevents higher-level code from checking iteration bounds (such as prefix or refname comparisons) until after all tombstones have been scanned. This affects two code paths during ref creation: - refs_verify_refnames_available() seeks to "refs/tags/foo-1/" to check for D/F conflicts and must scan through all subsequent tombstones before the caller can see that they are past the prefix of interest. - reftable_backend_read_ref() seeks to a specific refname and must scan through all subsequent tombstones before returning "not found", because the merged iterator skips the matching tombstone and searches for the next live record. Fix this by removing suppress_deletions from the merged iterator and instead handling deletion records at each call site in the reftable backend, where prefix and refname bounds are available. Tombstones are now returned to callers, which skip them after their existing bounds checks. This allows iteration to terminate as soon as a tombstone past the relevant bound is encountered. This also requires adding deletion checks to the log iteration paths, since suppress_deletions applied to both ref and log iterators. Both tests in p1401 go from ~14s to ~0.2s with this change. Reported-by: Jeff King <peff@peff.net> Signed-off-by: Kristofer Karlsson <krka@spotify.com>
spkrka
force-pushed
the
reftable-tombstone-perf
branch
from
aa0e191 to
1459371
Compare
Author
|
/submit |
|
Submitted as pull.2166.git.1783344957.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
This patch series was integrated into seen via git@f09e87b. |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| @@ -0,0 +1,44 @@ | |||
| #!/bin/sh | |||
There was a problem hiding this comment.
Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):
On Mon, Jul 06, 2026 at 01:35:55PM +0000, Kristofer Karlsson via GitGitGadget wrote:
> diff --git a/t/perf/p1401-ref-store-tombstones.sh b/t/perf/p1401-ref-store-tombstones.sh
> new file mode 100755
> index 0000000000..e40a6dcbf4
> --- /dev/null
> +++ b/t/perf/p1401-ref-store-tombstones.sh
> @@ -0,0 +1,44 @@
> +#!/bin/sh
> +
> +test_description="Tests performance of ref operations with many tombstones"
> +
> +. ./perf-lib.sh
> +
> +test_expect_success "setup" '
> + git init --ref-format=reftable repo &&
> + blob=$(echo foo | git -C repo hash-object -w --stdin) &&
> + for i in $(test_seq 8000)
> + do
> + printf "create refs/tags/tag-%d %s\n" "$i" "$blob" ||
> + return 1
> + done >repo/input &&
> + git -C repo update-ref --stdin <repo/input &&
> + git -C repo for-each-ref --format="delete %(refname)" |
> + git -C repo update-ref --stdin
> +'
> +
> +test_perf "recreate refs after mass delete" '
> + git -C repo update-ref --stdin <repo/input &&
> + git -C repo for-each-ref --format="delete %(refname)" |
> + git -C repo update-ref --stdin
> +'
You're not only benchmarking the reference recreation, but also their
deletion. If I'm not misreading things, then you can queue cleanups via
`test_when_finished`, and these calls will not be measured.
> +test_expect_success "setup asymmetric" '
> + for i in $(test_seq 8000)
> + do
> + printf "create refs/tags/old-%d %s\n" "$i" "$blob" ||
> + return 1
> + done >repo/input-old &&
> + sed "s/old-/new-/" <repo/input-old >repo/input-new &&
> + git -C repo update-ref --stdin <repo/input-old &&
> + git -C repo for-each-ref --format="delete %(refname)" |
> + git -C repo update-ref --stdin
> +'
Would it make sense to use separate repositories? Otherwise, state from
the preceding benchmark(s) will impact subsequent ones.
> diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh
> index e19e036898..4b7cfe38e4 100755
> --- a/t/t0610-reftable-basics.sh
> +++ b/t/t0610-reftable-basics.sh
> @@ -1163,4 +1163,26 @@ test_expect_success 'writes do not persist peeled value for invalid tags' '
> )
> '
>
> +test_expect_success 'delete and re-create refs with tombstones' '
> + test_when_finished "rm -rf repo" &&
> + git init repo &&
> + test_commit -C repo A &&
> + A=$(git -C repo rev-parse HEAD) &&
> + cat >input <<-EOF &&
> + create refs/tags/a $A
> + create refs/tags/b $A
> + create refs/tags/c $A
> + EOF
> + git -C repo update-ref --stdin <input &&
> +
> + # delete all tags, leaving tombstones
> + git -C repo for-each-ref --format="delete %(refname)" refs/tags/ |
> + git -C repo update-ref --stdin &&
> +
> + # re-create the same refs and verify they are visible
> + git -C repo update-ref --stdin <input &&
> + git -C repo tag -l >actual &&
> + test_line_count = 3 actual
> +'
I wonder whether this test really adds any value. We probably have lots
of tests already that test creation/deletion of references.
PatrickThere was a problem hiding this comment.
Kristofer Karlsson wrote on the Git mailing list (how to reply to this email):
On Tue, 7 Jul 2026 at 17:24, Patrick Steinhardt <ps@pks.im> wrote:
>
> On Mon, Jul 06, 2026 at 01:35:55PM +0000, Kristofer Karlsson via GitGitGadget wrote:
> > diff --git a/t/perf/p1401-ref-store-tombstones.sh b/t/perf/p1401-ref-store-tombstones.sh
> > new file mode 100755
> > index 0000000000..e40a6dcbf4
> > --- /dev/null
> > +++ b/t/perf/p1401-ref-store-tombstones.sh
> > @@ -0,0 +1,44 @@
> > +#!/bin/sh
> > +
> > +test_description="Tests performance of ref operations with many tombstones"
> > +
> > +. ./perf-lib.sh
> > +
> > +test_expect_success "setup" '
> > + git init --ref-format=reftable repo &&
> > + blob=$(echo foo | git -C repo hash-object -w --stdin) &&
> > + for i in $(test_seq 8000)
> > + do
> > + printf "create refs/tags/tag-%d %s\n" "$i" "$blob" ||
> > + return 1
> > + done >repo/input &&
> > + git -C repo update-ref --stdin <repo/input &&
> > + git -C repo for-each-ref --format="delete %(refname)" |
> > + git -C repo update-ref --stdin
> > +'
> > +
> > +test_perf "recreate refs after mass delete" '
> > + git -C repo update-ref --stdin <repo/input &&
> > + git -C repo for-each-ref --format="delete %(refname)" |
> > + git -C repo update-ref --stdin
> > +'
>
> You're not only benchmarking the reference recreation, but also their
> deletion. If I'm not misreading things, then you can queue cleanups via
> `test_when_finished`, and these calls will not be measured.
I don't think measuring the full create+delete cycle is wrong per se,
but you are right that if we can benchmark something more isolated
is even more useful. I will try to split this up better.
> > +test_expect_success "setup asymmetric" '
> > + for i in $(test_seq 8000)
> > + do
> > + printf "create refs/tags/old-%d %s\n" "$i" "$blob" ||
> > + return 1
> > + done >repo/input-old &&
> > + sed "s/old-/new-/" <repo/input-old >repo/input-new &&
> > + git -C repo update-ref --stdin <repo/input-old &&
> > + git -C repo for-each-ref --format="delete %(refname)" |
> > + git -C repo update-ref --stdin
> > +'
>
> Would it make sense to use separate repositories? Otherwise, state from
> the preceding benchmark(s) will impact subsequent ones.
Agreed, I can use a fresh repo for each scenario.
>
> > diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh
> > +test_expect_success 'delete and re-create refs with tombstones' '
>
> I wonder whether this test really adds any value. We probably have lots
> of tests already that test creation/deletion of references.
I could not find an existing test that covers the delete-then-recreate
flow (where tombstones are present when the new refs are created).
The existing tests cover creation and deletion separately but not the
interaction with tombstones.
(But perhaps such a test exists and I just can't find it.)
Thanks,
KristoferThere was a problem hiding this comment.
Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):
On Tue, Jul 07, 2026 at 06:12:31PM +0200, Kristofer Karlsson wrote:
> On Tue, 7 Jul 2026 at 17:24, Patrick Steinhardt <ps@pks.im> wrote:
> > On Mon, Jul 06, 2026 at 01:35:55PM +0000, Kristofer Karlsson via GitGitGadget wrote:
> > > diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh
> > > +test_expect_success 'delete and re-create refs with tombstones' '
> >
> > I wonder whether this test really adds any value. We probably have lots
> > of tests already that test creation/deletion of references.
>
> I could not find an existing test that covers the delete-then-recreate
> flow (where tombstones are present when the new refs are created).
> The existing tests cover creation and deletion separately but not the
> interaction with tombstones.
> (But perhaps such a test exists and I just can't find it.)
In t1400 we definitely have some tests where we exercise this
implicitly. In any case, if we want to retain this test I'd rather add
it to t1400 itself, as the functionality that we're testing is itself
not specific to the backend.
PatrickThere was a problem hiding this comment.
Kristofer Karlsson wrote on the Git mailing list (how to reply to this email):
On Wed, 8 Jul 2026 at 08:00, Patrick Steinhardt <ps@pks.im> wrote:
>
> >
> > I could not find an existing test that covers the delete-then-recreate
> > flow (where tombstones are present when the new refs are created).
> > The existing tests cover creation and deletion separately but not the
> > interaction with tombstones.
> > (But perhaps such a test exists and I just can't find it.)
>
> In t1400 we definitely have some tests where we exercise this
> implicitly. In any case, if we want to retain this test I'd rather add
> it to t1400 itself, as the functionality that we're testing is itself
> not specific to the backend.
Thanks, you are right about the placement -- the contract is valid
regardless of backend.
You are also right about it already being tested this is implicitly
tested between multiple test runs since they have shared state
(the repo). Multiple tests delete the ref as clean up and
multiple tests also create a ref and verifies it.
So it is technically covered but it depends on multiple tests
being executed. I think this is simply exposing my personal
preference to have more self-contained and explicit tests,
but I am happy to drop the added tests -- it is perhaps more
important to avoid bloating the test code.
I will drop the added correctness test for the next iteration.
Thanks,
Kristofer
|
User |
| @@ -86,7 +86,8 @@ static int reftable_backend_read_ref(struct reftable_backend *be, | |||
| if (ret) | |||
There was a problem hiding this comment.
Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):
On Mon, Jul 06, 2026 at 01:35:56PM +0000, Kristofer Karlsson via GitGitGadget wrote:
> From: Kristofer Karlsson <krka@spotify.com>
>
> When many refs are deleted and then re-created, update-ref exhibits
> quadratic behavior. With 8000 refs deleted and re-created, the
> runtime is ~15s, quadrupling for each doubling of input size.
>
> The root cause is the merged iterator's suppress_deletions flag.
> When set, merged_iter_next_void() silently consumes tombstone records
> in a tight internal loop before returning to the caller. This
> prevents higher-level code from checking iteration bounds (such as
> prefix or refname comparisons) until after all tombstones have been
> scanned.
>
> This affects two code paths during ref creation:
>
> - refs_verify_refnames_available() seeks to "refs/tags/foo-1/" to
> check for D/F conflicts and must scan through all subsequent
> tombstones before the caller can see that they are past the prefix
> of interest.
>
> - reftable_backend_read_ref() seeks to a specific refname and must
> scan through all subsequent tombstones before returning "not
> found", because the merged iterator skips the matching tombstone
> and searches for the next live record.
It probably not only impacts reference creation, but also every reader
that wants to search for a specific reference that doesn't exist.
> Fix this by removing suppress_deletions from the merged iterator and
> instead handling deletion records at each call site in the reftable
> backend, where prefix and refname bounds are available. Tombstones
> are now returned to callers, which skip them after their existing
> bounds checks. This allows iteration to terminate as soon as a
> tombstone past the relevant bound is encountered.
This option is still used by downstream users of the reftable library,
like libgit2. So we shouldn't just delete it outright.
> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
> index 4ae22922de..8c4f119ff1 100644
> --- a/refs/reftable-backend.c
> +++ b/refs/reftable-backend.c
> @@ -633,6 +633,9 @@ static int reftable_ref_iterator_advance(struct ref_iterator *ref_iterator)
> break;
> }
>
> + if (iter->ref.value_type == REFTABLE_REF_DELETION)
> + continue;
> +
> if (iter->exclude_patterns && should_exclude_current_ref(iter))
> continue;
>
Okay. I was first wondering whether we should move this call earlier.
But we actually don't want to, as this is the code that precedes the
above:
if (iter->prefix_len &&
strncmp(iter->prefix, iter->ref.refname, iter->prefix_len)) {
iter->err = 1;
break;
}
So this allows us to not only skip the current iteration, but completely
abort iteration by observing tombstones that sort after our prefix.
In any case, as far as I can see all sites where we iterate through
either ref or log records have been adapted to handle deletions.
Thanks!
PatrickThere was a problem hiding this comment.
Kristofer Karlsson wrote on the Git mailing list (how to reply to this email):
On Tue, 7 Jul 2026 at 17:24, Patrick Steinhardt <ps@pks.im> wrote:
>
> > This affects two code paths during ref creation:
> >
> > - refs_verify_refnames_available() seeks to "refs/tags/foo-1/" to
> > check for D/F conflicts and must scan through all subsequent
> > tombstones before the caller can see that they are past the prefix
> > of interest.
> >
> > - reftable_backend_read_ref() seeks to a specific refname and must
> > scan through all subsequent tombstones before returning "not
> > found", because the merged iterator skips the matching tombstone
> > and searches for the next live record.
>
> It probably not only impacts reference creation, but also every reader
> that wants to search for a specific reference that doesn't exist.
Hm good point, I will try to rephrase this better.
> > Fix this by removing suppress_deletions from the merged iterator and
> > instead handling deletion records at each call site in the reftable
> > backend, where prefix and refname bounds are available. Tombstones
> > are now returned to callers, which skip them after their existing
> > bounds checks. This allows iteration to terminate as soon as a
> > tombstone past the relevant bound is encountered.
>
> This option is still used by downstream users of the reftable library,
> like libgit2. So we shouldn't just delete it outright.
Good catch! I can keep suppress_deletions as-is and just
stop setting it from stack.c. That way libgit2 is unchanged, while
we still optimize it at the other call sites. The reftable library
diff then shrinks to a single removed line.
> > diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
> > index 4ae22922de..8c4f119ff1 100644
> > --- a/refs/reftable-backend.c
> > +++ b/refs/reftable-backend.c
> > @@ -633,6 +633,9 @@ static int reftable_ref_iterator_advance(struct ref_iterator *ref_iterator)
> > break;
> > }
> >
> > + if (iter->ref.value_type == REFTABLE_REF_DELETION)
> > + continue;
> > +
> > if (iter->exclude_patterns && should_exclude_current_ref(iter))
> > continue;
> >
>
> Okay. I was first wondering whether we should move this call earlier.
> But we actually don't want to, as this is the code that precedes the
> above:
>
> if (iter->prefix_len &&
> strncmp(iter->prefix, iter->ref.refname, iter->prefix_len)) {
> iter->err = 1;
> break;
> }
>
> So this allows us to not only skip the current iteration, but completely
> abort iteration by observing tombstones that sort after our prefix.
Indeed, this is the primary win.
> In any case, as far as I can see all sites where we iterate through
> either ref or log records have been adapted to handle deletions.
Thanks! Appreciate the review (and spotting the libgit breakage!)
Kristofer
|
User |
|
"brian m. carlson" wrote on the Git mailing list (how to reply to this email): On 2026-07-06 at 13:35:54, Kristofer Karlsson via GitGitGadget wrote:
> This series fixes quadratic behavior in update-ref when many refs are
> deleted (tombstoned) and then new refs are created with the reftable
> backend.
[…]
> The first patch adds tests for tombstone scenarios: a perf test (p1401)
> exercising two patterns with 8000 refs, and a correctness test (t0610)
> verifying that deleted-then-recreated refs are visible.
>
> The second patch is the pure optimization. Both p1401 tests go from ~14s to
> ~0.2s with the fix.
>
> Note that auto-compaction typically merges tombstones before they accumulate
> to this degree, so the quadratic behavior may not show up in every workflow.
> But the fix ensures correct time complexity regardless of compaction state,
> and the change is fairly contained.
I had hit this before when doing some benchmarks for using reftable at
$DAYJOB. We had discussed it on the list and decided that it was
synthetic at the time, but I'm glad to see that this is being fixed now.
I don't have comments on the patches themselves because I haven't spent
enough time in the reftable code to be familiar with it, but I do
definitely appreciate the performance improvement.
--
brian m. carlson (they/them)
Toronto, Ontario, CA |
|
User |
|
This branch is now known as |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This series fixes quadratic behavior in update-ref when many refs are
deleted (tombstoned) and then new refs are created with the reftable
backend.
The root cause is the merged iterator's suppress_deletions flag, which
silently consumes tombstone records in a tight internal loop. This
prevents higher-level code from checking iteration bounds until after
all tombstones have been scanned, making both refs_verify_refnames_available()
and reftable_backend_read_ref() O(n) per call in the presence of
tombstones.
The fix removes suppress_deletions from the merged iterator and instead
handles deletion records at each call site in the reftable backend,
where prefix and refname bounds are available. This lets existing bounds
checks terminate iteration early when encountering tombstones past the
relevant bound.
The first patch adds tests for tombstone scenarios: a perf test (p1401)
exercising two patterns with 8000 refs, and a correctness test (t0610)
verifying that deleted-then-recreated refs are visible.
The second patch is the pure optimization. Both p1401 tests go from
~14s to ~0.2s with the fix.
Note that auto-compaction typically merges tombstones before they
accumulate to this degree, so the quadratic behavior may not show up
in every workflow. But the fix ensures correct time complexity
regardless of compaction state, and the change is fairly contained.
Previous discussion: https://lore.kernel.org/git/20260701080014.GA3748390@coredump.intra.peff.net/
cc: Patrick Steinhardt ps@pks.im
cc: Kristofer Karlsson krka@spotify.com
cc: "brian m. carlson" sandals@crustytoothpaste.net