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

[Taylor Blau <me@ttaylorr.com>]

On Mon, Jun 20, 2022 at 12:33:14PM +0000, Abhradeep Chakraborty via GitGitGadget wrote:
> From: Abhradeep Chakraborty <chakrabortyabhradeep79@gmail.com>
>
> Add performance tests for bitmap lookup table extension.

These tests look good, though I left a few notes below which boil down
to recommending a separate commit to set pack.writeReverseIndex=true,
and some suggestions for how to clean up the diff in the two performance
scripts you modified.

I would be interested to see the relevant results from running these
perf scripts on a reasonably large-sized repository, e.g. the kernel or
similar.

For the next version of this series, would you mind running these
scripts and including the results in this commit message?

> Mentored-by: Taylor Blau <ttaylorr@github.com>
> Co-mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
> Signed-off-by: Abhradeep Chakraborty <chakrabortyabhradeep79@gmail.com>
> ---
>  t/perf/p5310-pack-bitmaps.sh       | 60 +++++++++++++++++++-----------
>  t/perf/p5326-multi-pack-bitmaps.sh | 55 +++++++++++++++++----------
>  2 files changed, 73 insertions(+), 42 deletions(-)
>
> diff --git a/t/perf/p5310-pack-bitmaps.sh b/t/perf/p5310-pack-bitmaps.sh
> index 7ad4f237bc3..a8d9414de92 100755
> --- a/t/perf/p5310-pack-bitmaps.sh
> +++ b/t/perf/p5310-pack-bitmaps.sh
> @@ -10,10 +10,11 @@ test_perf_large_repo
>  # since we want to be able to compare bitmap-aware
>  # git versus non-bitmap git
>  #
> -# We intentionally use the deprecated pack.writebitmaps
> +# We intentionally use the deprecated pack.writeBitmaps
>  # config so that we can test against older versions of git.
>  test_expect_success 'setup bitmap config' '
> -	git config pack.writebitmaps true
> +	git config pack.writeBitmaps true &&
> +	git config pack.writeReverseIndex true

I suspect that eliminating the overhead of generating the reverse index
in memory is important to see the effect of this test. We should make
sure that this is done in a separate step so when we compare two commits
that both have a reverse index written.

That being said, we should probably make reverse indexes be the default
anyways, since they help significantly with all kinds of things (really,
any operation which has to generate a reverse index in memory, like
preparing a pack to push, the '%(objectsize:disk)' cat-file formatting
atom, and so on.

So at a minimum I would suggest extracting a separate commit here which
sets pack.writeReverseIndex to true for this test. That way the commit
prior to this has reverse indexes written, and comparing "this commit"
to "the previous one" is isolating the effect of just the lookup table.

But as a useful sideproject, it would be worthwhile to investigate
setting this to true by default everywhere, perhaps after this series
has settled a little more (or if you are blocked / want something else
to do).

>  '
>
>  # we need to create the tag up front such that it is covered by the repack and
> @@ -28,27 +29,42 @@ test_perf 'repack to disk' '
>
>  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_perf 'use lookup table' '
> +    git config pack.writeBitmapLookupTable true
>  '

This part doesn't need to use 'test_perf', since we don't care about the
performance of running "git config". Instead, using
`test_expect_success` is more appropriate here.

> -test_partial_bitmap
> +test_perf 'repack to disk (lookup table)' '
> +    git repack -adb
> +'
> +
> +test_full_bitmap
> +
> +for i in false true
> +do
> +	$i && lookup=" (lookup table)"
> +	test_expect_success "create partial bitmap state$lookup" '
> +		git config pack.writeBitmapLookupTable '"$i"' &&
> +		# 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
> +done

Could we extract the body of this loop into a function whose first
argument is either true/false? I think that would improve readability
here, and potentially clean up the diff a little bit.

For what it's worth, I don't think we need to do anything fancier for
the test name other than:


    test_partial_bitmap () {
      local enabled="$1"
      test_expect_success "create partial bitmap state (lookup=$enabled)" '
        git config pack.writeBitmapLookupTable "$enabled" &&
        [...]
      '
    }

    test_partial_bitmap false
    test_partial_bitmap true

or something.

> +for i in false true
> +do
> +	$i && lookup=" (lookup table)"
> +	test_expect_success "create partial bitmap state$lookup" '
> +		git config pack.writeBitmapLookupTable '"$i"' &&
> +		# 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 pretend we have just 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 &&
> +		git multi-pack-index write --bitmap &&
> +
> +		# and now restore our original tip, as if the pushes
> +		# had happened
> +		git update-ref HEAD $orig_tip
> +	'
> +
> +	test_partial_bitmap
> +done

Same note here.

Thanks,
Taylor