| Message ID | pull.1292.git.1658176565751.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | midx: reduce memory pressure while writing bitmaps | expand |
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?
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
diff --git a/midx.c b/midx.c index 5f0dd386b02..cc31d803a5f 100644 --- a/midx.c +++ b/midx.c @@ -1063,6 +1063,7 @@ static int write_midx_bitmap(char *midx_name, unsigned char *midx_hash, struct commit **commits = NULL; uint32_t i, commits_nr; uint16_t options = 0; + uint32_t *pack_order; char *bitmap_name = xstrfmt("%s-%s.bitmap", midx_name, hash_to_hex(midx_hash)); int ret; @@ -1076,6 +1077,15 @@ static int write_midx_bitmap(char *midx_name, unsigned char *midx_hash, commits = find_commits_for_midx_bitmap(&commits_nr, refs_snapshot, ctx); + /* + * 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; + /* * Build the MIDX-order index based on pdata.objects (which is already * in MIDX order; c.f., 'midx_pack_order_cmp()' for the definition of @@ -1102,7 +1112,7 @@ static int write_midx_bitmap(char *midx_name, unsigned char *midx_hash, * bitmap_writer_finish(). */ for (i = 0; i < pdata.nr_objects; i++) - index[ctx->pack_order[i]] = &pdata.objects[i].idx; + index[pack_order[i]] = &pdata.objects[i].idx; bitmap_writer_select_commits(commits, commits_nr, -1); ret = bitmap_writer_build(&pdata); @@ -1443,6 +1453,11 @@ static int write_midx_internal(const char *object_dir, if (flags & MIDX_WRITE_REV_INDEX && git_env_bool("GIT_TEST_MIDX_WRITE_REV", 0)) write_midx_reverse_index(midx_name.buf, midx_hash, &ctx); + + /* + * Writing the bitmap must be last, as it will free ctx.entries + * to reduce memory pressure during the bitmap write. + */ if (flags & MIDX_WRITE_BITMAP) { if (write_midx_bitmap(midx_name.buf, midx_hash, &ctx, refs_snapshot, flags) < 0) {