reftable: fix quadratic behavior when re-creating deleted refs#2166
Draft
spkrka wants to merge 2 commits into
Draft
reftable: fix quadratic behavior when re-creating deleted refs#2166spkrka wants to merge 2 commits into
spkrka wants to merge 2 commits into
Conversation
Add a performance test for update-ref when many tombstones are present. This exercises the scenario where all refs are deleted (creating tombstones in the reftable) and then re-created, which currently exhibits quadratic behavior. 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. Before: nr=1000 0.306s nr=2000 0.945s nr=4000 3.816s nr=8000 14.93s After: nr=1000 0.020s nr=2000 0.044s nr=4000 0.071s nr=8000 0.145s nr=16000 0.258s nr=32000 0.591s Signed-off-by: Kristofer Karlsson <krka@spotify.com>
Author
|
/cc peff@peff.net, ps@pks.im |
spkrka
changed the title
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 re-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.
Performance with Peff's benchmark (delete all refs, then re-create):
We also reproduced this with real ref names from a large monorepo
(~8500 remote-tracking branches). With compaction blocked to keep
tombstones unmerged, re-creating the refs took 19.1s on master vs
0.29s with this fix. In practice though, auto-compaction usually
merges tombstones away before they accumulate to this degree, so the
quadratic behavior rarely shows up in normal use. This is arguably
more of a theoretical cleanup than a fix for a pressing real-world
problem -- but having the correct time complexity makes us less
reliant on compaction to hide the issue, and the change is fairly
contained.
Previous discussion: https://lore.kernel.org/git/20260701080014.GA3748390@coredump.intra.peff.net/
Patches: