midx: reduce memory pressure while writing bitmaps

[derrickstolee]

We noticed an instance where writing multi-pack-index bitmaps was taking a lot of memory. This small change can reduce the memory pressure slightly (~25%), but more will be needed to significantly reduce the memory pressure. Such a change would require updating the bitmap writing code to use on-disk data structures instead. This is particularly tricky when the multi-pack-index has not been fully written, because we don't want a point in time where the object store has a new multi-pack-index without a reachability bitmap.

Updates in v2

To reduce confusion on the lifetime of 'ctx.entries', some refactoring patches are inserted to first extract the use of 'ctx' out of write_midx_bitmap() and into write_midx_internal(). This makes the FREE_AND_NULL() stand out more clearly.

Thanks,
-Stolee

cc: gitster@pobox.com
cc: me@ttaylorr.com
cc: vdye@github.com
cc: chakrabortyabhradeep79@gmail.com
cc: Ævar Arnfjörð Bjarmason avarab@gmail.com

[gitgitgadget]

There are issues in commit 9bb826c:
midx: reduce memory pressure while writing bitmaps
Lines in the body of the commit messages should be wrapped between 60 and 76 characters.

[derrickstolee]

/submit

[gitgitgadget]

Submitted as pull.1292.git.1658176565751.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1292/derrickstolee/bitmap-memory-v1

To fetch this version to local tag pr-1292/derrickstolee/bitmap-memory-v1:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1292/derrickstolee/bitmap-memory-v1

[gitgitgadget]

On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):

On Mon, Jul 18 2022, Derrick Stolee via GitGitGadget wrote:

> From: Derrick Stolee <derrickstolee@github.com>
> [...]
> It is unfortunate that the lifetime of the 'entries' array is less
> clear. To make this simpler, I added a few things to try and prevent an
> accidental reference:
>
>  1. Using FREE_AND_NULL() we will at least get a segfault from reading a
>     NULL pointer instead of a use-after-free.
>
>  2. 'entries_nr' is also set to zero to make any loop that would iterate
>     over the entries be trivial.
>
>  3. Set the 'ctx' pointer to NULL within write_midx_bitmap() so it does
>     not get another reference later. This requires adding a local copy
>     of 'pack_order' giving us a reference that we can use later in the
>     method.
>
>  4. Add significant comments in write_midx_bitmap() and
>     write_midx_internal() to add warnings for future authors who might
>     accidentally add references to this cleared memory.
> [...]
> +	/*
> +	 * Remove the ctx.entries to reduce memory pressure.
> +	 * Nullify 'ctx' to help avoid adding new references to ctx->entries.
> +	 */
> +	FREE_AND_NULL(ctx->entries);
> +	ctx->entries_nr = 0;
> +	pack_order = ctx->pack_order;
> +	ctx = NULL;

After this change this is a ~70 line function, but only 3 lines at the
top actually use ctx for anything:
    
	/* the bug check for ctx.nr... */
	prepare_midx_packing_data(&pdata, ctx);
	commits = find_commits_for_midx_bitmap(&commits_nr, refs_snapshot, ctx);

Did you consider just splitting it up so that that there's a "prepare
write" function? Then you don't need to worry about the scoping of ctx.

I'd think that would be better, then you also wouldn't need to implement
your own free-ing, nothing after this seems to use ctx->entries_nr (but
I just skimmed it), so it could just fall through to the free() at the
end of write_midx_internal() (the only caller), couldn't it?

[gitgitgadget]

User Ævar Arnfjörð Bjarmason <avarab@gmail.com> has been added to the cc: list.

[gitgitgadget]

This branch is now known as ds/midx-with-less-memory.

[gitgitgadget]

This patch series was integrated into seen via git@b1a51ed.

[gitgitgadget]

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 7/18/2022 5:47 PM, Ævar Arnfjörð Bjarmason wrote:
> 
> On Mon, Jul 18 2022, Derrick Stolee via GitGitGadget wrote:
> 
>> From: Derrick Stolee <derrickstolee@github.com>
>> [...]
>> It is unfortunate that the lifetime of the 'entries' array is less
>> clear. To make this simpler, I added a few things to try and prevent an
>> accidental reference:
>>
>>  1. Using FREE_AND_NULL() we will at least get a segfault from reading a
>>     NULL pointer instead of a use-after-free.
>>
>>  2. 'entries_nr' is also set to zero to make any loop that would iterate
>>     over the entries be trivial.
>>
>>  3. Set the 'ctx' pointer to NULL within write_midx_bitmap() so it does
>>     not get another reference later. This requires adding a local copy
>>     of 'pack_order' giving us a reference that we can use later in the
>>     method.
>>
>>  4. Add significant comments in write_midx_bitmap() and
>>     write_midx_internal() to add warnings for future authors who might
>>     accidentally add references to this cleared memory.
>> [...]
>> +	/*
>> +	 * Remove the ctx.entries to reduce memory pressure.
>> +	 * Nullify 'ctx' to help avoid adding new references to ctx->entries.
>> +	 */
>> +	FREE_AND_NULL(ctx->entries);
>> +	ctx->entries_nr = 0;
>> +	pack_order = ctx->pack_order;
>> +	ctx = NULL;
> 
> After this change this is a ~70 line function, but only 3 lines at the
> top actually use ctx for anything:
>     
> 	/* the bug check for ctx.nr... */
> 	prepare_midx_packing_data(&pdata, ctx);
> 	commits = find_commits_for_midx_bitmap(&commits_nr, refs_snapshot, ctx);
> 
> Did you consider just splitting it up so that that there's a "prepare
> write" function? Then you don't need to worry about the scoping of ctx.

I did not, and that's a good suggestion. Extracting these prepare steps
into write_midx_internal() works to reduce the complexity and make the
early free()ing more clear.
 
> I'd think that would be better, then you also wouldn't need to implement
> your own free-ing, nothing after this seems to use ctx->entries_nr (but
> I just skimmed it), so it could just fall through to the free() at the
> end of write_midx_internal() (the only caller), couldn't it?

I think this paragraph misunderstands the point. The bitmaps are being
computed and written before the MIDX lock file completes, so the free()
of the entries array is after the bitmaps are computed. To reduce the
memory pressure (by ~25%) by freeing early is the point of this patch.

We still want that free(ctx.entries) after the cleanup: label for the
error cases, but for the "happy path" we can free early.

By doing the refactoring, this point of having an earlier free() makes
things more clear.

Thanks,
-Stolee

[derrickstolee]

/submit

[gitgitgadget]

Submitted as pull.1292.v2.git.1658244366.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1292/derrickstolee/bitmap-memory-v2

To fetch this version to local tag pr-1292/derrickstolee/bitmap-memory-v2:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1292/derrickstolee/bitmap-memory-v2

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

"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);

[gitgitgadget]

This patch series was integrated into seen via git@134fbc6.

[gitgitgadget]

This patch series was integrated into seen via git@8dc290c.

[gitgitgadget]

There was a status update in the "New Topics" section about the branch ds/midx-with-less-memory on the Git mailing list:

The codepath to write multi-pack index has been taught to release a
large chunk of memory that holds an array of objects in the packs,
as soon as it is done with the array, to reduce memory consumption.

Will merge to 'next'?
source: <pull.1292.v2.git.1658244366.gitgitgadget@gmail.com>

[gitgitgadget]

This patch series was integrated into seen via git@96374cc.

[gitgitgadget]

This patch series was integrated into seen via git@19229e3.

[gitgitgadget]

This patch series was integrated into next via git@250d257.