Re: [PATCH 2/2 v7] pack-objects: use reachability bitmap index when generating non-stdout pack

From: Kirill Smelkov <kirr@nexedi.com>
Date: Sep 10, 2016, 14:59 UTC
Message-ID: <20160910145925.xbbus7eck5ineika@teco.navytux.spb.ru>
In-Reply-To: <20160818180615.q25p57v35m2xxtww@sigill.intra.peff.net>

On Thu, Aug 18, 2016 at 02:06:15PM -0400, Jeff King wrote:

Show 9 quoted lines

> On Tue, Aug 09, 2016 at 10:32:17PM +0300, Kirill Smelkov wrote:
> 
> > Subject: Re: [PATCH 2/2 v7] pack-objects: use reachability bitmap index when
> >    generating non-stdout pack
> 
> This is v7, but as I understand your numbering, it goes with v5 of patch
> 1/2 that I just reviewed (usually we just increment the version number
> on the whole series and treat it as a unit, even if some patches didn't
> change from version to version).

The reason those patches are having their own numbers is that they are orthogonal to each other and can be applied / rejected independently. Since I though Junio might want to pick them up as separate topics they were versioned separately.

But ok, since now we have them considered both together, their next versions posted will be uniform v8.

Show 10 quoted lines

> > So we can teach pack-objects to use bitmap index for initial object
> > counting phase when generating resultant pack file too:
> > 
> > - if we care it is not activated under git-repack:
> 
> Do you mean "if we take care that it is not..." here?
> 
> (I think you might just be getting tripped up in the English idioms;
> "care" means that we have a preference; "to take care" means that we are
> being careful).

Ok, I've might have been tripped and thanks for the catch up. I've changed to

	"if we take care to not let it be activated under git-repack"

Show 10 quoted lines

> 
> > - if we know bitmap index generation is not enabled for resultant pack:
> > 
> >   Current code has singleton bitmap_git so cannot work simultaneously
> >   with two bitmap indices.
> 
> Minor English fixes:
> 
>   The current code has a singleton bitmap_git, so it cannot work
>   simultaneously with two bitmap indices.

ok.

Show 17 quoted lines

> > - if we keep pack reuse enabled still only for "send-to-stdout" case:
> > 
> >   Because on pack reuse raw entries are directly written out to destination
> >   pack by write_reused_pack() bypassing needed for pack index generation
> >   bookkeeping done by regular codepath in write_one() and friends.
> 
> Ditto on English:
> 
>   On pack reuse raw entries are directly written out to the destination
>   pack by write_reused_pack(), bypassing the need for pack index
>   generation bookkeeping done by the regular code path in write_one()
>   and friends.
> 
> I think this is missing the implication. Why wouldn't we want to reuse
> in this case? Certainly we don't when doing a "careful" on-disk repack.
> I suspect the answer is that we cannot write a ".idx" off of the result
> of write_reused_pack(), and write-to-disk always includes the .idx.

Yes, mentioning pack-to-file needs to generate .idx makes it more clear and thanks for pointing this out. I've changed this item to the following (picking some of your English corrections):

    - if we keep pack reuse enabled still only for "send-to-stdout" case:

      Because pack-to-file needs to generate index for destination pack, and
      currently on pack reuse raw entries are directly written out to the
      destination pack by write_reused_pack(), bypassing needed for pack index
      generation bookkeeping done by regular codepath in write_one() and
      friends.

      ( In the future we might teach pack-reuse code about cases when index
        also needs to be generated for resultant pack and remove
        pack-reuse-only-for-stdout limitation )

Hope it is ok.

Show 9 quoted lines

> > More context:
> > 
> >     http://marc.info/?t=146792101400001&r=1&w=2
> 
> Can we turn this into a link to public-inbox? We have just been bit by
> all of our old links to gmane dying, and they cannot easily be replaced
> because they use a gmane-specific article number. public-inbox URLs use
> message-ids, which should be usable for other archives if public-inbox
> goes away.

Yes, makes sense to put msgid here. I've added

	http://public-inbox.org/git/20160707190917.20011-1-kirr@nexedi.com/T/#t

Show 6 quoted lines

> > diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> > index b1007f2..c92d7fc 100644
> > --- a/builtin/pack-objects.c
> > +++ b/builtin/pack-objects.c
> 
> The code here looks fine.

Thanks.

Show 22 quoted lines

> > diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
> > index a278d30..9602e9a 100755
> > --- a/t/t5310-pack-bitmaps.sh
> > +++ b/t/t5310-pack-bitmaps.sh
> > @@ -196,6 +196,18 @@ test_expect_success 'pack-objects respects --local (non-local bitmapped pack)' '
> >  	! has_any packbitmap.objects 3b.objects
> >  '
> >  
> > +test_expect_success 'pack-objects to file can use bitmap' '
> > +	# make sure we still have 1 bitmap index from previous tests
> > +	ls .git/objects/pack/ | grep bitmap >output &&
> > +	test_line_count = 1 output &&
> > +	# verify equivalent packs are generated with/without using bitmap index
> > +	packasha1=$(git pack-objects --no-use-bitmap-index --all packa </dev/null) &&
> > +	packbsha1=$(git pack-objects --use-bitmap-index --all packb </dev/null) &&
> > +	list_packed_objects <packa-$packasha1.idx >packa.objects &&
> > +	list_packed_objects <packb-$packbsha1.idx >packb.objects &&
> > +	test_cmp packa.objects packb.objects
> > +'
> 
> Of course we can't know if bitmaps were actually used, or if they were
> turned off under the hood. But at least this exercises the code a bit.

Yes, I was thinking how to know the bitmap codepath was actually active, and without adding debugging points there is no way (at least I could not find it).

> You could possibly add a perf test which shows off the improvement, but
> I don't think it's strictly necessary.

Good idea. I've added this

---- 8< ----

diff --git a/t/perf/p5310-pack-bitmaps.sh b/t/perf/p5310-pack-bitmaps.sh
index de2a224..bb91dbb 100755
--- a/t/perf/p5310-pack-bitmaps.sh
+++ b/t/perf/p5310-pack-bitmaps.sh
@@ -32,6 +32,14 @@ test_perf 'simulated fetch' '
        } | git pack-objects --revs --stdout >/dev/null
 '
 
+test_perf 'pack to file' '
+       git pack-objects --all pack1 </dev/null >/dev/null
+'
+
+test_perf 'pack to file (bitmap)' '
+       git pack-objects --use-bitmap-index --all pack1b </dev/null >/dev/null
+'
+
 test_expect_success 'create partial bitmap state' '
        # pick a commit to represent the repo tip in the past
        cutoff=$(git rev-list HEAD~100 -1) &&
@@ -53,8 +61,12 @@ test_expect_success 'create partial bitmap state' '
        git update-ref HEAD $orig_tip
 '
 
-test_perf 'partial bitmap' '
+test_perf 'clone (partial bitmap)' '
        git pack-objects --stdout --all </dev/null >/dev/null
 '
 
+test_perf 'pack to file (partial bitmap)' '
+       git pack-objects --use-bitmap-index --all pack2b </dev/null >/dev/null
+'
+
 test_done
---- 8< ----

    Test                                    56dfeb62          this tree
    --------------------------------------------------------------------------------
    5310.2: repack to disk                  8.98(8.05+0.29)   9.05(8.08+0.33) +0.8%
    5310.3: simulated clone                 2.02(2.27+0.09)   2.01(2.25+0.08) -0.5%
    5310.4: simulated fetch                 0.81(1.07+0.02)   0.81(1.05+0.04) +0.0%
    5310.5: pack to file                    7.58(7.04+0.28)   7.60(7.04+0.30) +0.3%
    5310.6: pack to file (bitmap)           7.55(7.02+0.28)   3.25(2.82+0.18) -57.0%
    5310.8: clone (partial bitmap)          1.83(2.26+0.12)   1.82(2.22+0.14) -0.5%
    5310.9: pack to file (partial bitmap)   6.86(6.58+0.30)   2.87(2.74+0.20) -58.2%
    

Kirill