delta-islands: free island_marks and bitmaps

Commit Message

Eric Wong Feb. 2, 2023, 1:03 a.m. UTC

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-mail]>
---
 Slowly chipping away, this is low-hanging fruit.

 delta-islands.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

Comments

Ævar Arnfjörð Bjarmason Feb. 2, 2023, 1:45 a.m. UTC | #1

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) ?

> +			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?

Patch

diff --git a/delta-islands.c b/delta-islands.c
index 90c0d6958f..7052a3a6ba 100644
--- a/delta-islands.c
+++ b/delta-islands.c
@@ -513,12 +513,29 @@  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 == 0)
+			free(bitmap);
+	});
+	kh_destroy_oid_map(island_marks);
+	island_marks = (void *)1; /* crash on unintended future use */
+}
+
 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 +550,7 @@  int compute_pack_layers(struct packing_data *to_pack)
 				oe_set_layer(to_pack, entry, 0);
 		}
 	}
+	free_island_marks();
 
 	return 2;
 }