Re: [PATCH v2 5/6] bitmap-lookup-table: add performance tests for lookup table

[Taylor Blau <me@ttaylorr.com>]

On Sun, Jun 26, 2022 at 01:10:16PM +0000, Abhradeep Chakraborty via GitGitGadget wrote:
> From: Abhradeep Chakraborty <chakrabortyabhradeep79@gmail.com>
>
> Add performance tests to verify the performance of lookup table.
>
> Lookup table makes Git run faster in most of the cases. Below is the
> result of `t/perf/p5310-pack-bitmaps.sh`.`perf/p5326-multi-pack-bitmaps.sh`
> gives similar result. The repository used in the test is linux kernel.
>
> Test                                                      this tree
> --------------------------------------------------------------------------
> 5310.4: repack to disk (lookup=false)                   295.94(250.45+15.24)
> 5310.5: simulated clone                                 12.52(5.07+1.40)
> 5310.6: simulated fetch                                 1.89(2.94+0.24)
> 5310.7: pack to file (bitmap)                           41.39(20.33+7.20)
> 5310.8: rev-list (commits)                              0.98(0.59+0.12)
> 5310.9: rev-list (objects)                              3.40(3.27+0.10)
> 5310.10: rev-list with tag negated via --not		0.07(0.02+0.04)
>          --all (objects)
> 5310.11: rev-list with negative tag (objects)           0.23(0.16+0.06)
> 5310.12: rev-list count with blob:none                  0.26(0.18+0.07)
> 5310.13: rev-list count with blob:limit=1k              6.45(5.94+0.37)
> 5310.14: rev-list count with tree:0                     0.26(0.18+0.07)
> 5310.15: simulated partial clone                        4.99(3.19+0.45)
> 5310.19: repack to disk (lookup=true)                   269.67(174.70+21.33)
> 5310.20: simulated clone                                11.03(5.07+1.11)
> 5310.21: simulated fetch                                0.79(0.79+0.17)
> 5310.22: pack to file (bitmap)                          43.03(20.28+7.43)
> 5310.23: rev-list (commits)                             0.86(0.54+0.09)
> 5310.24: rev-list (objects)                             3.35(3.26+0.07)
> 5310.25: rev-list with tag negated via --not		0.05(0.00+0.03)
> 	 --all (objects)
> 5310.26: rev-list with negative tag (objects)           0.22(0.16+0.05)
> 5310.27: rev-list count with blob:none                  0.22(0.16+0.05)
> 5310.28: rev-list count with blob:limit=1k              6.45(5.87+0.31)
> 5310.29: rev-list count with tree:0                     0.22(0.16+0.05)
> 5310.30: simulated partial clone                        5.17(3.12+0.48)
>
> Test 4-15 are tested without using lookup table. Same tests are
> repeated in 16-30 (using lookup table).
>
> Signed-off-by: Abhradeep Chakraborty <chakrabortyabhradeep79@gmail.com>
> Mentored-by: Taylor Blau <me@ttaylorr.com>
> Co-Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
> ---
>  t/perf/p5310-pack-bitmaps.sh       | 77 ++++++++++++++-----------
>  t/perf/p5326-multi-pack-bitmaps.sh | 93 ++++++++++++++++--------------
>  2 files changed, 94 insertions(+), 76 deletions(-)
>
> diff --git a/t/perf/p5310-pack-bitmaps.sh b/t/perf/p5310-pack-bitmaps.sh
> index 7ad4f237bc3..6ff42bdd391 100755
> --- a/t/perf/p5310-pack-bitmaps.sh
> +++ b/t/perf/p5310-pack-bitmaps.sh
> @@ -16,39 +16,48 @@ test_expect_success 'setup bitmap config' '
>  	git config pack.writebitmaps true
>  '
>
> -# we need to create the tag up front such that it is covered by the repack and
> -# thus by generated bitmaps.
> -test_expect_success 'create tags' '
> -	git tag --message="tag pointing to HEAD" perf-tag HEAD
> -'
> -
> -test_perf 'repack to disk' '
> -	git repack -ad
> -'
> -
> -test_full_bitmap
> -
> -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) &&
> -	orig_tip=$(git rev-parse HEAD) &&
> -
> -	# now kill off all of the refs and pretend we had
> -	# just the one tip
> -	rm -rf .git/logs .git/refs/* .git/packed-refs &&
> -	git update-ref HEAD $cutoff &&
> -
> -	# and then repack, which will leave us with a nice
> -	# big bitmap pack of the "old" history, and all of
> -	# the new history will be loose, as if it had been pushed
> -	# up incrementally and exploded via unpack-objects
> -	git repack -Ad &&
> -
> -	# and now restore our original tip, as if the pushes
> -	# had happened
> -	git update-ref HEAD $orig_tip
> -'
> -
> -test_partial_bitmap
> +test_bitmap () {
> +    local enabled="$1"
> +
> +	# we need to create the tag up front such that it is covered by the repack and
> +	# thus by generated bitmaps.
> +	test_expect_success 'create tags' '
> +		git tag --message="tag pointing to HEAD" perf-tag HEAD
> +	'

I think this "create tags" step can happen outside of the test_bitmap()
function, since it should only need to be done once, right?

> +	test_expect_success "use lookup table: $enabled" '
> +		git config pack.writeBitmapLookupTable '"$enabled"'
> +	'
> +
> +	test_perf "repack to disk (lookup=$enabled)" '
> +		git repack -ad
> +	'

And I think these two tests could be combined, since this could just
become:

    git -c pack.writeBitmapLookupTable "$enabled" repack -ad

right?

> +	test_full_bitmap
> +
> +    test_expect_success "create partial bitmap state (lookup=$enabled)" '

There is some funky spacing going on here, at least in my email client.
Could you double check that tabs are used consistently here?

Thanks,
Taylor