connected: add incremental connectivity check - #2211
Open
spkrka wants to merge 1 commit into
Open
Conversation
spkrka
marked this pull request as ready for review
August 28, 2026 10:15
spkrka
force-pushed
the
tree-diff-connectivity-v1-clean
branch
10 times, most recently
from
September 1, 2026 18:03
326b1e4 to
452d6b9
Compare
spkrka
force-pushed
the
tree-diff-connectivity-v1-clean
branch
8 times, most recently
from
September 11, 2026 12:03
c029b3a to
0d19d97
Compare
The connectivity check uses rev-list to find commits reachable
from the incoming tips but not from any local ref and then walks
their object closure. Commit traversal stops at the connectivity
boundary, but trees and blobs reachable from that boundary still
need to be walked so they can be marked uninteresting, allocating
a struct object for each one. On repositories where the boundary
commits have large trees, the connectivity check for small
incoming changes visits and tracks more objects than needed.
Add an alternative connectivity check that verifies incoming
commits incrementally against their parents.
Instead of traversing the full boundary closure, the new check
compares each new commit's tree with its parent trees. Already
trusted entries are skipped, changed subtrees are descended into
recursively, and blobs are checked for existence. This approach
thus avoids descending into untouched subtrees.
For example, consider a commit that changes one file under lib/
and also moves an unchanged subtree from src/ to dev/:
Parent tree New tree
+-- src/ (aaa) +-- dev/ (aaa)
+-- lib/ (bbb) +-- lib/ (ccc)
+-- foo.c (ddd) +-- foo.c (ddd)
+-- bar.c (eee) +-- bar.c (fff)
The verifier first scans the new root and collects aaa and ccc as
work items. It then scans the parent root, publishing aaa and bbb
into the trusted sets and recording bbb as the comparison base for
ccc.
When the work list is revisited, aaa is now trusted and skipped
even though it appears at a different path. The verifier descends
into ccc using bbb as its parent base. Scanning bbb similarly
makes ddd and eee trusted, leaving only the new fff blob to be
checked for existence.
Thus neither the moved subtree nor any other unchanged subtree is
recursively explored; only the changed lib/ subtree is descended
into, and only the new bar.c blob needs an existence check. The
root trees still need to be read and scanned as comparison bases.
New commits are processed with ancestors before descendants.
Parents outside the incoming commit set are on the already-connected
side of the boundary and provide the initial trusted bases. Once
an incoming commit has been verified, its tree can in turn be used
as a trusted base for descendant commits.
The verifier distinguishes trusted trees from expanded trees. A
trusted tree can be accepted without further verification. An
expanded tree has additionally published its direct non-gitlink
entries into the trusted sets. Expanded parent trees therefore
need not be read again for blob-only work, but may still be reread
when recursive verification needs same-path parent subtrees.
The implementation adds a --verify-trees-incremental flag to
rev-list, following the same pattern as --exclude-promisor-objects:
a pre-setup_revisions() scan sets the flag, the
post-setup_revisions() option loop skips it, and the main
traversal short-circuits into the incremental verifier after
collecting commits from the revision walk.
Because the incremental block eagerly consumes all commits via
get_revision(), the subsequent mark_edges_uninteresting() and
traverse_commit_list_filtered() calls naturally find no commit
work to do and only process any non-commit tips (trees, blobs)
left in revs.pending.
For partial clones, missing promisor objects (trees and blobs
promised by a promisor remote) are silently accepted during
verification instead of triggering errors. The revision walk
receives --exclude-promisor-objects so promisor commits do not
enter the verification set.
Gate the new algorithm behind transfer.connectivityCheck=incremental,
keeping full as the default. Fall back to the full algorithm for
deepening fetches to keep this commit easy to reason about, though
incremental mode could potentially handle them as well.
p5412 results (median of 3), scaling one dimension at a time.
Each modified file is in a different directory, with
directories chosen round-robin.
Scaling tree size (10 commits, 10 files/commit):
files full incr. full/incr
5K 0.01s 0.01s 1.1x
50K 0.04s 0.01s 2.2x
200K 0.14s 0.02s 5.7x
800K 0.60s 0.04s 14.8x
The full mode must traverse the boundary tree closure, which
grows with overall tree size. Incremental still scans the
root trees, but avoids descending into unchanged subtrees,
so it grows much more slowly with repository size.
Scaling commit count (200K files, 10 files/commit):
commits full incr. full/incr
1 0.14s 0.01s 7.5x
10 0.15s 0.02s 6.8x
100 0.16s 0.07s 2.2x
500 0.29s 0.29s 0.9x
3000 1.26s 1.67s 0.7x
5000 1.95s 2.71s 0.7x
10000 3.74s 6.02s 0.6x
With many commits the per-commit overhead of scanning both
the new and parent root trees accumulates and incremental
becomes slower. Breakeven is around 500 commits and the
ratio stabilizes near 0.6x for this fixture.
Scaling files per commit (200K files, 10 commits):
files/commit full incr. full/incr
1 0.15s 0.02s 7.5x
10 0.15s 0.02s 6.8x
100 0.15s 0.04s 3.1x
500 0.26s 0.16s 1.6x
1000 0.36s 0.35s 1.0x
2000 0.66s 0.79s 0.8x
Breakeven is around 1000 files/commit. At 2000 files/commit
(every directory touched), incremental is about 1.2x slower.
For small repositories both modes are fast enough that the
difference is difficult to measure reliably.
Signed-off-by: Kristofer Karlsson <krka@spotify.com>
spkrka
force-pushed
the
tree-diff-connectivity-v1-clean
branch
from
September 11, 2026 12:35
0d19d97 to
3f4473e
Compare
Author
|
/cc Patrick Steinhardt ps@pks.im |
Author
|
/cc Jeff King <peff@peff.net |
|
User |
Author
|
/cc Elijah Newren newren@gmail.com |
|
User |
|
User |
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 is a single-commit patch adding an incremental connectivity
check to check_connected(), gated behind
transfer.connectivityCheck=incremental (no expected changes unless
you opt in).
The intent here is to solve the problem of the connectivity
check slowing down as the number of reachable objects from
the boundary grows.
It relates to the RFC I sent out earlier:
Design
The verifier runs inside the same rev-list subprocess that
check_connected() already spawns, triggered by a new internal
flag --verify-trees-incremental. After get_revision() collects
the incoming commits, the verifier processes them in topological
order (ancestors before descendants).
The idea is to keep a set of trusted objects, shared across the
incoming commits, that grows over time. We visit the new/untrusted
commit trees and do a comparison walk over the trees of their
parents. Everything we visit from the parent side gets added
to the trusted set, and that helps us shortcut the object graph
traversal.
More algorithmic details are in the commit message.
Benchmarks
I'll just mention a short summary here, to avoid repeating
what's already in the commit message. The incremental mode is
faster than the full mode when there are few commits to verify
and when the active object tree is large. In the happy case,
the work tracks the changed paths and their comparison trees
rather than the full reachable object closure, which
substantially reduces the dependence on total repository size.
I've seen speedups up to around 20x for the synthetic perf
tests.
Running against a large real-world repo (skipping the commit
boundary search), I see a speedup of 67x for checking a single
commit and 6x for checking 100 commits.
There are also regression cases. With long incoming histories,
the extra parent-tree scans accumulate; in the synthetic fixture
incremental is about 1.7x slower at 10000 commits, and the
simple tree-scanning model approaches a 2x slowdown when
root-tree scanning dominates. Large per-commit changes can
similarly reduce the benefit.
I cannot prove that these cases are rare, though I expect they
are. It is at least encouraging that the regression appears
bounded in the cases I have tested; I have not proved such a
bound, but have not been able to provoke a larger regression.
My initial feeling is to accept that tradeoff behind the opt-in
config, though we could also add a simple heuristic based on
properties of the incoming changes and/or the repository.
Test coverage
Most correctness cases in t5412-connectivity-check.sh are run
in both full and incremental modes to check semantic equivalence.
Selected cases additionally assert trace2 tree/blob counts for
the incremental mode, to verify that unchanged portions of the
object graph are actually skipped. It covers:
mismatches, malformed trees (unparseable, mid-tree
corruption)
subtrees are skipped, subtree moves, merge parent boundaries
trees, verification of local commits
Unit-style tests call git rev-list directly with the
appropriate flags; integration tests exercise the full
check_connected() path through push, fetch, and clone.
Alternatives considered
My first prototype ran the verifier in-process inside
connected.c. This required a second rev-list subprocess just
for boundary finding, _nofetch variants of several
object-reading functions to prevent lazy fetches in partial
clones, explicit shallow-file plumbing, and careful avoidance
of die() in all code paths reachable from the verifier. The
result worked but was fragile and touched many files.
Moving the verifier into the rev-list subprocess eliminated
all of those problems: in partial clones the existing
--exclude-promisor-objects handling already disables lazy
fetching, die() is isolated by the process boundary, shallow
and replacement semantics are established before the verifier
runs, and error routing comes for free via stderr.
So while I liked the idea of being less reliant on checking within
a subprocess, making that work ended up being a lot more complex.
Next steps
This patch only addresses tree verification; the other
significant cost is finding the commit boundary, especially for
repos with many refs. I already have some prototypes for
optimizing that too, and if this ends up landing, that would be
something I would start polishing up.
Thanks,
Kristofer
cc: Patrick Steinhardt ps@pks.im
cc: Jeff King peff@peff.net
cc: Elijah Newren newren@gmail.com