@matheusbsbs / matheus-bagliano / commits / 3a37876b5d

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 <s-beyer@gmx.net> Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Jeff King committed Sep 5, 2019 at 21:36 UTC 3a37876b5dca4c18bda67bcdead9c1d79a59933d

5 files changed +29 -34

builtin/pack-objects.c

+14 -19

@@ -610,12 +610,12 @@ static int mark_tagged(const char *path, const struct object_id *oid, int flag,

610 void *cb_data)

611 {

612 struct object_id peeled;

613 - struct object_entry *entry = packlist_find(&to_pack, oid, NULL);

613 + struct object_entry *entry = packlist_find(&to_pack, oid);

614

615 if (entry)

616 entry->tagged = 1;

617 if (!peel_ref(path, &peeled)) {

618 - entry = packlist_find(&to_pack, &peeled, NULL);

618 + entry = packlist_find(&to_pack, &peeled);

619 if (entry)

620 entry->tagged = 1;

621 }

@@ -996,12 +996,11 @@ static int no_try_delta(const char *path)

996 * few lines later when we want to add the new entry.

997 */

998 static int have_duplicate_entry(const struct object_id *oid,

999 - int exclude,

1000 - uint32_t *index_pos)

999 + int exclude)

1000 {

1001 struct object_entry *entry;

1002

1004 - entry = packlist_find(&to_pack, oid, index_pos);

1003 + entry = packlist_find(&to_pack, oid);

1004 if (!entry)

1005 return 0;

1006

@@ -1141,13 +1140,12 @@ static void create_object_entry(const struct object_id *oid,

1140 uint32_t hash,

1141 int exclude,

1142 int no_try_delta,

1144 - uint32_t index_pos,

1143 struct packed_git *found_pack,

1144 off_t found_offset)

1145 {

1146 struct object_entry *entry;

1147

1150 - entry = packlist_alloc(&to_pack, oid, index_pos);

1148 + entry = packlist_alloc(&to_pack, oid);

1149 entry->hash = hash;

1150 oe_set_type(entry, type);

1151 if (exclude)

@@ -1171,11 +1169,10 @@ static int add_object_entry(const struct object_id *oid, enum object_type type,

1169 {

1170 struct packed_git *found_pack = NULL;

1171 off_t found_offset = 0;

1174 - uint32_t index_pos;

1172

1173 display_progress(progress_state, ++nr_seen);

1174

1178 - if (have_duplicate_entry(oid, exclude, &index_pos))

1175 + if (have_duplicate_entry(oid, exclude))

1176 return 0;

1177

1178 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,

1187

1188 create_object_entry(oid, type, pack_name_hash(name),

1189 exclude, name && no_try_delta(name),

1193 - index_pos, found_pack, found_offset);

1190 + found_pack, found_offset);

1191 return 1;

1192 }

1193

@@ -1199,17 +1196,15 @@ static int add_object_entry_from_bitmap(const struct object_id *oid,

1196 int flags, uint32_t name_hash,

1197 struct packed_git *pack, off_t offset)

1198 {

1202 - uint32_t index_pos;

1203 -

1199 display_progress(progress_state, ++nr_seen);

1200

1206 - if (have_duplicate_entry(oid, 0, &index_pos))

1201 + if (have_duplicate_entry(oid, 0))

1202 return 0;

1203

1204 if (!want_object_in_pack(oid, 0, &pack, &offset))

1205 return 0;

1206

1212 - create_object_entry(oid, type, name_hash, 0, 0, index_pos, pack, offset);

1207 + create_object_entry(oid, type, name_hash, 0, 0, pack, offset);

1208 return 1;

1209 }

1210

@@ -1507,7 +1502,7 @@ static int can_reuse_delta(const unsigned char *base_sha1,

1502 * First see if we're already sending the base (or it's explicitly in

1503 * our "excluded" list).

1504 */

1510 - base = packlist_find(&to_pack, &base_oid, NULL);

1505 + base = packlist_find(&to_pack, &base_oid);

1506 if (base) {

1507 if (!in_same_island(&delta->idx.oid, &base->idx.oid))

1508 return 0;

@@ -2579,7 +2574,7 @@ static void add_tag_chain(const struct object_id *oid)

2574 * it was included via bitmaps, we would not have parsed it

2575 * previously).

2576 */

2582 - if (packlist_find(&to_pack, oid, NULL))

2577 + if (packlist_find(&to_pack, oid))

2578 return;

2579

2580 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,

2598

2599 if (starts_with(path, "refs/tags/") && /* is a tag? */

2600 !peel_ref(path, &peeled) && /* peelable? */

2606 - packlist_find(&to_pack, &peeled, NULL)) /* object packed? */

2601 + packlist_find(&to_pack, &peeled)) /* object packed? */

2602 add_tag_chain(oid);

2603 return 0;

2604 }

@@ -2803,7 +2798,7 @@ static void show_object(struct object *obj, const char *name, void *data)

2798 for (p = strchr(name, '/'); p; p = strchr(p + 1, '/'))

2799 depth++;

2800

2806 - ent = packlist_find(&to_pack, &obj->oid, NULL);

2801 + ent = packlist_find(&to_pack, &obj->oid);

2802 if (ent && depth > oe_tree_depth(&to_pack, ent))

2803 oe_set_tree_depth(&to_pack, ent, depth);

2804 }

@@ -3034,7 +3029,7 @@ static void loosen_unused_packed_objects(void)

3029

3030 for (i = 0; i < p->num_objects; i++) {

3031 nth_packed_object_oid(&oid, p, i);

3037 - if (!packlist_find(&to_pack, &oid, NULL) &&

3032 + if (!packlist_find(&to_pack, &oid) &&

3033 !has_sha1_pack_kept_or_nonlocal(&oid) &&

3034 !loosened_object_can_be_discarded(&oid, p->mtime))

3035 if (force_object_loose(&oid, p->mtime))

pack-bitmap-write.c

+1 -1

@@ -144,7 +144,7 @@ static inline void reset_all_seen(void)

144

145 static uint32_t find_object_pos(const struct object_id *oid)

146 {

147 - struct object_entry *entry = packlist_find(writer.to_pack, oid, NULL);

147 + struct object_entry *entry = packlist_find(writer.to_pack, oid);

148

149 if (!entry) {

150 die("Failed to write bitmap index. Packfile doesn't have full closure "

pack-bitmap.c

+1 -1

@@ -1063,7 +1063,7 @@ int rebuild_existing_bitmaps(struct bitmap_index *bitmap_git,

1063

1064 entry = &bitmap_git->pack->revindex[i];

1065 nth_packed_object_oid(&oid, bitmap_git->pack, entry->nr);

1066 - oe = packlist_find(mapping, &oid, NULL);

1066 + oe = packlist_find(mapping, &oid);

1067

1068 if (oe)

1069 reposition[i] = oe_in_pack_pos(mapping, oe) + 1;

pack-objects.c

+11 -9

@@ -68,8 +68,7 @@ static void rehash_objects(struct packing_data *pdata)

68 }

70 struct object_entry *packlist_find(struct packing_data *pdata,

71 - const struct object_id *oid,

72 - uint32_t *index_pos)

71 + const struct object_id *oid)

72 {

73 uint32_t i;

74 int found;

@@ -79,9 +78,6 @@ struct object_entry *packlist_find(struct packing_data *pdata,

79 i = locate_object_entry_hash(pdata, oid, &found);

82 - if (index_pos)

83 - *index_pos = i;

84 -

81 if (!found)

82 return NULL;

@@ -153,8 +149,7 @@ void prepare_packing_data(struct repository *r, struct packing_data *pdata)

149 }

150

151 struct object_entry *packlist_alloc(struct packing_data *pdata,

156 - const struct object_id *oid,

157 - uint32_t index_pos)

152 + const struct object_id *oid)

153 {

154 struct object_entry *new_entry;

155

@@ -181,8 +176,15 @@ struct object_entry *packlist_alloc(struct packing_data *pdata,

176

177 if (pdata->index_size * 3 <= pdata->nr_objects * 4)

178 rehash_objects(pdata);

184 - else

185 - pdata->index[index_pos] = pdata->nr_objects;

179 + else {

180 + int found;

181 + uint32_t pos = locate_object_entry_hash(pdata,

182 + &new_entry->idx.oid,

183 + &found);

184 + if (found)

185 + BUG("duplicate object inserted into hash");

186 + pdata->index[pos] = pdata->nr_objects;

187 + }

188

189 if (pdata->in_pack)

190 pdata->in_pack[pdata->nr_objects - 1] = NULL;

pack-objects.h

+2 -4

@@ -183,12 +183,10 @@ static inline void packing_data_unlock(struct packing_data *pdata)

183 }

184

185 struct object_entry *packlist_alloc(struct packing_data *pdata,

186 - const struct object_id *oid,

187 - uint32_t index_pos);

186 + const struct object_id *oid);

187

188 struct object_entry *packlist_find(struct packing_data *pdata,

190 - const struct object_id *oid,

191 - uint32_t *index_pos);

189 + const struct object_id *oid);

190

191 static inline uint32_t pack_name_hash(const char *name)

192 {