Re: [PATCH v2 3/3] midx: reduce memory pressure while writing bitmaps

[Junio C Hamano <gitster@pobox.com>]

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

> diff --git a/midx.c b/midx.c
> index e2dd808b35d..772ab7d2944 100644
> --- a/midx.c
> +++ b/midx.c
> @@ -1451,6 +1451,15 @@ static int write_midx_internal(const char *object_dir,
>  
>  		commits = find_commits_for_midx_bitmap(&commits_nr, refs_snapshot, &ctx);
>  
> +		/*
> +		 * The previous steps translated the information from
> +		 * 'entries' into information suitable for constructing
> +		 * bitmaps. We no longer need that array, so clear it to
> +		 * reduce memory pressure.
> +		 */
> +		FREE_AND_NULL(ctx.entries);
> +		ctx.entries_nr = 0;
> +
>  		if (write_midx_bitmap(midx_name.buf, midx_hash, &pdata,
>  				      commits, commits_nr, ctx.pack_order,
>  				      refs_snapshot, flags) < 0) {

As the reduced helper, thanks to step [1/3], only takes the
pack_order[] array, without being even aware of other members in the
ctx struct, it is immediately obvious that this early freeing is
safe for this call.  It is a bit messy.

I've been staring at the code and was wondering if we can just get
rid of pack_order member from the context, and make pack_order a
separate local variable that belong to this function.  The separate
variable needs to be packaged together with ctx back to please the
chunk-format API, so it may require more boilerplate code and may
not be an overall win.

> @@ -1459,6 +1468,10 @@ static int write_midx_internal(const char *object_dir,
>  			goto cleanup;
>  		}
>  	}
> +	/*
> +	 * NOTE: Do not use ctx.entries beyond this point, since it might
> +	 * have been freed in the previous if block.
> +	 */

OK.

>  	if (ctx.m)
>  		close_object_store(the_repository->objects);