From: Kristofer Karlsson <krka@spotify.com>
Date: Fri, 07 Aug 2026 12:34:18 GMT
Subject: Re: [PATCH v7 05/10] commit-reach: add trace2 instrumentation to paint_down_to_common()
Message-ID: <CAL71e4Opn3u6qYG9xhhkB1qqYj9ZLk6_=fxznyFzSFbrh2BMTw@mail.gmail.com>
In-Reply-To: <CABPp-BHLHGQxuG3gO+nCa-FPFyOFEU2rk_oxLtFjekLqENvQUw@mail.gmail.com>

On Fri, 7 Aug 2026 at 05:03, Elijah Newren <newren@gmail.com> wrote:
>
> > Add a step counter and trace2_data_intmax() call so that the number
> > of commits visited during the paint walk is observable via
> > GIT_TRACE2_EVENT. This provides a way to measure the impact of
> > future optimizations without relying on wall-clock benchmarks alone.
>
> Ooh, I like it.

I will need to credit Stolee for this idea to count steps instead
of measuring wall clock -- but I agree, it comes in very handy
here.

> > -       test_all_modes in_merge_bases_many
> > +       test_all_modes in_merge_bases_many &&
> > +       test_paint_down_steps 45 2 25 3
> >  '
>
> Whoa, what?  <Digs around for a while.>  So, this is really confusing
> at first to a reviewer; it makes me think you are testing that you've
> already written the optimization and that some forms of commit-graphs
> provide a speedup from your work that doesn't land until later in the
> series.  It might help if you point out either in the commit message
> or a comment here that this code is just relying on pre-existing
> optimization where a min_generation is passed and --all is not passed.
> (In contrast to below where --all is passed, so it has to dig deeper
> with or without the commit graph).

Yeah, the numbers are a bit hard to understand here -- I could
add a comment saying that the min_generation floor optimization
kicks in here and this is how it behaves for:
no graph, full v2 graph, partial v2 graph, v1 graph
(in that order)

So it's not about the new optimization, it's adding these
counters to existing graph tests.

I am not sure what the best approach is here:
skip these step-asserts for graphs that already use some other
optimization (min_generation floor), add a test comment,
or leave it as it is (confusing for reviewing now, but perhaps
not as confusing long term?)

Thanks,
Kristofer