Re: [PATCH] commit: detect commits that exist in commit-graph but not in the ODB

From: Patrick Steinhardt <ps@pks.im>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org, Karthik Nayak <karthik.188@gmail.com>,
	Taylor Blau <me@ttaylorr.com>
Subject: Re: [PATCH] commit: detect commits that exist in commit-graph but not in the ODB
Date: Thu, 19 Oct 2023 10:25:29 +0200	[thread overview]
Message-ID: <ZTDn-Wd5xsFrBmqI@tanuki> (raw)
In-Reply-To: <ZTDQjangLsQ1cSJl@tanuki>

[-- Attachment #1: Type: text/plain, Size: 2654 bytes --]

On Thu, Oct 19, 2023 at 08:45:34AM +0200, Patrick Steinhardt wrote:
> On Tue, Oct 17, 2023 at 11:34:53AM -0700, Junio C Hamano wrote:
> > Patrick Steinhardt <ps@pks.im> writes:
> > 
> > > Fair point indeed. The following is a worst-case scenario benchmark of
> > > of the change where we do a full topological walk of all reachable
> > > commits in the graph, executed in linux.git. We parse commit parents via
> > > `repo_parse_commit_gently()`, so the new code path now basically has to
> > > check for object existence of every reachable commit:
> > > ...
> > > The added check does lead to a performance regression indeed, which is
> > > not all that unexpected. That being said, the commit-graph still results
> > > in a significant speedup compared to the case where we don't have it.
> > 
> > Yeah, I agree that both points are expected.  An extra check that is
> > much cheaper than the full parsing is paying a small price to be a
> > bit more careful than before.  The question is if the price is small
> > enough.  I am still not sure if the extra carefulness is warranted
> > for all normal cases to spend 30% extra cycles.
> > 
> > Thanks.
> 
> Well, seen in contexts like the thread that spawned this discussion I
> think it's preferable to take a relatively smaller price compared to
> disabling the commit graph entirely in some situations. With that in
> mind, personally I'd be happy with either of two outcomes here:
> 
>     - We take the patch I proposed as a hardening measure, which allows
>       us to use the commit graph in basically any situation where we
>       could parse objects from the ODB without any downside except for a
>       performance hit.
> 
>     - We say that corrupt repositories do not need to be accounted for
>       when using metadata caches like the commit-graph. That would mean
>       in consequence that for the `--missing` flag we would not have to
>       disable commit graphs.
> 
> The test failure that was exposed in Karthik's test only stems from the
> fact that we manually corrupt the repository and is not specific to the
> `--missing` flag. This is further stressed by the new test aded in my
> own patch, as we can trigger a similar bug when not using `--missing`.

There's another way to handle this, which is to conditionally enable the
object existence check. This would be less of a performance hit compared
to disabling commit graphs altogether with `--missing`, but still ensure
that repository corruption was detected. Second, it would not regress
performance for all preexisting users of `repo_parse_commit_gently()`.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]