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

From: Derrick Stolee <stolee@gmail.com>
Date: Jan 11, 2021, 11:52 UTC
Message-ID: <87cd1b2c-7a28-da77-4ae4-99ffbbdfda72@gmail.com>
In-Reply-To: <7c17db7a7df8b524f13969efd1cb5e6e95de5a2d.1610129796.git.me@ttaylorr.com>

On 1/8/2021 1:17 PM, Taylor Blau wrote:

Show 11 quoted lines

> '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.

This is so far the only not-completely-obvious change.

> But, this is an approximation anyway, and it does remove a use of the
> 'struct revindex_entry' from outside of pack-revindex internals.

And this might be enough justification for it, but...

> -	heap += sizeof(struct revindex_entry) * nr_objects;
> +	heap += (sizeof(off_t) + sizeof(uint32_t)) * nr_objects;

...outside of the estimation change, will this need another change when the rev-index is mmap'd? Should this instead be an API call, such as estimate_rev_index_memory(nr_objects)? That would centralize the estimate to be next to the code that currently interacts with 'struct revindex_entry' and will later interact with the mmap region.

Thanks, -Stolee