[PATCH v2 0/2] commit-graph: fix topo_levels slab propagation regression

["Kristofer Karlsson via GitGitGadget" <gitgitgadget@gmail.com>]

When fetch.writeCommitGraph is enabled (or git maintenance runs after
fetch), an incremental commit-graph write computes generation numbers for
the newly added commits. For commits already in the graph, their topo levels
should be read from the existing layers, making the DFS proportional to the
number of new commits.

199d452758 (commit-graph: return the prepared commit graph from
prepare_commit_graph(), 2025-09-04), part of the ps/commit-graph-via-source
series [1], refactored the loop that propagates the topo_levels slab to each
layer of the commit-graph chain. The original code used a single variable
that advanced through the chain:

while (g) {
    g->topo_levels = &topo_levels;
    g = g->base_graph;
}


The refactored code introduced a separate iteration variable but did not
update the loop body to match:

for (struct commit_graph *chain = g; chain; chain = chain->base_graph)
    g->topo_levels = &topo_levels;


This always assigns to the topmost layer instead of the current one. Commits
from lower layers appear to have no generation numbers, so the DFS re-walks
the entire ancestry.

On a repo with a multi-layer split commit-graph, an incremental commit-graph
write triggered by git fetch drops from ~3.5 seconds to ~0.2 seconds after
the fix.

[1]
https://lore.kernel.org/git/aMNTELw0Wk8jWoPc@nand.local/T/#mb55b5f0e1ccf82d969ac1d8144c56ecf87b833e8

Changes since v1:

 * Fixed wrong commit title and date in the reference (Junio, Taylor).
 * use test_expect_failure with the correct assertion instead of a # BUG
   comment (Taylor).
 * Simplified commit messages.

Kristofer Karlsson (2):
  commit-graph: add trace2 instrumentation for generation DFS
  commit-graph: propagate topo_levels slab to all chain layers

 commit-graph.c                |  7 ++++++-
 t/t5324-split-commit-graph.sh | 24 ++++++++++++++++++++++++
 2 files changed, 30 insertions(+), 1 deletion(-)


base-commit: f85a7e662054a7b0d9070e432508831afa214b47
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2170%2Fspkrka%2Fkrka%2Ffix-topo-levels-slab-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2170/spkrka/krka/fix-topo-levels-slab-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/2170

Range-diff vs v1:

 1:  b865c2bcff ! 1:  100efa22a9 commit-graph: add trace2 instrumentation for generation DFS
     @@ Metadata
       ## Commit message ##
          commit-graph: add trace2 instrumentation for generation DFS
      
     -    Add a step counter and trace2_data_intmax call to
     -    compute_reachable_generation_numbers() to make the cost of
     -    the generation number DFS observable.  This exposes a
     -    regression introduced in 199d452758 (commit-graph: fix
     -    "filling in" topological levels, 2025-04-07) where
     -    incremental commit-graph writes re-walk the entire commit
     -    ancestry instead of reading topo levels from lower graph
     -    layers.
     +    Count the number of steps taken in
     +    compute_reachable_generation_numbers() and expose it via
     +    trace2 to make it easier to detect performance regressions.
      
     -    Add a test that demonstrates the problem: with a two-layer
     -    split commit-graph, writing a new incremental layer for a
     -    commit whose parent is in the base layer walks all the way
     -    down to the root (7 steps for 5 base commits) instead of
     -    reading the existing topo level and stopping immediately
     -    (1 step).
     +    Add a failing test for such a regression, introduced in
     +    199d452758 (commit-graph: return the prepared commit graph
     +    from `prepare_commit_graph()`, 2025-09-04), where incremental
     +    commit-graph writes do not see existing generation numbers
     +    from lower graph layers and fall back to walking the full
     +    ancestry.
      
          Signed-off-by: Kristofer Karlsson <krka@spotify.com>
      
     @@ t/t5324-split-commit-graph.sh: test_expect_success 'write generation data chunk
       	)
       '
       
     -+test_expect_success 'incremental write reads topo levels from all layers' '
     ++test_expect_failure 'incremental write reads topo levels from all layers' '
      +	git init topo-from-lower &&
      +	(
      +		cd topo-from-lower &&
     @@ t/t5324-split-commit-graph.sh: test_expect_success 'write generation data chunk
      +		GIT_TRACE2_EVENT="$(pwd)/trace.txt" \
      +			git commit-graph write --reachable --split=no-merge &&
      +
     -+		# BUG: topo levels from lower graph layers are not
     -+		# propagated, so the DFS re-walks from base-3 down to
     -+		# the root (7 steps) instead of reading topo levels
     -+		# from the existing graph (1 step).
     -+		test_trace2_data commit-graph generation-dfs-steps 7 <trace.txt
     ++		test_trace2_data commit-graph generation-dfs-steps 1 <trace.txt
      +	)
      +'
      +
 2:  f9c1482a76 ! 2:  679dd2e392 commit-graph: propagate topo_levels slab to all chain layers
     @@ Metadata
       ## Commit message ##
          commit-graph: propagate topo_levels slab to all chain layers
      
     -    Fix a regression introduced in 199d452758 (commit-graph: fix
     -    "filling in" topological levels, 2025-04-07) where the loop
     -    propagating the topo_levels slab to each layer of the
     -    commit-graph chain always assigned to `g->topo_levels`
     -    (the topmost layer) instead of `chain->topo_levels` (the
     -    current iteration variable).
     +    The topo_levels slab is only propagated to the topmost graph
     +    layer instead of all layers in the chain.  Commits from lower
     +    layers appear to have no generation numbers, so the DFS
     +    re-walks the entire ancestry.
      
     -    This meant only the topmost layer had its topo_levels pointer
     -    set.  When compute_reachable_generation_numbers() ran for an
     -    incremental write, commits parsed from lower layers had their
     -    topo levels left at zero in the slab, since
     -    fill_commit_graph_info() could not store them without the
     -    pointer.  The DFS then re-walked the entire commit ancestry
     -    instead of stopping at commits with known levels.
     -
     -    On a repository with 2.78M commits and a multi-layer split
     -    commit-graph, this caused a single incremental commit-graph
     -    write to spend ~3.7 seconds in the generation DFS instead of
     -    microseconds.
     +    Fix by making topo_levels visible to all layers, not just
     +    the first one.
      
          Signed-off-by: Kristofer Karlsson <krka@spotify.com>
      
     @@ commit-graph.c: int write_commit_graph(struct odb_source *source,
       		ctx.changed_paths = 1;
      
       ## t/t5324-split-commit-graph.sh ##
     -@@ t/t5324-split-commit-graph.sh: test_expect_success 'incremental write reads topo levels from all layers' '
     - 		GIT_TRACE2_EVENT="$(pwd)/trace.txt" \
     - 			git commit-graph write --reachable --split=no-merge &&
     - 
     --		# BUG: topo levels from lower graph layers are not
     --		# propagated, so the DFS re-walks from base-3 down to
     --		# the root (7 steps) instead of reading topo levels
     --		# from the existing graph (1 step).
     --		test_trace2_data commit-graph generation-dfs-steps 7 <trace.txt
     -+		test_trace2_data commit-graph generation-dfs-steps 1 <trace.txt
     +@@ t/t5324-split-commit-graph.sh: test_expect_success 'write generation data chunk when commit-graph chain is repl
       	)
       '
       
     +-test_expect_failure 'incremental write reads topo levels from all layers' '
     ++test_expect_success 'incremental write reads topo levels from all layers' '
     + 	git init topo-from-lower &&
     + 	(
     + 		cd topo-from-lower &&

-- 
gitgitgadget