Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> wrote:
> On Thu, Feb 02 2023, Eric Wong wrote:
>
> > + kh_foreach_value(island_marks, bitmap, {
> > + if (--bitmap->refcount == 0)
>
> Style nit: if (!--x) rather than if (--x == 0) ?
Oops, fixed in v2
> > + free(bitmap);
> > + });
> > + kh_destroy_oid_map(island_marks);
> > + island_marks = (void *)1; /* crash on unintended future use */
>
> This seems counter-productive. If you just leave it free'd then various
> analysis tools will spot a use-after-free earlier, won't they?
*shrug* I thought it might be better to use an explicitly bad
pointer to catch lifetime issues that I might've missed from
reading the code. Since I've run and tested at this point,
it probably doesn't matter, now. So I'll just omit the
assignment and save some icache footprint.
Thanks for the review!
--------8<-------
Subject: [PATCH] delta-islands: free island_marks and bitmaps
On my mirror of linux.git forkgroup with 780 islands, this saves
nearly 4G of heap memory in pack-objects. This savings only
benefits delta island users of pack bitmaps, as the process
would otherwise be exiting anyways.
However, there's probably not many delta island users, but the
majority of delta island users would also be pack bitmaps users.
Signed-off-by: Eric Wong <e@xxxxxxxxx>
---
delta-islands.c | 19 ++++++++++++++++++-
1 file changed, 18 insertions(+), 1 deletion(-)
diff --git a/delta-islands.c b/delta-islands.c
index 90c0d6958f..c09dab31a4 100644
--- a/delta-islands.c
+++ b/delta-islands.c
@@ -513,12 +513,28 @@ void propagate_island_marks(struct commit *commit)
}
}
+static void free_island_marks(void)
+{
+ struct island_bitmap *bitmap;
+
+ kh_foreach_value(island_marks, bitmap, {
+ if (!--bitmap->refcount)
+ free(bitmap);
+ });
+ kh_destroy_oid_map(island_marks);
+}
+
int compute_pack_layers(struct packing_data *to_pack)
{
uint32_t i;
- if (!core_island_name || !island_marks)
+ if (!island_marks)
+ return 1;
+
+ if (!core_island_name) {
+ free_island_marks();
return 1;
+ }
for (i = 0; i < to_pack->nr_objects; ++i) {
struct object_entry *entry = &to_pack->objects[i];
@@ -533,6 +549,7 @@ int compute_pack_layers(struct packing_data *to_pack)
oe_set_layer(to_pack, entry, 0);
}
}
+ free_island_marks();
return 2;
}
