reftable/block: move ownership of block reader into `struct table_iter`
The table iterator allows the caller to iterate through all records in a reftable table. To do so it iterates through all blocks of the desired type one by one, where for each block it creates a new block iterator and yields all its entries. One of the things that is somewhat confusing in this context is who owns the block reader that is being used to read the blocks and pass them to the block iterator. Intuitively, as the table iterator is responsible for iterating through the blocks, one would assume that this iterator is also responsible for managing the lifecycle of the reader. And while it somewhat is, the block reader is ultimately stored inside of the block iterator. Refactor the code such that the block reader is instead fully managed by the table iterator. Instead of passing the reader to the block iterator, we now only end up passing the block data to it. Despite clearing up the lifecycle of the reader, it will also allow for better reuse of the reader in subsequent patches. The following benchmark prints a single matching ref out of 1 million refs. Before: HEAP SUMMARY: in use at exit: 13,603 bytes in 125 blocks total heap usage: 6,607 allocs, 6,482 frees, 509,635 bytes allocated After: HEAP SUMMARY: in use at exit: 13,603 bytes in 125 blocks total heap usage: 7,235 allocs, 7,110 frees, 301,481 bytes allocated Note that while there are more allocation and free calls now, the overall number of bytes allocated is significantly lower. The number of allocations will be reduced significantly by the next patch though. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Patrick Steinhardt committed
Apr 8, 2024 at 14:16 UTC
bcdc586db0b3310d05256cbe38724551e4f70475
3 files changed
+100
-83
reftable/block.c
+29
-14
@@ -261,12 +261,12 @@ void block_reader_release(struct block_reader *br)
261
reftable_block_done(&br->block);
262
}
263
264
-uint8_t block_reader_type(struct block_reader *r)
264
+uint8_t block_reader_type(const struct block_reader *r)
265
{
266
return r->block.data[r->header_off];
267
}
268
269
-int block_reader_first_key(struct block_reader *br, struct strbuf *key)
269
+int block_reader_first_key(const struct block_reader *br, struct strbuf *key)
270
{
271
int off = br->header_off + 4, n;
272
struct string_view in = {
@@ -286,14 +286,16 @@ int block_reader_first_key(struct block_reader *br, struct strbuf *key)
286
return 0;
287
}
288
289
-static uint32_t block_reader_restart_offset(struct block_reader *br, int i)
289
+static uint32_t block_reader_restart_offset(const struct block_reader *br, int i)
290
{
291
return get_be24(br->restart_bytes + 3 * i);
292
}
293
294
-void block_iter_seek_start(struct block_iter *it, struct block_reader *br)
294
+void block_iter_seek_start(struct block_iter *it, const struct block_reader *br)
295
{
296
- it->br = br;
296
+ it->block = br->block.data;
297
+ it->block_len = br->block_len;
298
+ it->hash_size = br->hash_size;
299
strbuf_reset(&it->last_key);
300
it->next_off = br->header_off + 4;
301
}
@@ -301,7 +303,7 @@ void block_iter_seek_start(struct block_iter *it, struct block_reader *br)
303
struct restart_needle_less_args {
304
int error;
305
struct strbuf needle;
304
- struct block_reader *reader;
306
+ const struct block_reader *reader;
307
};
308
309
static int restart_needle_less(size_t idx, void *_args)
@@ -340,9 +342,11 @@ static int restart_needle_less(size_t idx, void *_args)
342
return args->needle.len < suffix_len;
343
}
344
343
-void block_iter_copy_from(struct block_iter *dest, struct block_iter *src)
345
+void block_iter_copy_from(struct block_iter *dest, const struct block_iter *src)
346
{
345
- dest->br = src->br;
347
+ dest->block = src->block;
348
+ dest->block_len = src->block_len;
349
+ dest->hash_size = src->hash_size;
350
dest->next_off = src->next_off;
351
strbuf_reset(&dest->last_key);
352
strbuf_addbuf(&dest->last_key, &src->last_key);
@@ -351,14 +355,14 @@ void block_iter_copy_from(struct block_iter *dest, struct block_iter *src)
355
int block_iter_next(struct block_iter *it, struct reftable_record *rec)
356
{
357
struct string_view in = {
354
- .buf = it->br->block.data + it->next_off,
355
- .len = it->br->block_len - it->next_off,
358
+ .buf = (unsigned char *) it->block + it->next_off,
359
+ .len = it->block_len - it->next_off,
360
};
361
struct string_view start = in;
362
uint8_t extra = 0;
363
int n = 0;
364
361
- if (it->next_off >= it->br->block_len)
365
+ if (it->next_off >= it->block_len)
366
return 1;
367
368
n = reftable_decode_key(&it->last_key, &extra, in);
@@ -368,7 +372,7 @@ int block_iter_next(struct block_iter *it, struct reftable_record *rec)
372
return REFTABLE_FORMAT_ERROR;
373
374
string_view_consume(&in, n);
371
- n = reftable_record_decode(rec, it->last_key, extra, in, it->br->hash_size,
375
+ n = reftable_record_decode(rec, it->last_key, extra, in, it->hash_size,
376
&it->scratch);
377
if (n < 0)
378
return -1;
@@ -378,13 +382,22 @@ int block_iter_next(struct block_iter *it, struct reftable_record *rec)
382
return 0;
383
}
384
385
+void block_iter_reset(struct block_iter *it)
386
+{
387
+ strbuf_reset(&it->last_key);
388
+ it->next_off = 0;
389
+ it->block = NULL;
390
+ it->block_len = 0;
391
+ it->hash_size = 0;
392
+}
393
+
394
void block_iter_close(struct block_iter *it)
395
{
396
strbuf_release(&it->last_key);
397
strbuf_release(&it->scratch);
398
}
399
387
-int block_iter_seek_key(struct block_iter *it, struct block_reader *br,
400
+int block_iter_seek_key(struct block_iter *it, const struct block_reader *br,
401
struct strbuf *want)
402
{
403
struct restart_needle_less_args args = {
@@ -436,7 +449,9 @@ int block_iter_seek_key(struct block_iter *it, struct block_reader *br,
449
it->next_off = block_reader_restart_offset(br, i - 1);
450
else
451
it->next_off = br->header_off + 4;
439
- it->br = br;
452
+ it->block = br->block.data;
453
+ it->block_len = br->block_len;
454
+ it->hash_size = br->hash_size;
455
456
reftable_record_init(&rec, block_reader_type(br));
457
reftable/block.h
+11
-6
@@ -84,16 +84,18 @@ int block_reader_init(struct block_reader *br, struct reftable_block *bl,
84
void block_reader_release(struct block_reader *br);
85
86
/* Returns the block type (eg. 'r' for refs) */
87
-uint8_t block_reader_type(struct block_reader *r);
87
+uint8_t block_reader_type(const struct block_reader *r);
88
89
/* Decodes the first key in the block */
90
-int block_reader_first_key(struct block_reader *br, struct strbuf *key);
90
+int block_reader_first_key(const struct block_reader *br, struct strbuf *key);
91
92
/* Iterate over entries in a block */
93
struct block_iter {
94
/* offset within the block of the next entry to read. */
95
uint32_t next_off;
96
- struct block_reader *br;
96
+ const unsigned char *block;
97
+ size_t block_len;
98
+ int hash_size;
99
100
/* key for last entry we read. */
101
struct strbuf last_key;
@@ -106,17 +108,20 @@ struct block_iter {
108
}
109
110
/* Position `it` at start of the block */
109
-void block_iter_seek_start(struct block_iter *it, struct block_reader *br);
111
+void block_iter_seek_start(struct block_iter *it, const struct block_reader *br);
112
113
/* Position `it` to the `want` key in the block */
112
-int block_iter_seek_key(struct block_iter *it, struct block_reader *br,
114
+int block_iter_seek_key(struct block_iter *it, const struct block_reader *br,
115
struct strbuf *want);
116
115
-void block_iter_copy_from(struct block_iter *dest, struct block_iter *src);
117
+void block_iter_copy_from(struct block_iter *dest, const struct block_iter *src);
118
119
/* return < 0 for error, 0 for OK, > 0 for EOF. */
120
int block_iter_next(struct block_iter *it, struct reftable_record *rec);
121
122
+/* Reset the block iterator to pristine state without releasing its memory. */
123
+void block_iter_reset(struct block_iter *it);
124
+
125
/* deallocate memory for `it`. The block reader and its block is left intact. */
126
void block_iter_close(struct block_iter *it);
127
reftable/reader.c
+60
-63
@@ -220,6 +220,7 @@ struct table_iter {
220
struct reftable_reader *r;
221
uint8_t typ;
222
uint64_t block_off;
223
+ struct block_reader br;
224
struct block_iter bi;
225
int is_finished;
226
};
@@ -227,16 +228,6 @@ struct table_iter {
228
.bi = BLOCK_ITER_INIT \
229
}
230
230
-static void table_iter_copy_from(struct table_iter *dest,
231
- struct table_iter *src)
232
-{
233
- dest->r = src->r;
234
- dest->typ = src->typ;
235
- dest->block_off = src->block_off;
236
- dest->is_finished = src->is_finished;
237
- block_iter_copy_from(&dest->bi, &src->bi);
238
-}
239
-
231
static int table_iter_next_in_block(struct table_iter *ti,
232
struct reftable_record *rec)
233
{
@@ -250,14 +241,8 @@ static int table_iter_next_in_block(struct table_iter *ti,
241
242
static void table_iter_block_done(struct table_iter *ti)
243
{
253
- if (!ti->bi.br) {
254
- return;
255
- }
256
- block_reader_release(ti->bi.br);
257
- FREE_AND_NULL(ti->bi.br);
258
-
259
- ti->bi.last_key.len = 0;
260
- ti->bi.next_off = 0;
244
+ block_reader_release(&ti->br);
245
+ block_iter_reset(&ti->bi);
246
}
247
248
static int32_t extract_block_size(uint8_t *data, uint8_t *typ, uint64_t off,
@@ -321,32 +306,33 @@ done:
306
return err;
307
}
308
309
+static void table_iter_close(struct table_iter *ti)
310
+{
311
+ table_iter_block_done(ti);
312
+ block_iter_close(&ti->bi);
313
+}
314
+
315
static int table_iter_next_block(struct table_iter *dest,
316
struct table_iter *src)
317
{
327
- uint64_t next_block_off = src->block_off + src->bi.br->full_block_size;
328
- struct block_reader br = { 0 };
329
- int err = 0;
318
+ uint64_t next_block_off = src->block_off + src->br.full_block_size;
319
+ int err;
320
321
dest->r = src->r;
322
dest->typ = src->typ;
323
dest->block_off = next_block_off;
324
335
- err = reader_init_block_reader(src->r, &br, next_block_off, src->typ);
336
- if (err > 0) {
325
+ err = reader_init_block_reader(src->r, &dest->br, next_block_off, src->typ);
326
+ if (err > 0)
327
dest->is_finished = 1;
338
- return 1;
339
- }
340
- if (err != 0)
328
+ if (err) {
329
+ table_iter_block_done(dest);
330
return err;
342
- else {
343
- struct block_reader *brp =
344
- reftable_malloc(sizeof(struct block_reader));
345
- *brp = br;
346
-
347
- dest->is_finished = 0;
348
- block_iter_seek_start(&dest->bi, brp);
331
}
332
+
333
+ dest->is_finished = 0;
334
+ block_iter_seek_start(&dest->bi, &dest->br);
335
+
336
return 0;
337
}
338
@@ -377,14 +363,13 @@ static int table_iter_next(struct table_iter *ti, struct reftable_record *rec)
363
* iterator is drained.
364
*/
365
err = table_iter_next_block(&next, ti);
380
- table_iter_block_done(ti);
366
if (err) {
367
ti->is_finished = 1;
368
return err;
369
}
370
386
- table_iter_copy_from(ti, &next);
387
- block_iter_close(&next.bi);
371
+ table_iter_close(ti);
372
+ *ti = next;
373
}
374
}
375
@@ -393,16 +378,14 @@ static int table_iter_next_void(void *ti, struct reftable_record *rec)
378
return table_iter_next(ti, rec);
379
}
380
396
-static void table_iter_close(void *p)
381
+static void table_iter_close_void(void *ti)
382
{
398
- struct table_iter *ti = p;
399
- table_iter_block_done(ti);
400
- block_iter_close(&ti->bi);
383
+ table_iter_close(ti);
384
}
385
386
static struct reftable_iterator_vtable table_iter_vtable = {
387
.next = &table_iter_next_void,
405
- .close = &table_iter_close,
388
+ .close = &table_iter_close_void,
389
};
390
391
static void iterator_from_table_iter(struct reftable_iterator *it,
@@ -417,19 +400,16 @@ static int reader_table_iter_at(struct reftable_reader *r,
400
struct table_iter *ti, uint64_t off,
401
uint8_t typ)
402
{
420
- struct block_reader br = { 0 };
421
- struct block_reader *brp = NULL;
403
+ int err;
404
423
- int err = reader_init_block_reader(r, &br, off, typ);
405
+ err = reader_init_block_reader(r, &ti->br, off, typ);
406
if (err != 0)
407
return err;
408
427
- brp = reftable_malloc(sizeof(struct block_reader));
428
- *brp = br;
409
ti->r = r;
430
- ti->typ = block_reader_type(brp);
410
+ ti->typ = block_reader_type(&ti->br);
411
ti->block_off = off;
432
- block_iter_seek_start(&ti->bi, brp);
412
+ block_iter_seek_start(&ti->bi, &ti->br);
413
return 0;
414
}
415
@@ -454,23 +434,34 @@ static int reader_seek_linear(struct table_iter *ti,
434
{
435
struct strbuf want_key = STRBUF_INIT;
436
struct strbuf got_key = STRBUF_INIT;
457
- struct table_iter next = TABLE_ITER_INIT;
437
struct reftable_record rec;
438
int err = -1;
439
440
reftable_record_init(&rec, reftable_record_type(want));
441
reftable_record_key(want, &want_key);
442
443
+ /*
444
+ * First we need to locate the block that must contain our record. To
445
+ * do so we scan through blocks linearly until we find the first block
446
+ * whose first key is bigger than our wanted key. Once we have found
447
+ * that block we know that the key must be contained in the preceding
448
+ * block.
449
+ *
450
+ * This algorithm is somewhat unfortunate because it means that we
451
+ * always have to seek one block too far and then back up. But as we
452
+ * can only decode the _first_ key of a block but not its _last_ key we
453
+ * have no other way to do this.
454
+ */
455
while (1) {
456
+ struct table_iter next = TABLE_ITER_INIT;
457
+
458
err = table_iter_next_block(&next, ti);
459
if (err < 0)
460
goto done;
468
-
469
- if (err > 0) {
461
+ if (err > 0)
462
break;
471
- }
463
473
- err = block_reader_first_key(next.bi.br, &got_key);
464
+ err = block_reader_first_key(&next.br, &got_key);
465
if (err < 0)
466
goto done;
467
@@ -480,16 +471,20 @@ static int reader_seek_linear(struct table_iter *ti,
471
}
472
473
table_iter_block_done(ti);
483
- table_iter_copy_from(ti, &next);
474
+ *ti = next;
475
}
476
486
- err = block_iter_seek_key(&ti->bi, ti->bi.br, &want_key);
477
+ /*
478
+ * We have located the block that must contain our record, so we seek
479
+ * the wanted key inside of it. If the block does not contain our key
480
+ * we know that the corresponding record does not exist.
481
+ */
482
+ err = block_iter_seek_key(&ti->bi, &ti->br, &want_key);
483
if (err < 0)
484
goto done;
485
err = 0;
486
487
done:
492
- block_iter_close(&next.bi);
488
reftable_record_release(&rec);
489
strbuf_release(&want_key);
490
strbuf_release(&got_key);
@@ -508,6 +503,7 @@ static int reader_seek_indexed(struct reftable_reader *r,
503
.u.idx = { .last_key = STRBUF_INIT },
504
};
505
struct table_iter index_iter = TABLE_ITER_INIT;
506
+ struct table_iter empty = TABLE_ITER_INIT;
507
struct table_iter next = TABLE_ITER_INIT;
508
int err = 0;
509
@@ -549,7 +545,6 @@ static int reader_seek_indexed(struct reftable_reader *r,
545
* not exist.
546
*/
547
err = table_iter_next(&index_iter, &index_result);
552
- table_iter_block_done(&index_iter);
548
if (err != 0)
549
goto done;
550
@@ -558,7 +553,7 @@ static int reader_seek_indexed(struct reftable_reader *r,
553
if (err != 0)
554
goto done;
555
561
- err = block_iter_seek_key(&next.bi, next.bi.br, &want_index.u.idx.last_key);
556
+ err = block_iter_seek_key(&next.bi, &next.br, &want_index.u.idx.last_key);
557
if (err < 0)
558
goto done;
559
@@ -572,18 +567,20 @@ static int reader_seek_indexed(struct reftable_reader *r,
567
break;
568
}
569
575
- table_iter_copy_from(&index_iter, &next);
570
+ table_iter_close(&index_iter);
571
+ index_iter = next;
572
+ next = empty;
573
}
574
575
if (err == 0) {
579
- struct table_iter empty = TABLE_ITER_INIT;
576
struct table_iter *malloced = reftable_calloc(1, sizeof(*malloced));
581
- *malloced = empty;
582
- table_iter_copy_from(malloced, &next);
577
+ *malloced = next;
578
+ next = empty;
579
iterator_from_table_iter(it, malloced);
580
}
581
+
582
done:
586
- block_iter_close(&next.bi);
583
+ table_iter_close(&next);
584
table_iter_close(&index_iter);
585
reftable_record_release(&want_index);
586
reftable_record_release(&index_result);