Re: [PATCH v2 16/20] builtin/gc.c: guess the size of the revindex

From: Junio C Hamano <gitster@pobox.com>
To: Taylor Blau <me@ttaylorr.com>
Cc: git@vger.kernel.org, dstolee@microsoft.com, jrnieder@gmail.com,
	peff@peff.net
Subject: Re: [PATCH v2 16/20] builtin/gc.c: guess the size of the revindex
Date: Wed, 13 Jan 2021 22:33:01 -0800	[thread overview]
Message-ID: <xmqqwnwgyqn6.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <a500311e33a2f7e11a539dd0718ed946f4bd6bc8.1610576604.git.me@ttaylorr.com> (Taylor Blau's message of "Wed, 13 Jan 2021 17:24:54 -0500")

Taylor Blau <me@ttaylorr.com> writes:

> 'estimate_repack_memory()' takes into account the amount of memory
> required to load the reverse index in memory by multiplying the assumed
> number of objects by the size of the 'revindex_entry' struct.
>
> Prepare for hiding the definition of 'struct revindex_entry' by removing
> a 'sizeof()' of that type from outside of pack-revindex.c. Instead,
> guess that one off_t and one uint32_t are required per object. Strictly
> speaking, this is a worse guess than asking for 'sizeof(struct
> revindex_entry)' directly, since the true size of this struct is 16
> bytes with padding on the end of the struct in order to align the offset
> field.

Meaning that we under-estimate by 25%?

> But, this is an approximation anyway, and it does remove a use of the
> 'struct revindex_entry' from outside of pack-revindex internals.
>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
>  builtin/gc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/gc.c b/builtin/gc.c
> index 4c24f41852..c60811f212 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -301,7 +301,7 @@ static uint64_t estimate_repack_memory(struct packed_git *pack)
>  	/* and then obj_hash[], underestimated in fact */
>  	heap += sizeof(struct object *) * nr_objects;
>  	/* revindex is used also */
> -	heap += sizeof(struct revindex_entry) * nr_objects;
> +	heap += (sizeof(off_t) + sizeof(uint32_t)) * nr_objects;
>  	/*
>  	 * read_sha1_file() (either at delta calculation phase, or
>  	 * writing phase) also fills up the delta base cache