@matheusbsbs / matheus-bagliano / commits / daf4f43d0d

reftable/record: decode keys in place

When reading a record from a block, we need to decode the record's key. As reftable keys are prefix-compressed, meaning they reuse a prefix from the preceding record's key, this is a bit more involved than just having to copy the relevant bytes: we need to figure out the prefix and suffix lengths, copy the prefix from the preceding record and finally copy the suffix from the current record. This is done by passing three buffers to `reftable_decode_key()`: one buffer that holds the result, one buffer that holds the last key, and one buffer that points to the current record. The final key is then assembled by calling `strbuf_add()` twice to copy over the prefix and suffix. Performing two memory copies is inefficient though. And we can indeed do better by decoding keys in place. Instead of providing two buffers, the caller may only call a single buffer that is already pre-populated with the last key. Like this, we only have to call `strbuf_setlen()` to trim the record to its prefix and then `strbuf_add()` to add the suffix. This refactoring leads to a noticeable performance bump when iterating over 1 million refs: Benchmark 1: show-ref: single matching ref (revision = HEAD~) Time (mean ± σ): 112.2 ms ± 3.9 ms [User: 109.3 ms, System: 2.8 ms] Range (min … max): 109.2 ms … 149.6 ms 1000 runs Benchmark 2: show-ref: single matching ref (revision = HEAD) Time (mean ± σ): 106.0 ms ± 3.5 ms [User: 103.2 ms, System: 2.7 ms] Range (min … max): 103.2 ms … 133.7 ms 1000 runs Summary show-ref: single matching ref (revision = HEAD) ran 1.06 ± 0.05 times faster than show-ref: single matching ref (revision = HEAD~) Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Patrick Steinhardt committed Mar 4, 2024 at 11:49 UTC daf4f43d0d84234c2308c95ba63e54bdb8846859

5 files changed +28 -30

reftable/block.c

+11 -14

@@ -291,9 +291,8 @@ static int restart_key_less(size_t idx, void *args)

291 /* the restart key is verbatim in the block, so this could avoid the

292 alloc for decoding the key */

293 struct strbuf rkey = STRBUF_INIT;

294 - struct strbuf last_key = STRBUF_INIT;

294 uint8_t unused_extra;

296 - int n = reftable_decode_key(&rkey, &unused_extra, last_key, in);

295 + int n = reftable_decode_key(&rkey, &unused_extra, in);

296 int result;

297 if (n < 0) {

298 a->error = 1;

@@ -326,35 +325,34 @@ int block_iter_next(struct block_iter *it, struct reftable_record *rec)

325 if (it->next_off >= it->br->block_len)

326 return 1;

327

329 - n = reftable_decode_key(&it->key, &extra, it->last_key, in);

328 + n = reftable_decode_key(&it->last_key, &extra, in);

329 if (n < 0)

330 return -1;

332 -

333 - if (!it->key.len)

331 + if (!it->last_key.len)

332 return REFTABLE_FORMAT_ERROR;

333

334 string_view_consume(&in, n);

337 - n = reftable_record_decode(rec, it->key, extra, in, it->br->hash_size);

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

336 if (n < 0)

337 return -1;

338 string_view_consume(&in, n);

339

342 - strbuf_swap(&it->last_key, &it->key);

340 it->next_off += start.len - in.len;

341 return 0;

342 }

343

344 int block_reader_first_key(struct block_reader *br, struct strbuf *key)

345 {

349 - struct strbuf empty = STRBUF_INIT;

350 - int off = br->header_off + 4;

346 + int off = br->header_off + 4, n;

347 struct string_view in = {

348 .buf = br->block.data + off,

349 .len = br->block_len - off,

350 };

355 -

351 uint8_t extra = 0;

357 - int n = reftable_decode_key(key, &extra, empty, in);

352 +

353 + strbuf_reset(key);

354 +

355 + n = reftable_decode_key(key, &extra, in);

356 if (n < 0)

357 return n;

358 if (!key->len)

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

369 void block_iter_close(struct block_iter *it)

370 {

371 strbuf_release(&it->last_key);

374 - strbuf_release(&it->key);

372 }

373

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

@@ -408,8 +405,8 @@ int block_reader_seek(struct block_reader *br, struct block_iter *it,

405 if (err < 0)

406 goto done;

407

411 - reftable_record_key(&rec, &it->key);

412 - if (err > 0 || strbuf_cmp(&it->key, want) >= 0) {

408 + reftable_record_key(&rec, &it->last_key);

409 + if (err > 0 || strbuf_cmp(&it->last_key, want) >= 0) {

410 err = 0;

411 goto done;

412 }

reftable/block.h

-2

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

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

86 struct strbuf last_key;

87 - struct strbuf key;

87 };

89 #define BLOCK_ITER_INIT { \

90 .last_key = STRBUF_INIT, \

92 - .key = STRBUF_INIT, \

91 }

93 /* initializes a block reader. */

reftable/record.c

+9 -10

@@ -159,20 +159,19 @@ int reftable_encode_key(int *restart, struct string_view dest,

159 return start.len - dest.len;

160 }

161

162 -int reftable_decode_key(struct strbuf *key, uint8_t *extra,

163 - struct strbuf last_key, struct string_view in)

162 +int reftable_decode_key(struct strbuf *last_key, uint8_t *extra,

163 + struct string_view in)

164 {

165 int start_len = in.len;

166 uint64_t prefix_len = 0;

167 uint64_t suffix_len = 0;

168 - int n = get_var_int(&prefix_len, &in);

168 + int n;

169 +

170 + n = get_var_int(&prefix_len, &in);

171 if (n < 0)

172 return -1;

173 string_view_consume(&in, n);

174

173 - if (prefix_len > last_key.len)

174 - return -1;

175 -

175 n = get_var_int(&suffix_len, &in);

176 if (n <= 0)

177 return -1;

@@ -181,12 +180,12 @@ int reftable_decode_key(struct strbuf *key, uint8_t *extra,

180 *extra = (uint8_t)(suffix_len & 0x7);

181 suffix_len >>= 3;

182

184 - if (in.len < suffix_len)

183 + if (in.len < suffix_len ||

184 + prefix_len > last_key->len)

185 return -1;

186

187 - strbuf_reset(key);

188 - strbuf_add(key, last_key.buf, prefix_len);

189 - strbuf_add(key, in.buf, suffix_len);

187 + strbuf_setlen(last_key, prefix_len);

188 + strbuf_add(last_key, in.buf, suffix_len);

189 string_view_consume(&in, suffix_len);

190

191 return start_len - in.len;

reftable/record.h

+6 -3

@@ -81,9 +81,12 @@ int reftable_encode_key(int *is_restart, struct string_view dest,

81 struct strbuf prev_key, struct strbuf key,

82 uint8_t extra);

84 -/* Decode into `key` and `extra` from `in` */

85 -int reftable_decode_key(struct strbuf *key, uint8_t *extra,

86 - struct strbuf last_key, struct string_view in);

84 +/*

85 + * Decode into `last_key` and `extra` from `in`. `last_key` is expected to

86 + * contain the decoded key of the preceding record, if any.

87 + */

88 +int reftable_decode_key(struct strbuf *last_key, uint8_t *extra,

89 + struct string_view in);

91 /* reftable_index_record are used internally to speed up lookups. */

92 struct reftable_index_record {

reftable/record_test.c

+2 -1

@@ -295,7 +295,8 @@ static void test_key_roundtrip(void)

295 EXPECT(!restart);

296 EXPECT(n > 0);

297

298 - m = reftable_decode_key(&roundtrip, &rt_extra, last_key, dest);

298 + strbuf_addstr(&roundtrip, "refs/heads/master");

299 + m = reftable_decode_key(&roundtrip, &rt_extra, dest);

300 EXPECT(n == m);

301 EXPECT(0 == strbuf_cmp(&key, &roundtrip));

302 EXPECT(rt_extra == extra);