Re: [PATCH v2] send-pack.c: add config push.useBitmaps

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jun 16, 2022, 13:38 UTC
Message-ID: <220616.86fsk4ww69.gmgdl@evledraar.gmail.com>
In-Reply-To: <pull.1263.v2.git.1655350617442.gitgitgadget@gmail.com>

On Thu, Jun 16 2022, Kyle Zhao via GitGitGadget wrote:

Show 22 quoted lines

> From: Kyle Zhao <kylezhao@tencent.com>
>
> This allows you to disable bitmaps for "git push". Default is false.
>
> Reachability bitmaps are designed to speed up the "counting objects"
> phase of generating a pack during a clone or fetch. They are not
> optimized for Git clients sending a small topic branch via "git push".
> In some cases (see [1]), using reachability bitmaps during "git push"
> can cause significant performance regressions.
>
> Add a new "push.useBitmaps" config option to disable reachability
> bitmaps during "git push". This allows reachability bitmaps to still
> be used in other areas, such as "git rev-list --use-bitmap-index".
>
> [1]: https://lore.kernel.org/git/87zhoz8b9o.fsf@evledraar.gmail.com/
> [...]
> +	mk_test testrepo heads/main &&
> +	git checkout main &&
> +	GIT_TRACE2_EVENT="$PWD/default" \
> +	git push testrepo main:test &&
> +	test_subcommand git pack-objects --all-progress-implied --revs --stdout \
> +	--thin --delta-base-offset -q --no-use-bitmap-index <default &&

Nit: We tend to indent these ase we wrap, so e.g.:

	test_subcommand git ... \
		--thin --delta [...]

The rest all looks good as far as the diff goes, if what we want to do is to disable this by default, and this isn't worth a re-roll in itself.

But I still think that completely disabling bitmaps might be premature here, especially per Taylor's comment on v1 (which I understand to mean that they should help some of the time, even with push).

And since this is linking to that old thread I started in 2019 I really think the commit message should be exploring and justifying why this might be slower in all cases, or at least a sufficient number of cases to flip the default.

if it's not are we throwing out a generally useful optimization (by default) due to some edge cases we've found?

E.g. have you tried to follow-up on this comment by Jeff King? https://lore.kernel.org/git/20190523113031.GA17448@sigill.intra.peff.net/

At the time I didn't, because as noted in a follow-up I'd lost my test case by the time I read that, but it seems you haven't, and have a current test case.

If you apply that patch on that old git version at the time (as it probably won't apply cleanly), does it make a difference for your case?

To be clear I'm *not* confident that this isn't the right thing to do as far as the diff is concerned. But I *am* confident that the proposed commit message isn't selling me on the idea.

I think a really good start would be to come up with some benchmarks for a few common cases, e.g. how do bitmaps do if we have a big repo but few refs, how about a lot of refs? Does their stale-ness make a difference or not (per previous discussions) etc. etc.

Or, if you don't have time for any of that I think a good change for now would be to add the flag, but not flip the default, and say "we don't have enough data to say if bitmaps are overall worse for pushes, but here's an opt-opt".

But the change as it stands is effectively saying "bitmaps on push hinder more than they help, and turning them on for push was a mistake".

Maybe that's true, but I don't think we've got any data to support that. Even though I've got one of those anecdotes (and from occasional investigations of slow "git push" I'm pretty sure this would help my use-cases more than it would harm them) the plural of anecdote isn't data.

I've only personally run into this with repos with a large number (>30k at least) number of refs, and where the bitmaps were in some state of staleness (this usually being my working copy, where I rely on "git gc --auto").