Skip to content

Commit 7b8abc4

Browse files
pks-tgitster
pks-t
authored and
gitster
committed
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 <[email protected]> Signed-off-by: Junio C Hamano <[email protected]>
1 parent e0bd13b commit 7b8abc4

File tree

5 files changed

+68
-52
lines changed

5 files changed

+68
-52
lines changed

‎reftable/block.c

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -332,7 +332,8 @@ int block_iter_next(struct block_iter *it, struct reftable_record *rec)
332332
return REFTABLE_FORMAT_ERROR;
333333

334334
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);
336337
if (n < 0)
337338
return -1;
338339
string_view_consume(&in, n);
@@ -369,6 +370,7 @@ int block_iter_seek(struct block_iter *it, struct strbuf *want)
369370
void block_iter_close(struct block_iter *it)
370371
{
371372
strbuf_release(&it->last_key);
373+
strbuf_release(&it->scratch);
372374
}
373375

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

‎reftable/block.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,10 +84,12 @@ struct block_iter {
8484

8585
/* key for last entry we read. */
8686
struct strbuf last_key;
87+
struct strbuf scratch;
8788
};
8889

8990
#define BLOCK_ITER_INIT { \
9091
.last_key = STRBUF_INIT, \
92+
.scratch = STRBUF_INIT, \
9193
}
9294

9395
/* initializes a block reader. */

‎reftable/record.c

Lines changed: 24 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -374,7 +374,7 @@ static int reftable_ref_record_encode(const void *rec, struct string_view s,
374374

375375
static int reftable_ref_record_decode(void *rec, struct strbuf key,
376376
uint8_t val_type, struct string_view in,
377-
int hash_size)
377+
int hash_size, struct strbuf *scratch)
378378
{
379379
struct reftable_ref_record *r = rec;
380380
struct string_view start = in;
@@ -425,13 +425,12 @@ static int reftable_ref_record_decode(void *rec, struct strbuf key,
425425
break;
426426

427427
case REFTABLE_REF_SYMREF: {
428-
struct strbuf dest = STRBUF_INIT;
429-
int n = decode_string(&dest, in);
428+
int n = decode_string(scratch, in);
430429
if (n < 0) {
431430
return -1;
432431
}
433432
string_view_consume(&in, n);
434-
r->value.symref = dest.buf;
433+
r->value.symref = strbuf_detach(scratch, NULL);
435434
} break;
436435

437436
case REFTABLE_REF_DELETION:
@@ -579,7 +578,7 @@ static int reftable_obj_record_encode(const void *rec, struct string_view s,
579578

580579
static int reftable_obj_record_decode(void *rec, struct strbuf key,
581580
uint8_t val_type, struct string_view in,
582-
int hash_size)
581+
int hash_size, struct strbuf *scratch UNUSED)
583582
{
584583
struct string_view start = in;
585584
struct reftable_obj_record *r = rec;
@@ -849,13 +848,12 @@ static int reftable_log_record_encode(const void *rec, struct string_view s,
849848

850849
static int reftable_log_record_decode(void *rec, struct strbuf key,
851850
uint8_t val_type, struct string_view in,
852-
int hash_size)
851+
int hash_size, struct strbuf *scratch)
853852
{
854853
struct string_view start = in;
855854
struct reftable_log_record *r = rec;
856855
uint64_t max = 0;
857856
uint64_t ts = 0;
858-
struct strbuf dest = STRBUF_INIT;
859857
int n;
860858

861859
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,
892890

893891
string_view_consume(&in, 2 * hash_size);
894892

895-
n = decode_string(&dest, in);
893+
n = decode_string(scratch, in);
896894
if (n < 0)
897895
goto done;
898896
string_view_consume(&in, n);
@@ -904,26 +902,25 @@ static int reftable_log_record_decode(void *rec, struct strbuf key,
904902
* skip copying over the name in case it's accurate already.
905903
*/
906904
if (!r->value.update.name ||
907-
strcmp(r->value.update.name, dest.buf)) {
905+
strcmp(r->value.update.name, scratch->buf)) {
908906
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;
912910
}
913911

914-
strbuf_reset(&dest);
915-
n = decode_string(&dest, in);
912+
n = decode_string(scratch, in);
916913
if (n < 0)
917914
goto done;
918915
string_view_consume(&in, n);
919916

920917
/* Same as above, but for the reflog email. */
921918
if (!r->value.update.email ||
922-
strcmp(r->value.update.email, dest.buf)) {
919+
strcmp(r->value.update.email, scratch->buf)) {
923920
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;
927924
}
928925

929926
ts = 0;
@@ -938,22 +935,19 @@ static int reftable_log_record_decode(void *rec, struct strbuf key,
938935
r->value.update.tz_offset = get_be16(in.buf);
939936
string_view_consume(&in, 2);
940937

941-
strbuf_reset(&dest);
942-
n = decode_string(&dest, in);
938+
n = decode_string(scratch, in);
943939
if (n < 0)
944940
goto done;
945941
string_view_consume(&in, n);
946942

947-
REFTABLE_ALLOC_GROW(r->value.update.message, dest.len + 1,
943+
REFTABLE_ALLOC_GROW(r->value.update.message, scratch->len + 1,
948944
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;
951947

952-
strbuf_release(&dest);
953948
return start.len - in.len;
954949

955950
done:
956-
strbuf_release(&dest);
957951
return REFTABLE_FORMAT_ERROR;
958952
}
959953

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

10941088
static int reftable_index_record_decode(void *rec, struct strbuf key,
10951089
uint8_t val_type, struct string_view in,
1096-
int hash_size)
1090+
int hash_size, struct strbuf *scratch UNUSED)
10971091
{
10981092
struct string_view start = in;
10991093
struct reftable_index_record *r = rec;
@@ -1174,10 +1168,12 @@ uint8_t reftable_record_val_type(struct reftable_record *rec)
11741168
}
11751169

11761170
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)
11781173
{
11791174
return reftable_record_vtable(rec)->decode(reftable_record_data(rec),
1180-
key, extra, src, hash_size);
1175+
key, extra, src, hash_size,
1176+
scratch);
11811177
}
11821178

11831179
void reftable_record_release(struct reftable_record *rec)

‎reftable/record.h

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,8 @@ struct reftable_record_vtable {
5555

5656
/* decode data from `src` into the record. */
5757
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);
5960

6061
/* deallocate and null the record. */
6162
void (*release)(void *rec);
@@ -138,7 +139,7 @@ int reftable_record_encode(struct reftable_record *rec, struct string_view dest,
138139
int hash_size);
139140
int reftable_record_decode(struct reftable_record *rec, struct strbuf key,
140141
uint8_t extra, struct string_view src,
141-
int hash_size);
142+
int hash_size, struct strbuf *scratch);
142143
int reftable_record_is_deletion(struct reftable_record *rec);
143144

144145
static inline uint8_t reftable_record_type(struct reftable_record *rec)

‎reftable/record_test.c

Lines changed: 36 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,7 @@ static void set_hash(uint8_t *h, int j)
9999

100100
static void test_reftable_ref_record_roundtrip(void)
101101
{
102+
struct strbuf scratch = STRBUF_INIT;
102103
int i = 0;
103104

104105
for (i = REFTABLE_REF_DELETION; i < REFTABLE_NR_REF_VALUETYPES; i++) {
@@ -140,7 +141,7 @@ static void test_reftable_ref_record_roundtrip(void)
140141
EXPECT(n > 0);
141142

142143
/* 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);
144145
EXPECT(n == m);
145146

146147
EXPECT(reftable_ref_record_equal(&in.u.ref, &out.u.ref,
@@ -150,6 +151,8 @@ static void test_reftable_ref_record_roundtrip(void)
150151
strbuf_release(&key);
151152
reftable_record_release(&out);
152153
}
154+
155+
strbuf_release(&scratch);
153156
}
154157

155158
static void test_reftable_log_record_equal(void)
@@ -175,7 +178,6 @@ static void test_reftable_log_record_equal(void)
175178
static void test_reftable_log_record_roundtrip(void)
176179
{
177180
int i;
178-
179181
struct reftable_log_record in[] = {
180182
{
181183
.refname = xstrdup("refs/heads/master"),
@@ -202,6 +204,8 @@ static void test_reftable_log_record_roundtrip(void)
202204
.value_type = REFTABLE_LOG_UPDATE,
203205
}
204206
};
207+
struct strbuf scratch = STRBUF_INIT;
208+
205209
set_test_hash(in[0].value.update.new_hash, 1);
206210
set_test_hash(in[0].value.update.old_hash, 2);
207211
set_test_hash(in[2].value.update.new_hash, 3);
@@ -241,7 +245,7 @@ static void test_reftable_log_record_roundtrip(void)
241245
EXPECT(n >= 0);
242246
valtype = reftable_record_val_type(&rec);
243247
m = reftable_record_decode(&out, key, valtype, dest,
244-
GIT_SHA1_RAWSZ);
248+
GIT_SHA1_RAWSZ, &scratch);
245249
EXPECT(n == m);
246250

247251
EXPECT(reftable_log_record_equal(&in[i], &out.u.log,
@@ -250,6 +254,8 @@ static void test_reftable_log_record_roundtrip(void)
250254
strbuf_release(&key);
251255
reftable_record_release(&out);
252256
}
257+
258+
strbuf_release(&scratch);
253259
}
254260

255261
static void test_u24_roundtrip(void)
@@ -299,23 +305,27 @@ static void test_reftable_obj_record_roundtrip(void)
299305
{
300306
uint8_t testHash1[GIT_SHA1_RAWSZ] = { 1, 2, 3, 4, 0 };
301307
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;
318327
int i = 0;
328+
319329
for (i = 0; i < ARRAY_SIZE(recs); i++) {
320330
uint8_t buffer[1024] = { 0 };
321331
struct string_view dest = {
@@ -339,13 +349,15 @@ static void test_reftable_obj_record_roundtrip(void)
339349
EXPECT(n > 0);
340350
extra = reftable_record_val_type(&in);
341351
m = reftable_record_decode(&out, key, extra, dest,
342-
GIT_SHA1_RAWSZ);
352+
GIT_SHA1_RAWSZ, &scratch);
343353
EXPECT(n == m);
344354

345355
EXPECT(reftable_record_equal(&in, &out, GIT_SHA1_RAWSZ));
346356
strbuf_release(&key);
347357
reftable_record_release(&out);
348358
}
359+
360+
strbuf_release(&scratch);
349361
}
350362

351363
static void test_reftable_index_record_roundtrip(void)
@@ -362,6 +374,7 @@ static void test_reftable_index_record_roundtrip(void)
362374
.buf = buffer,
363375
.len = sizeof(buffer),
364376
};
377+
struct strbuf scratch = STRBUF_INIT;
365378
struct strbuf key = STRBUF_INIT;
366379
struct reftable_record out = {
367380
.type = BLOCK_TYPE_INDEX,
@@ -379,13 +392,15 @@ static void test_reftable_index_record_roundtrip(void)
379392
EXPECT(n > 0);
380393

381394
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);
383397
EXPECT(m == n);
384398

385399
EXPECT(reftable_record_equal(&in, &out, GIT_SHA1_RAWSZ));
386400

387401
reftable_record_release(&out);
388402
strbuf_release(&key);
403+
strbuf_release(&scratch);
389404
strbuf_release(&in.u.idx.last_key);
390405
}
391406

0 commit comments

Comments
 (0)