Re: [PATCH 5/7] p5313: add size comparison test

[Taylor Blau <me@ttaylorr.com>]

On Tue, Nov 05, 2024 at 03:05:05AM +0000, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <stolee@gmail.com>
>
> As custom options are added to 'git pack-objects' and 'git repack' to
> adjust how compression is done, use this new performance test script to
> demonstrate their effectiveness in performance and size.

Nicely done, thank you for adding a perf test to allow readers to easily
verify these changes themselves.

> In the case of the Git repository, these numbers show some of the issues
> with this approach:
>
> [...]
>
> The thin pack that simulates a push is much worse with --full-name-hash
> in this case. The name hash values are doing a lot to assist with delta
> bases, it seems. The big pack and shallow clone cases are slightly worse
> with the --full-name-hash option. Only the full repack gains some
> benefits in size.

Not a problem with your patch, but just thinking aloud: do you think
there is an easy/straightforward way to suggest when to use
--full-name-hash or not?

> ---
>  t/perf/p5313-pack-objects.sh | 94 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 94 insertions(+)
>  create mode 100755 t/perf/p5313-pack-objects.sh
>
> diff --git a/t/perf/p5313-pack-objects.sh b/t/perf/p5313-pack-objects.sh
> new file mode 100755
> index 00000000000..dfa29695315
> --- /dev/null
> +++ b/t/perf/p5313-pack-objects.sh
> @@ -0,0 +1,94 @@
> +#!/bin/sh
> +
> +test_description='Tests pack performance using bitmaps'
> +. ./perf-lib.sh
> +
> +GIT_TEST_PASSING_SANITIZE_LEAK=0
> +export GIT_TEST_PASSING_SANITIZE_LEAK
> +
> +test_perf_large_repo
> +
> +test_expect_success 'create rev input' '
> +	cat >in-thin <<-EOF &&
> +	$(git rev-parse HEAD)
> +	^$(git rev-parse HEAD~1)
> +	EOF
> +
> +	cat >in-big <<-EOF &&
> +	$(git rev-parse HEAD)
> +	^$(git rev-parse HEAD~1000)
> +	EOF
> +
> +	cat >in-shallow <<-EOF
> +	$(git rev-parse HEAD)
> +	--shallow $(git rev-parse HEAD)
> +	EOF
> +'

I was going to comment that these could probably be moved into the
individual perf test that cares about reading each of these inputs. But
having them shared here makes sense since we are naturally comparing
generating two packs with the same input (with and without
--full-name-hash). So the shared setup here makes sense to me.

> +
> +test_perf 'thin pack' '
> +	git pack-objects --thin --stdout --revs --sparse  <in-thin >out
> +'
> +
> +test_size 'thin pack size' '
> +	test_file_size out
> +'

Nice. I always forget about this and end up writing 'wc -c <out'.

> +test_perf 'thin pack with --full-name-hash' '
> +	git pack-objects --thin --stdout --revs --sparse --full-name-hash <in-thin >out
> +'
> +
> +test_size 'thin pack size with --full-name-hash' '
> +	test_file_size out
> +'
> +
> +test_perf 'big pack' '
> +	git pack-objects --stdout --revs --sparse  <in-big >out
> +'
> +
> +test_size 'big pack size' '
> +	test_file_size out
> +'
> +
> +test_perf 'big pack with --full-name-hash' '
> +	git pack-objects --stdout --revs --sparse --full-name-hash <in-big >out
> +'
> +
> +test_size 'big pack size with --full-name-hash' '
> +	test_file_size out
> +'
> +
> +test_perf 'shallow fetch pack' '
> +	git pack-objects --stdout --revs --sparse --shallow <in-shallow >out
> +'
> +
> +test_size 'shallow pack size' '
> +	test_file_size out
> +'
> +
> +test_perf 'shallow pack with --full-name-hash' '
> +	git pack-objects --stdout --revs --sparse --shallow --full-name-hash <in-shallow >out
> +'
> +
> +test_size 'shallow pack size with --full-name-hash' '
> +	test_file_size out
> +'
> +
> +test_perf 'repack' '
> +	git repack -adf
> +'
> +
> +test_size 'repack size' '
> +	pack=$(ls .git/objects/pack/pack-*.pack) &&
> +	test_file_size "$pack"

Here and below, I think it's fine to inline this as in:

    test_file_size "$(ls .git/objects/pack/pack-*.pack)"

...but I wonder: will using ".git" break this test in bare repositories?
Should we write instead:

    pack="$(ls $(git rev-parse --git-dir)/objects/pack/pack-*.pack)" &&
    test_file_size

?

Thanks,
Taylor