Subject: Re: [PATCH] pack-write: skip *.rev work when not writing *.rev

From: Taylor Blau <[email-masked]>
Date: Tue, 7 Sep 2021 22:50:58 -0400

On Wed, Sep 08, 2021 at 03:08:03AM +0200, Ævar Arnfjörð Bjarmason wrote:
> Fix a performance regression introduced in a587b5a786 (pack-write.c:
> extract 'write_rev_file_order', 2021-03-30) and stop needlessly
> allocating the "pack_order" array and sorting it with
> "pack_order_cmp()", only to throw that work away when we discover that
> we're not writing *.rev files after all.
>
> This redundant work was not present in the original version of this
> code added in 8ef50d9958 (pack-write.c: prepare to write 'pack-*.rev'
> files, 2021-01-25). There we'd call write_rev_file() from
> e.g. finish_tmp_packfile(), but we'd "return NULL" early in
> write_rev_file() if not doing a "WRITE_REV" or "WRITE_REV_VERIFY".
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <[email-masked]>
> ---
>  pack-write.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/pack-write.c b/pack-write.c
> index f1fc3ecafa..1883848e7c 100644
> --- a/pack-write.c
> +++ b/pack-write.c
> @@ -224,6 +224,9 @@ const char *write_rev_file(const char *rev_name,
>  	uint32_t i;
>  	const char *ret;
>
> +	if (!(flags & WRITE_REV) && !(flags & WRITE_REV_VERIFY))
> +		return NULL;
> +

Great catch! This fix as-is is obviously correct, but it does make the
same checks in write_rev_file_order redundant as a result.

If we call write_rev_file() without WRITE_REV, then we'll never call
write_rev_file_order(). The other caller of write_rev_file_order() is
from midx.c:write_midx_reverse_index(), which is only called by
write_midx_internal() where it is guarded by a similar conditional.

So I think we could probably either:

  - remove the check from write_rev_file_order() altogether, moving it
    to write_rev_file() like you did here, or

  - remove the check from write_rev_file_order() and elevate it to the
    caller which is missing the check in finish_tmp_packfile()

Of the two, I think the former is more appealing (since no other
functions called by finish_tmp_packfile() are guarded like that; they
conditionally behave as noops depending on `flags`).

But what you wrote here is just fine as-is, so the above are just some
optional ideas for potential improvements.

Thanks,
Taylor