reftable: fix perf regression when reading blocks of unwanted type

In fd888311fbc (reftable/table: move reading block into block reader,
2025-04-07), we have refactored how reftable blocks are read so that
most of the logic is contained in the "block.c" subsystem itself. Most
importantly, the whole logic to read the data itself is now contained in
that subsystem.

This change caused a significant performance regression though when
reading blocks that aren't of the specific type one is searching for:

    Benchmark 1: update-ref: create 100k refs (revision = fd888311fbc~)
      Time (mean ± σ):      2.171 s ±  0.028 s    [User: 1.189 s, System: 0.977 s]
      Range (min … max):    2.117 s …  2.206 s    10 runs

    Benchmark 2: update-ref: create 100k refs (revision = fd888311fbc)
      Time (mean ± σ):      3.418 s ±  0.030 s    [User: 2.371 s, System: 1.037 s]
      Range (min … max):    3.377 s …  3.473 s    10 runs

    Summary
      update-ref: create 100k refs (revision = fd888311fbc~) ran
        1.57 ± 0.02 times faster than update-ref: create 100k refs (revision = fd888311fbc)

The root caute of the performance regression is that we changed when
exactly blocks of an uninteresting type are being discarded. Previous to
the refactoring in the mentioned commit we'd load the block data, read
its type, notice that it's not the wanted type and discard the block.
After the commit though we don't discard the block immediately, but we
fully decode it only to realize that it's not the desired type. We then
discard the block again, but have already performed a bunch of pointless
work.

Fix the regression by making `reftable_block_init()` return early in
case the block is not of the desired type. This fixes the performance
hit:

    Benchmark 1: update-ref: create 100k refs (revision = HEAD~)
      Time (mean ± σ):      2.712 s ±  0.018 s    [User: 1.990 s, System: 0.716 s]
      Range (min … max):    2.682 s …  2.741 s    10 runs

    Benchmark 2: update-ref: create 100k refs (revision = HEAD)
      Time (mean ± σ):      1.670 s ±  0.012 s    [User: 0.991 s, System: 0.676 s]
      Range (min … max):    1.652 s …  1.693 s    10 runs

    Summary
      update-ref: create 100k refs (revision = HEAD) ran
        1.62 ± 0.02 times faster than update-ref: create 100k refs (revision = HEAD~)

Note that the baseline performance is lower than in the original due to
a couple of unrelated performance improvements that have landed since
the original commit.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/reftable/block.c b/reftable/block.c
index 795815b47629912997a55cc08693de39caab6381..31569cba593abc0266c511b2f9b7cbe28df8e6ee 100644 (file)
--- a/reftable/block.c
+++ b/reftable/block.c
@@ -226,7 +226,8 @@ static int read_block(struct reftable_block_source *source,
 int reftable_block_init(struct reftable_block *block,
                        struct reftable_block_source *source,
                        uint32_t offset, uint32_t header_size,
-                       uint32_t table_block_size, uint32_t hash_size)
+                       uint32_t table_block_size, uint32_t hash_size,
+                       uint8_t want_type)
 {
        uint32_t guess_block_size = table_block_size ?
                table_block_size : DEFAULT_BLOCK_SIZE;
@@ -246,6 +247,10 @@ int reftable_block_init(struct reftable_block *block,
                err = REFTABLE_FORMAT_ERROR;
                goto done;
        }
+       if (want_type != REFTABLE_BLOCK_TYPE_ANY && block_type != want_type) {
+               err = 1;
+               goto done;
+       }
 
        block_size = reftable_get_be24(block->block_data.data + header_size + 1);
        if (block_size > guess_block_size) {

diff --git a/reftable/reftable-block.h b/reftable/reftable-block.h
index 04c3b518c8746092bb1b1b5456b107c9981fbdc4..0b05a8f7e376bc897399d701f9634f650e4d87c8 100644 (file)
--- a/reftable/reftable-block.h
+++ b/reftable/reftable-block.h
@@ -56,7 +56,8 @@ struct reftable_block {
 int reftable_block_init(struct reftable_block *b,
                        struct reftable_block_source *source,
                        uint32_t offset, uint32_t header_size,
-                       uint32_t table_block_size, uint32_t hash_size);
+                       uint32_t table_block_size, uint32_t hash_size,
+                       uint8_t want_type);
 
 /* Release resources allocated by the block. */
 void reftable_block_release(struct reftable_block *b);

diff --git a/reftable/table.c b/reftable/table.c
index ee831276158c93a1e6daba219fce182a653eac45..56362df0eda5a6b63fbbac5395c97ce05b822596 100644 (file)
--- a/reftable/table.c
+++ b/reftable/table.c
@@ -173,16 +173,7 @@ int table_init_block(struct reftable_table *t, struct reftable_block *block,
                return 1;
 
        err = reftable_block_init(block, &t->source, next_off, header_off,
-                                 t->block_size, hash_size(t->hash_id));
-       if (err < 0)
-               goto done;
-
-       if (want_typ != REFTABLE_BLOCK_TYPE_ANY && block->block_type != want_typ) {
-               err = 1;
-               goto done;
-       }
-
-done:
+                                 t->block_size, hash_size(t->hash_id), want_typ);
        if (err)
                reftable_block_release(block);
        return err;

diff --git a/t/unit-tests/t-reftable-block.c b/t/unit-tests/t-reftable-block.c
index 7dbd93601c769650ca8adaa78d0b384c6bebbf71..52f1dae1c9576de8ffd82bfca1b84089bc11cdc5 100644 (file)
--- a/t/unit-tests/t-reftable-block.c
+++ b/t/unit-tests/t-reftable-block.c
@@ -64,7 +64,8 @@ static void t_ref_block_read_write(void)
        block_writer_release(&bw);
 
        block_source_from_buf(&source ,&block_data);
-       reftable_block_init(&block, &source, 0, header_off, block_size, REFTABLE_HASH_SIZE_SHA1);
+       reftable_block_init(&block, &source, 0, header_off, block_size,
+                           REFTABLE_HASH_SIZE_SHA1, REFTABLE_BLOCK_TYPE_REF);
 
        block_iter_init(&it, &block);
 
@@ -153,7 +154,8 @@ static void t_log_block_read_write(void)
        block_writer_release(&bw);
 
        block_source_from_buf(&source, &block_data);
-       reftable_block_init(&block, &source, 0, header_off, block_size, REFTABLE_HASH_SIZE_SHA1);
+       reftable_block_init(&block, &source, 0, header_off, block_size,
+                           REFTABLE_HASH_SIZE_SHA1, REFTABLE_BLOCK_TYPE_LOG);
 
        block_iter_init(&it, &block);
 
@@ -245,7 +247,8 @@ static void t_obj_block_read_write(void)
        block_writer_release(&bw);
 
        block_source_from_buf(&source, &block_data);
-       reftable_block_init(&block, &source, 0, header_off, block_size, REFTABLE_HASH_SIZE_SHA1);
+       reftable_block_init(&block, &source, 0, header_off, block_size,
+                           REFTABLE_HASH_SIZE_SHA1, REFTABLE_BLOCK_TYPE_OBJ);
 
        block_iter_init(&it, &block);
 
@@ -329,7 +332,8 @@ static void t_index_block_read_write(void)
        block_writer_release(&bw);
 
        block_source_from_buf(&source, &block_data);
-       reftable_block_init(&block, &source, 0, header_off, block_size, REFTABLE_HASH_SIZE_SHA1);
+       reftable_block_init(&block, &source, 0, header_off, block_size,
+                           REFTABLE_HASH_SIZE_SHA1, REFTABLE_BLOCK_TYPE_INDEX);
 
        block_iter_init(&it, &block);
 
@@ -411,7 +415,8 @@ static void t_block_iterator(void)
        check_int(err, >, 0);
 
        block_source_from_buf(&source, &data);
-       reftable_block_init(&block, &source, 0, 0, data.len, REFTABLE_HASH_SIZE_SHA1);
+       reftable_block_init(&block, &source, 0, 0, data.len,
+                           REFTABLE_HASH_SIZE_SHA1, REFTABLE_BLOCK_TYPE_REF);
 
        err = reftable_block_init_iterator(&block, &it);
        check_int(err, ==, 0);