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

From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, gitster@pobox.com, me@ttaylorr.com,
	vdye@github.com, chakrabortyabhradeep79@gmail.com,
	Derrick Stolee <derrickstolee@github.com>
Subject: Re: [PATCH] midx:  reduce memory pressure while writing bitmaps
Date: Mon, 18 Jul 2022 23:47:12 +0200	[thread overview]
Message-ID: <220718.867d4aw0k3.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <pull.1292.git.1658176565751.gitgitgadget@gmail.com>

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?