[PATCH 6/6] pack-objects: drop packlist index_pos optimization

[Jeff King <peff@peff.net>]

Once upon a time, the code to add an object to our packing list in
pack-objects all lived in a single function. It computed the position
within the hash table once, then used it to check if the object was
already present, and if not, to add it.

Later, in 2834bc27c1 (pack-objects: refactor the packing list,
2013-10-24), this was split into two functions: packlist_find() and
packlist_alloc(). We ended up with an "index_pos" variable that gets
passed through several functions to make it from one to the other.

The resulting code is rather confusing to follow. The "index_pos"
variable is sometimes undefined, if we don't yet have a hash table. This
works out in practice because in that case packlist_alloc() won't use
our index_pos at all, since it will have to create/grow the hash table
fresh (and reassign the index for all entries). But it's hard to verify
that, and it does cause gcc 9.2.1's -Wmaybe-uninitialized to complain
when compiled with "-flto -O3" (rightfully, since we do pass the
uninitialized value as a function parameter, even if nobody ends up
using it).

All of this is to save computing the hash index again when we're
inserting into the hash table, which I found doesn't make a measurable
difference in the program runtime (which is not surprising, since we're
doing all kinds of other heavyweight things for each object).

Let's just drop this index_pos variable entirely, simplifying the code
(and pleasing the compiler).

We might be better still refactoring this custom hash table to use one
of our existing implementations (an oidmap, or a kh_oid_map). I stopped
short of that here, but this would be the likely first step towards that
anyway.

Reported-by: Stephan Beyer <s-beyer@gmx.net>
Signed-off-by: Jeff King <peff@peff.net>
---
I suspect one advantage of this custom hash is that it stores a uint32_t
instead of the hash, probably saving 16 bytes per object. But that's
something we can think about when looking at converting the hash.

 builtin/pack-objects.c | 33 ++++++++++++++-------------------
 pack-bitmap-write.c    |  2 +-
 pack-bitmap.c          |  2 +-
 pack-objects.c         | 18 +++++++++---------
 pack-objects.h         |  6 ++----
 5 files changed, 27 insertions(+), 34 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index dc2a7e9ac0..9a8d935700 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -610,12 +610,12 @@ static int mark_tagged(const char *path, const struct object_id *oid, int flag,
 		       void *cb_data)
 {
 	struct object_id peeled;
-	struct object_entry *entry = packlist_find(&to_pack, oid, NULL);
+	struct object_entry *entry = packlist_find(&to_pack, oid);
 
 	if (entry)
 		entry->tagged = 1;
 	if (!peel_ref(path, &peeled)) {
-		entry = packlist_find(&to_pack, &peeled, NULL);
+		entry = packlist_find(&to_pack, &peeled);
 		if (entry)
 			entry->tagged = 1;
 	}
@@ -996,12 +996,11 @@ static int no_try_delta(const char *path)
  * few lines later when we want to add the new entry.
  */
 static int have_duplicate_entry(const struct object_id *oid,
-				int exclude,
-				uint32_t *index_pos)
+				int exclude)
 {
 	struct object_entry *entry;
 
-	entry = packlist_find(&to_pack, oid, index_pos);
+	entry = packlist_find(&to_pack, oid);
 	if (!entry)
 		return 0;
 
@@ -1141,13 +1140,12 @@ static void create_object_entry(const struct object_id *oid,
 				uint32_t hash,
 				int exclude,
 				int no_try_delta,
-				uint32_t index_pos,
 				struct packed_git *found_pack,
 				off_t found_offset)
 {
 	struct object_entry *entry;
 
-	entry = packlist_alloc(&to_pack, oid, index_pos);
+	entry = packlist_alloc(&to_pack, oid);
 	entry->hash = hash;
 	oe_set_type(entry, type);
 	if (exclude)
@@ -1171,11 +1169,10 @@ static int add_object_entry(const struct object_id *oid, enum object_type type,
 {
 	struct packed_git *found_pack = NULL;
 	off_t found_offset = 0;
-	uint32_t index_pos;
 
 	display_progress(progress_state, ++nr_seen);
 
-	if (have_duplicate_entry(oid, exclude, &index_pos))
+	if (have_duplicate_entry(oid, exclude))
 		return 0;
 
 	if (!want_object_in_pack(oid, exclude, &found_pack, &found_offset)) {
@@ -1190,7 +1187,7 @@ static int add_object_entry(const struct object_id *oid, enum object_type type,
 
 	create_object_entry(oid, type, pack_name_hash(name),
 			    exclude, name && no_try_delta(name),
-			    index_pos, found_pack, found_offset);
+			    found_pack, found_offset);
 	return 1;
 }
 
@@ -1199,17 +1196,15 @@ static int add_object_entry_from_bitmap(const struct object_id *oid,
 					int flags, uint32_t name_hash,
 					struct packed_git *pack, off_t offset)
 {
-	uint32_t index_pos;
-
 	display_progress(progress_state, ++nr_seen);
 
-	if (have_duplicate_entry(oid, 0, &index_pos))
+	if (have_duplicate_entry(oid, 0))
 		return 0;
 
 	if (!want_object_in_pack(oid, 0, &pack, &offset))
 		return 0;
 
-	create_object_entry(oid, type, name_hash, 0, 0, index_pos, pack, offset);
+	create_object_entry(oid, type, name_hash, 0, 0, pack, offset);
 	return 1;
 }
 
@@ -1507,7 +1502,7 @@ static int can_reuse_delta(const unsigned char *base_sha1,
 	 * First see if we're already sending the base (or it's explicitly in
 	 * our "excluded" list).
 	 */
-	base = packlist_find(&to_pack, &base_oid, NULL);
+	base = packlist_find(&to_pack, &base_oid);
 	if (base) {
 		if (!in_same_island(&delta->idx.oid, &base->idx.oid))
 			return 0;
@@ -2579,7 +2574,7 @@ static void add_tag_chain(const struct object_id *oid)
 	 * it was included via bitmaps, we would not have parsed it
 	 * previously).
 	 */
-	if (packlist_find(&to_pack, oid, NULL))
+	if (packlist_find(&to_pack, oid))
 		return;
 
 	tag = lookup_tag(the_repository, oid);
@@ -2603,7 +2598,7 @@ static int add_ref_tag(const char *path, const struct object_id *oid, int flag,
 
 	if (starts_with(path, "refs/tags/") && /* is a tag? */
 	    !peel_ref(path, &peeled)    && /* peelable? */
-	    packlist_find(&to_pack, &peeled, NULL))      /* object packed? */
+	    packlist_find(&to_pack, &peeled))      /* object packed? */
 		add_tag_chain(oid);
 	return 0;
 }
@@ -2803,7 +2798,7 @@ static void show_object(struct object *obj, const char *name, void *data)
 		for (p = strchr(name, '/'); p; p = strchr(p + 1, '/'))
 			depth++;
 
-		ent = packlist_find(&to_pack, &obj->oid, NULL);
+		ent = packlist_find(&to_pack, &obj->oid);
 		if (ent && depth > oe_tree_depth(&to_pack, ent))
 			oe_set_tree_depth(&to_pack, ent, depth);
 	}
@@ -3034,7 +3029,7 @@ static void loosen_unused_packed_objects(void)
 
 		for (i = 0; i < p->num_objects; i++) {
 			nth_packed_object_oid(&oid, p, i);
-			if (!packlist_find(&to_pack, &oid, NULL) &&
+			if (!packlist_find(&to_pack, &oid) &&
 			    !has_sha1_pack_kept_or_nonlocal(&oid) &&
 			    !loosened_object_can_be_discarded(&oid, p->mtime))
 				if (force_object_loose(&oid, p->mtime))
diff --git a/pack-bitmap-write.c b/pack-bitmap-write.c
index fa78a460c9..a7a4964b50 100644
--- a/pack-bitmap-write.c
+++ b/pack-bitmap-write.c
@@ -144,7 +144,7 @@ static inline void reset_all_seen(void)
 
 static uint32_t find_object_pos(const struct object_id *oid)
 {
-	struct object_entry *entry = packlist_find(writer.to_pack, oid, NULL);
+	struct object_entry *entry = packlist_find(writer.to_pack, oid);
 
 	if (!entry) {
 		die("Failed to write bitmap index. Packfile doesn't have full closure "
diff --git a/pack-bitmap.c b/pack-bitmap.c
index ed2befaac6..84cd1bed4a 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -1063,7 +1063,7 @@ int rebuild_existing_bitmaps(struct bitmap_index *bitmap_git,
 
 		entry = &bitmap_git->pack->revindex[i];
 		nth_packed_object_oid(&oid, bitmap_git->pack, entry->nr);
-		oe = packlist_find(mapping, &oid, NULL);
+		oe = packlist_find(mapping, &oid);
 
 		if (oe)
 			reposition[i] = oe_in_pack_pos(mapping, oe) + 1;
diff --git a/pack-objects.c b/pack-objects.c
index c1df08df1a..3624090593 100644
--- a/pack-objects.c
+++ b/pack-objects.c
@@ -68,8 +68,7 @@ static void rehash_objects(struct packing_data *pdata)
 }
 
 struct object_entry *packlist_find(struct packing_data *pdata,
-				   const struct object_id *oid,
-				   uint32_t *index_pos)
+				   const struct object_id *oid)
 {
 	uint32_t i;
 	int found;
@@ -79,9 +78,6 @@ struct object_entry *packlist_find(struct packing_data *pdata,
 
 	i = locate_object_entry_hash(pdata, oid, &found);
 
-	if (index_pos)
-		*index_pos = i;
-
 	if (!found)
 		return NULL;
 
@@ -153,8 +149,7 @@ void prepare_packing_data(struct repository *r, struct packing_data *pdata)
 }
 
 struct object_entry *packlist_alloc(struct packing_data *pdata,
-				    const struct object_id *oid,
-				    uint32_t index_pos)
+				    const struct object_id *oid)
 {
 	struct object_entry *new_entry;
 
@@ -181,8 +176,13 @@ struct object_entry *packlist_alloc(struct packing_data *pdata,
 
 	if (pdata->index_size * 3 <= pdata->nr_objects * 4)
 		rehash_objects(pdata);
-	else
-		pdata->index[index_pos] = pdata->nr_objects;
+	else {
+		int found;
+		uint32_t pos = locate_object_entry_hash(pdata,
+							&new_entry->idx.oid,
+							&found);
+		pdata->index[pos] = pdata->nr_objects;
+	}
 
 	if (pdata->in_pack)
 		pdata->in_pack[pdata->nr_objects - 1] = NULL;
diff --git a/pack-objects.h b/pack-objects.h
index 47bf7ebf86..6fe6ae5ee8 100644
--- a/pack-objects.h
+++ b/pack-objects.h
@@ -183,12 +183,10 @@ static inline void packing_data_unlock(struct packing_data *pdata)
 }
 
 struct object_entry *packlist_alloc(struct packing_data *pdata,
-				    const struct object_id *oid,
-				    uint32_t index_pos);
+				    const struct object_id *oid);
 
 struct object_entry *packlist_find(struct packing_data *pdata,
-				   const struct object_id *oid,
-				   uint32_t *index_pos);
+				   const struct object_id *oid);
 
 static inline uint32_t pack_name_hash(const char *name)
 {
-- 
2.23.0.463.g883b23b1c5