| Message ID | 98e72f71b6bec6f5c2df4139ca3df37d97ddcf54.1658244366.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 068fa54c0034be0b953d798628b06815f9cfaff0 |
| Headers | show |
| Series | midx: reduce memory pressure while writing bitmaps | expand |
"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);
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) { @@ -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. + */ if (ctx.m) close_object_store(the_repository->objects);