Re: slow object packing during push

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Mon, Mar 15 2021, David Turner wrote:

> On 3/9/21 9:14 PM, Ævar Arnfjörð Bjarmason wrote:
>> On Tue, Mar 09 2021, David Turner wrote:
>>
>>> I have a large, funny repository that has very slow pushes unless I
>>> set pack.usebitmaps=false to false.
>> Good to see you on-list again! :)
>
> Thanks!
>
>>> First, a description of the repo: it's about 175GB, and was created by combining about 40,000 smaller repositories.  Historically, these repos were submodules of one meta repository[2].  I have stitched together the submodules, and this is the repository in which the stitching was done - that is, it contains all of the objects from the smaller repos, plus all of the objects from the meta repository, plus the newly-created trees & commits for the stitched repositories.  As new commits come into the meta repository (which have gitlinks to new submodule commits), we fetch from the meta repository (8s - it would be 2s if we were fetching into a normal clone without all of the other stuff), and the submodules (up to 10s per and embarrassingly parallel). Then we stitch (~0s), and push to the stitched "unity" repository (~2 minutes!!!).  The entire repo fits in RAM (yes, all 175G) and is in fact in the disk cache (I prewarmed the cache before testing anything).  
>>>
>>> The vast majority of the time appears to be spent in git pack-objects, and in particular in the stack trace in [1].  If I set pack.usebitmaps=false, the push only takes 10s.   This seems like pack bitmaps are a severe pessimization for my purposes.  This is true even immediately after a repack (that is, almost all of the objects are in one giant pack, except the newly-fetched ones).  I also tried setting up pack islands - one for each smaller repo, one for the stitched commits, and one for commits from the meta repo.  I'm not sure if this is necessary, but it's definitely not sufficient (my current config has it turned on, because I didn't feel like repacking again after testing it, and I tested it before testing pack.usebimaps). 
>>> [snip]
>> Without having carefully re-read it, I believe this issue is the same as
>> what I reported here in 2019, and I think you'll find the resulting
>> discussion intresting:
>> https://lore.kernel.org/git/87zhoz8b9o.fsf@evledraar.gmail.com/
>>
>> Having skimmed it, I think you're probably omitting that this is a bare
>> repo you're pushing from, and thus you're running into the combination
>> of repack.writeBitmaps being true by default on bare repos, and
>> pack.useBitmaps being true everywhere (but having no effect by default
>> unless you have a bare repo, unless you manually make bitmaps blah
>> blah).
>>
>> One of the semi-conclusions from the above thread was that we mostly
>> turned this on thinking that bare ~= server that accepts pushes, and
>> bitmaps are known to be worse (but not always!) for when you're pushing
>> *from* your repo.
>
> Thanks, that does explain it.  We should probably consider a default
> that bitmaps aren't used on pushes.

I think that would be a good move. Again, I haven't re-read that thread
+ the preceding discussion, but my recollection is that the performance
numbers were solid on the "accepts pushes" case, but the "does pushes"
wasn't much thought about / not seen as that worth dealing with.

So I think between my report and yours it would make sense to just do as
you suggest with a "this is probably a better default for this case than
not" patch.

I was about to submit such a thing, should be easy, right?

The problem is that it would be easy if we had one config variable in
play.

But in this case there's a default that causes the bitmaps to be written
on bare repos, and then it's just picked up as part of the general "do
we have a bitmap? use it!" default.

At that point we don't know how our bitmap came to be created, is it
because of this default that could probably be sensibly flipped for the
"not_set && is_bare && doing-push" case, or was it manually created by
someone's custom gc/repack script after careful testing of bitmaps (also
for pushes)?

So with that and Jeff King's note that it *does* help some of the time
even in that case I'm not sure what the best way forward is.

The best I can come up with now is:

    if (!have_bitmap_write_config && !have_explicit_use_bitmaps_config &&
        is_bare && doing_push)
        advice(blah blah maybe this is bad for you?);

Which sucks, but would have have helped both you/me find the root cause
quickly, so maybe it's not such a bad idea.

What do you think we should do?

There's also digging into the perf problem itself & fixing that of
course...