@matheusbsbs / matheus-bagliano / commits / 63f4a7fc01
Fork 0

pack-check: push oid lookup into loop

When we're checking a pack with fsck or verify-pack, we first sort the idx entries by offset, since accessing them in pack order is more efficient. To do so, we loop over them and fill in an array of structs with the offset, object_id, and index position of each, sort the result, and only then do we iterate over the sorted array and process each entry. In order to avoid the memory cost of storing the hash of each object, we just store a pointer into the copy in the mmap'd pack index file. To keep that property even as the rest of the code converted to "struct object_id", commit 9fd750461b (Convert the verify_pack callback to struct object_id, 2017-05-06) introduced a union in order to type-pun the pointer-to-hash into an object_id struct. But we can make this even simpler by observing that the sort operation doesn't need the object id at all! We only need them one at a time while we actually process each entry. So we can just omit the oid from the struct entirely and load it on the fly into a local variable in the second loop. This gets rid of the type-punning, and lets us directly use the more type-safe nth_packed_object_id(), simplifying the code. And as a bonus, it saves 8 bytes of memory per object. Note that this does mean we'll do the offset lookup for each object before the oid lookup. The oid lookup has more safety checks in it (e.g., for looking past p->num_objects) which in theory protected the offset lookup. But since violating those checks was already a BUG() condition (as described in the previous commit), it's not worth worrying about. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Jeff King committed Feb 23, 2020 at 23:36 UTC 63f4a7fc0107ec240f48605a4d4f8e41b91caa41
1 file changed +10 -13
pack-check.c
+10 -13
@@ -8,10 +8,6 @@
8
9 struct idx_entry {
10 off_t offset;
11 - union idx_entry_object {
12 - const unsigned char *hash;
13 - struct object_id *oid;
14 - } oid;
11 unsigned int nr;
12 };
13
@@ -97,10 +93,6 @@ static int verify_packfile(struct repository *r,
93 entries[nr_objects].offset = pack_sig_ofs;
94 /* first sort entries by pack offset, since unpacking them is more efficient that way */
95 for (i = 0; i < nr_objects; i++) {
100 - entries[i].oid.hash = nth_packed_object_sha1(p, i);
101 - if (!entries[i].oid.hash)
102 - BUG("unable to get oid of object %lu from %s",
103 - (unsigned long)i, p->pack_name);
96 entries[i].offset = nth_packed_object_offset(p, i);
97 entries[i].nr = i;
98 }
@@ -108,11 +100,16 @@ static int verify_packfile(struct repository *r,
100
101 for (i = 0; i < nr_objects; i++) {
102 void *data;
103 + struct object_id oid;
104 enum object_type type;
105 unsigned long size;
106 off_t curpos;
107 int data_valid;
108
109 + if (nth_packed_object_id(&oid, p, entries[i].nr) < 0)
110 + BUG("unable to get oid of object %lu from %s",
111 + (unsigned long)entries[i].nr, p->pack_name);
112 +
113 if (p->index_version > 1) {
114 off_t offset = entries[i].offset;
115 off_t len = entries[i+1].offset - offset;
@@ -120,7 +117,7 @@ static int verify_packfile(struct repository *r,
117 if (check_pack_crc(p, w_curs, offset, len, nr))
118 err = error("index CRC mismatch for object %s "
119 "from %s at offset %"PRIuMAX"",
123 - oid_to_hex(entries[i].oid.oid),
120 + oid_to_hex(&oid),
121 p->pack_name, (uintmax_t)offset);
122 }
123
@@ -143,14 +140,14 @@ static int verify_packfile(struct repository *r,
140
141 if (data_valid && !data)
142 err = error("cannot unpack %s from %s at offset %"PRIuMAX"",
146 - oid_to_hex(entries[i].oid.oid), p->pack_name,
143 + oid_to_hex(&oid), p->pack_name,
144 (uintmax_t)entries[i].offset);
148 - else if (check_object_signature(r, entries[i].oid.oid, data, size, type_name(type)))
145 + else if (check_object_signature(r, &oid, data, size, type_name(type)))
146 err = error("packed %s from %s is corrupt",
150 - oid_to_hex(entries[i].oid.oid), p->pack_name);
147 + oid_to_hex(&oid), p->pack_name);
148 else if (fn) {
149 int eaten = 0;
153 - err |= fn(entries[i].oid.oid, type, size, data, &eaten);
150 + err |= fn(&oid, type, size, data, &eaten);
151 if (eaten)
152 data = NULL;
153 }