@matheusbsbs / matheus-bagliano / commits / 7b8abc4d8c

reftable/record: use scratch buffer when decoding records

When decoding log records we need a temporary buffer to decode the reflog entry's name, mail address and message. As this buffer is local to the function we thus have to reallocate it for every single log record which we're about to decode, which is inefficient. Refactor the code such that callers need to pass in a scratch buffer, which allows us to reuse it for multiple decodes. This reduces the number of allocations when iterating through reflogs. Before: HEAP SUMMARY: in use at exit: 13,473 bytes in 122 blocks total heap usage: 2,068,487 allocs, 2,068,365 frees, 305,122,946 bytes allocated After: HEAP SUMMARY: in use at exit: 13,473 bytes in 122 blocks total heap usage: 1,068,485 allocs, 1,068,363 frees, 281,122,886 bytes allocated Note that this commit also drop some redundant calls to `strbuf_reset()` right before calling `decode_string()`. The latter already knows to reset the buffer, so there is no need for these. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Patrick Steinhardt committed Mar 5, 2024 at 13:11 UTC 7b8abc4d8cd428e4ce044e50f7980baacaccb761

5 files changed +68 -52

reftable/block.c

+3 -1

@@ -332,7 +332,8 @@ int block_iter_next(struct block_iter *it, struct reftable_record *rec)

332 return REFTABLE_FORMAT_ERROR;

333

334 string_view_consume(&in, n);

335 - n = reftable_record_decode(rec, it->last_key, extra, in, it->br->hash_size);

335 + n = reftable_record_decode(rec, it->last_key, extra, in, it->br->hash_size,

336 + &it->scratch);

337 if (n < 0)

338 return -1;

339 string_view_consume(&in, n);

@@ -369,6 +370,7 @@ int block_iter_seek(struct block_iter *it, struct strbuf *want)

370 void block_iter_close(struct block_iter *it)

371 {

372 strbuf_release(&it->last_key);

373 + strbuf_release(&it->scratch);

374 }

375

376 int block_reader_seek(struct block_reader *br, struct block_iter *it,

reftable/block.h

@@ -84,10 +84,12 @@ struct block_iter {

85 /* key for last entry we read. */

86 struct strbuf last_key;

87 + struct strbuf scratch;

88 };

90 #define BLOCK_ITER_INIT { \

91 .last_key = STRBUF_INIT, \

92 + .scratch = STRBUF_INIT, \

93 }

95 /* initializes a block reader. */

reftable/record.c

+24 -28

@@ -374,7 +374,7 @@ static int reftable_ref_record_encode(const void *rec, struct string_view s,

374

375 static int reftable_ref_record_decode(void *rec, struct strbuf key,

376 uint8_t val_type, struct string_view in,

377 - int hash_size)

377 + int hash_size, struct strbuf *scratch)

378 {

379 struct reftable_ref_record *r = rec;

380 struct string_view start = in;

@@ -425,13 +425,12 @@ static int reftable_ref_record_decode(void *rec, struct strbuf key,

425 break;

426

427 case REFTABLE_REF_SYMREF: {

428 - struct strbuf dest = STRBUF_INIT;

429 - int n = decode_string(&dest, in);

428 + int n = decode_string(scratch, in);

429 if (n < 0) {

430 return -1;

431 }

432 string_view_consume(&in, n);

434 - r->value.symref = dest.buf;

433 + r->value.symref = strbuf_detach(scratch, NULL);

434 } break;

435

436 case REFTABLE_REF_DELETION:

@@ -579,7 +578,7 @@ static int reftable_obj_record_encode(const void *rec, struct string_view s,

578

579 static int reftable_obj_record_decode(void *rec, struct strbuf key,

580 uint8_t val_type, struct string_view in,

582 - int hash_size)

581 + int hash_size, struct strbuf *scratch UNUSED)

582 {

583 struct string_view start = in;

584 struct reftable_obj_record *r = rec;

@@ -849,13 +848,12 @@ static int reftable_log_record_encode(const void *rec, struct string_view s,

848

849 static int reftable_log_record_decode(void *rec, struct strbuf key,

850 uint8_t val_type, struct string_view in,

852 - int hash_size)

851 + int hash_size, struct strbuf *scratch)

852 {

853 struct string_view start = in;

854 struct reftable_log_record *r = rec;

855 uint64_t max = 0;

856 uint64_t ts = 0;

858 - struct strbuf dest = STRBUF_INIT;

857 int n;

858

859 if (key.len <= 9 || key.buf[key.len - 9] != 0)

@@ -892,7 +890,7 @@ static int reftable_log_record_decode(void *rec, struct strbuf key,

890

891 string_view_consume(&in, 2 * hash_size);

892

895 - n = decode_string(&dest, in);

893 + n = decode_string(scratch, in);

894 if (n < 0)

895 goto done;

896 string_view_consume(&in, n);

@@ -904,26 +902,25 @@ static int reftable_log_record_decode(void *rec, struct strbuf key,

902 * skip copying over the name in case it's accurate already.

903 */

904 if (!r->value.update.name ||

907 - strcmp(r->value.update.name, dest.buf)) {

905 + strcmp(r->value.update.name, scratch->buf)) {

906 r->value.update.name =

909 - reftable_realloc(r->value.update.name, dest.len + 1);

910 - memcpy(r->value.update.name, dest.buf, dest.len);

911 - r->value.update.name[dest.len] = 0;

907 + reftable_realloc(r->value.update.name, scratch->len + 1);

908 + memcpy(r->value.update.name, scratch->buf, scratch->len);

909 + r->value.update.name[scratch->len] = 0;

910 }

911

914 - strbuf_reset(&dest);

915 - n = decode_string(&dest, in);

912 + n = decode_string(scratch, in);

913 if (n < 0)

914 goto done;

915 string_view_consume(&in, n);

916

917 /* Same as above, but for the reflog email. */

918 if (!r->value.update.email ||

922 - strcmp(r->value.update.email, dest.buf)) {

919 + strcmp(r->value.update.email, scratch->buf)) {

920 r->value.update.email =

924 - reftable_realloc(r->value.update.email, dest.len + 1);

925 - memcpy(r->value.update.email, dest.buf, dest.len);

926 - r->value.update.email[dest.len] = 0;

921 + reftable_realloc(r->value.update.email, scratch->len + 1);

922 + memcpy(r->value.update.email, scratch->buf, scratch->len);

923 + r->value.update.email[scratch->len] = 0;

924 }

925

926 ts = 0;

@@ -938,22 +935,19 @@ static int reftable_log_record_decode(void *rec, struct strbuf key,

935 r->value.update.tz_offset = get_be16(in.buf);

936 string_view_consume(&in, 2);

937

941 - strbuf_reset(&dest);

942 - n = decode_string(&dest, in);

938 + n = decode_string(scratch, in);

939 if (n < 0)

940 goto done;

941 string_view_consume(&in, n);

942

947 - REFTABLE_ALLOC_GROW(r->value.update.message, dest.len + 1,

943 + REFTABLE_ALLOC_GROW(r->value.update.message, scratch->len + 1,

944 r->value.update.message_cap);

949 - memcpy(r->value.update.message, dest.buf, dest.len);

950 - r->value.update.message[dest.len] = 0;

945 + memcpy(r->value.update.message, scratch->buf, scratch->len);

946 + r->value.update.message[scratch->len] = 0;

947

952 - strbuf_release(&dest);

948 return start.len - in.len;

949

950 done:

956 - strbuf_release(&dest);

951 return REFTABLE_FORMAT_ERROR;

952 }

953

@@ -1093,7 +1087,7 @@ static int reftable_index_record_encode(const void *rec, struct string_view out,

1087

1088 static int reftable_index_record_decode(void *rec, struct strbuf key,

1089 uint8_t val_type, struct string_view in,

1096 - int hash_size)

1090 + int hash_size, struct strbuf *scratch UNUSED)

1091 {

1092 struct string_view start = in;

1093 struct reftable_index_record *r = rec;

@@ -1174,10 +1168,12 @@ uint8_t reftable_record_val_type(struct reftable_record *rec)

1168 }

1169

1170 int reftable_record_decode(struct reftable_record *rec, struct strbuf key,

1177 - uint8_t extra, struct string_view src, int hash_size)

1171 + uint8_t extra, struct string_view src, int hash_size,

1172 + struct strbuf *scratch)

1173 {

1174 return reftable_record_vtable(rec)->decode(reftable_record_data(rec),

1180 - key, extra, src, hash_size);

1175 + key, extra, src, hash_size,

1176 + scratch);

1177 }

1178

1179 void reftable_record_release(struct reftable_record *rec)

reftable/record.h

+3 -2

@@ -55,7 +55,8 @@ struct reftable_record_vtable {

56 /* decode data from `src` into the record. */

57 int (*decode)(void *rec, struct strbuf key, uint8_t extra,

58 - struct string_view src, int hash_size);

58 + struct string_view src, int hash_size,

59 + struct strbuf *scratch);

61 /* deallocate and null the record. */

62 void (*release)(void *rec);

@@ -138,7 +139,7 @@ int reftable_record_encode(struct reftable_record *rec, struct string_view dest,

139 int hash_size);

140 int reftable_record_decode(struct reftable_record *rec, struct strbuf key,

141 uint8_t extra, struct string_view src,

141 - int hash_size);

142 + int hash_size, struct strbuf *scratch);

143 int reftable_record_is_deletion(struct reftable_record *rec);

144

145 static inline uint8_t reftable_record_type(struct reftable_record *rec)

reftable/record_test.c

+36 -21

@@ -99,6 +99,7 @@ static void set_hash(uint8_t *h, int j)

100 static void test_reftable_ref_record_roundtrip(void)

101 {

102 + struct strbuf scratch = STRBUF_INIT;

103 int i = 0;

104

105 for (i = REFTABLE_REF_DELETION; i < REFTABLE_NR_REF_VALUETYPES; i++) {

@@ -140,7 +141,7 @@ static void test_reftable_ref_record_roundtrip(void)

141 EXPECT(n > 0);

142

143 /* decode into a non-zero reftable_record to test for leaks. */

143 - m = reftable_record_decode(&out, key, i, dest, GIT_SHA1_RAWSZ);

144 + m = reftable_record_decode(&out, key, i, dest, GIT_SHA1_RAWSZ, &scratch);

145 EXPECT(n == m);

146

147 EXPECT(reftable_ref_record_equal(&in.u.ref, &out.u.ref,

@@ -150,6 +151,8 @@ static void test_reftable_ref_record_roundtrip(void)

151 strbuf_release(&key);

152 reftable_record_release(&out);

153 }

154 +

155 + strbuf_release(&scratch);

156 }

157

158 static void test_reftable_log_record_equal(void)

@@ -175,7 +178,6 @@ static void test_reftable_log_record_equal(void)

178 static void test_reftable_log_record_roundtrip(void)

179 {

180 int i;

178 -

181 struct reftable_log_record in[] = {

182 {

183 .refname = xstrdup("refs/heads/master"),

@@ -202,6 +204,8 @@ static void test_reftable_log_record_roundtrip(void)

204 .value_type = REFTABLE_LOG_UPDATE,

205 }

206 };

207 + struct strbuf scratch = STRBUF_INIT;

208 +

209 set_test_hash(in[0].value.update.new_hash, 1);

210 set_test_hash(in[0].value.update.old_hash, 2);

211 set_test_hash(in[2].value.update.new_hash, 3);

@@ -241,7 +245,7 @@ static void test_reftable_log_record_roundtrip(void)

245 EXPECT(n >= 0);

246 valtype = reftable_record_val_type(&rec);

247 m = reftable_record_decode(&out, key, valtype, dest,

244 - GIT_SHA1_RAWSZ);

248 + GIT_SHA1_RAWSZ, &scratch);

249 EXPECT(n == m);

250

251 EXPECT(reftable_log_record_equal(&in[i], &out.u.log,

@@ -250,6 +254,8 @@ static void test_reftable_log_record_roundtrip(void)

254 strbuf_release(&key);

255 reftable_record_release(&out);

256 }

257 +

258 + strbuf_release(&scratch);

259 }

260

261 static void test_u24_roundtrip(void)

@@ -299,23 +305,27 @@ static void test_reftable_obj_record_roundtrip(void)

305 {

306 uint8_t testHash1[GIT_SHA1_RAWSZ] = { 1, 2, 3, 4, 0 };

307 uint64_t till9[] = { 1, 2, 3, 4, 500, 600, 700, 800, 9000 };

302 - struct reftable_obj_record recs[3] = { {

303 - .hash_prefix = testHash1,

304 - .hash_prefix_len = 5,

305 - .offsets = till9,

306 - .offset_len = 3,

307 - },

308 - {

309 - .hash_prefix = testHash1,

310 - .hash_prefix_len = 5,

311 - .offsets = till9,

312 - .offset_len = 9,

313 - },

314 - {

315 - .hash_prefix = testHash1,

316 - .hash_prefix_len = 5,

317 - } };

308 + struct reftable_obj_record recs[3] = {

309 + {

310 + .hash_prefix = testHash1,

311 + .hash_prefix_len = 5,

312 + .offsets = till9,

313 + .offset_len = 3,

314 + },

315 + {

316 + .hash_prefix = testHash1,

317 + .hash_prefix_len = 5,

318 + .offsets = till9,

319 + .offset_len = 9,

320 + },

321 + {

322 + .hash_prefix = testHash1,

323 + .hash_prefix_len = 5,

324 + },

325 + };

326 + struct strbuf scratch = STRBUF_INIT;

327 int i = 0;

328 +

329 for (i = 0; i < ARRAY_SIZE(recs); i++) {

330 uint8_t buffer[1024] = { 0 };

331 struct string_view dest = {

@@ -339,13 +349,15 @@ static void test_reftable_obj_record_roundtrip(void)

349 EXPECT(n > 0);

350 extra = reftable_record_val_type(&in);

351 m = reftable_record_decode(&out, key, extra, dest,

342 - GIT_SHA1_RAWSZ);

352 + GIT_SHA1_RAWSZ, &scratch);

353 EXPECT(n == m);

354

355 EXPECT(reftable_record_equal(&in, &out, GIT_SHA1_RAWSZ));

356 strbuf_release(&key);

357 reftable_record_release(&out);

358 }

359 +

360 + strbuf_release(&scratch);

361 }

362

363 static void test_reftable_index_record_roundtrip(void)

@@ -362,6 +374,7 @@ static void test_reftable_index_record_roundtrip(void)

374 .buf = buffer,

375 .len = sizeof(buffer),

376 };

377 + struct strbuf scratch = STRBUF_INIT;

378 struct strbuf key = STRBUF_INIT;

379 struct reftable_record out = {

380 .type = BLOCK_TYPE_INDEX,

@@ -379,13 +392,15 @@ static void test_reftable_index_record_roundtrip(void)

392 EXPECT(n > 0);

393

394 extra = reftable_record_val_type(&in);

382 - m = reftable_record_decode(&out, key, extra, dest, GIT_SHA1_RAWSZ);

395 + m = reftable_record_decode(&out, key, extra, dest, GIT_SHA1_RAWSZ,

396 + &scratch);

397 EXPECT(m == n);

398

399 EXPECT(reftable_record_equal(&in, &out, GIT_SHA1_RAWSZ));

400

401 reftable_record_release(&out);

402 strbuf_release(&key);

403 + strbuf_release(&scratch);

404 strbuf_release(&in.u.idx.last_key);

405 }

406