From f1cbd033e201a18c7175bc6509b48d6243e79739 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 5 Sep 2019 18:52:25 -0400 Subject: pack-objects: use object_id in packlist_alloc() The only caller of packlist_alloc() already has a "struct object_id", and we immediately copy the hash they pass us into our own object_id. Let's avoid the unnecessary round-trip to a raw sha1 pointer. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- pack-objects.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'pack-objects.h') diff --git a/pack-objects.h b/pack-objects.h index 857d43850b..47bf7ebf86 100644 --- a/pack-objects.h +++ b/pack-objects.h @@ -183,7 +183,7 @@ static inline void packing_data_unlock(struct packing_data *pdata) } struct object_entry *packlist_alloc(struct packing_data *pdata, - const unsigned char *sha1, + const struct object_id *oid, uint32_t index_pos); struct object_entry *packlist_find(struct packing_data *pdata, -- cgit v1.2.3 From 3a37876b5dca4c18bda67bcdead9c1d79a59933d Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 5 Sep 2019 21:36:05 -0400 Subject: pack-objects: drop packlist index_pos optimization 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 it at all, since it will have to create/grow the hash table. 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 Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- pack-objects.h | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) (limited to 'pack-objects.h') 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) { -- cgit v1.2.3