reftable: introduce macros to grow arrays

Throughout the reftable library we have many cases where we need to grow
arrays. In order to avoid too many reallocations, we roughly double the
capacity of the array on each iteration. The resulting code pattern is
duplicated across many sites.

We have similar patterns in our main codebase, which is why we have
eventually introduced an `ALLOC_GROW()` macro to abstract it away and
avoid some code duplication. We cannot easily reuse this macro here
though because `ALLOC_GROW()` uses `REALLOC_ARRAY()`, which in turn will
call realloc(3P) to grow the array. The reftable code is structured as a
library though (even if the boundaries are fuzzy), and one property this
brings with it is that it is possible to plug in your own allocators. So
instead of using realloc(3P), we need to use `reftable_realloc()` that
knows to use the user-provided implementation.

So let's introduce two new macros `REFTABLE_REALLOC_ARRAY()` and
`REFTABLE_ALLOC_GROW()` that mirror what we do in our main codebase,
with two modifications:

  - They use `reftable_realloc()`, as explained above.

  - They use a different growth factor of `2 * cap + 1` instead of `(cap
    + 16) * 3 / 2`.

The second change is because we know a bit more about the allocation
patterns in the reftable library. In most cases, we end up only having a
handful of items in the array and don't end up growing them. The initial
capacity that our normal growth factor uses (which is 24) would thus end
up over-allocating in a lot of code paths. This effect is measurable:

  - Before change:

      HEAP SUMMARY:
          in use at exit: 671,983 bytes in 152 blocks
        total heap usage: 3,843,446 allocs, 3,843,294 frees, 223,761,402 bytes allocated

  - After change with a growth factor of `(2 * alloc + 1)`:

      HEAP SUMMARY:
          in use at exit: 671,983 bytes in 152 blocks
        total heap usage: 3,843,446 allocs, 3,843,294 frees, 223,761,410 bytes allocated

  - After change with a growth factor of `(alloc + 16)* 2 / 3`:

      HEAP SUMMARY:
          in use at exit: 671,983 bytes in 152 blocks
        total heap usage: 3,833,673 allocs, 3,833,521 frees, 4,728,251,742 bytes allocated

While the total heap usage is roughly the same, we do end up allocating
significantly more bytes with our usual growth factor (in fact, roughly
21 times as many).

Convert the reftable library to use these new macros.

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

diff --git a/reftable/basics.c b/reftable/basics.c
index f761e48028c4490e40728fd2879a7520297a0e41..af9004cec24db5de63ba6c0120ebf4578f74b055 100644 (file)
--- a/reftable/basics.c
+++ b/reftable/basics.c
@@ -89,17 +89,13 @@ void parse_names(char *buf, int size, char ***namesp)
                        next = end;
                }
                if (p < next) {
-                       if (names_len == names_cap) {
-                               names_cap = 2 * names_cap + 1;
-                               names = reftable_realloc(
-                                       names, names_cap * sizeof(*names));
-                       }
+                       REFTABLE_ALLOC_GROW(names, names_len + 1, names_cap);
                        names[names_len++] = xstrdup(p);
                }
                p = next + 1;
        }
 
-       names = reftable_realloc(names, (names_len + 1) * sizeof(*names));
+       REFTABLE_REALLOC_ARRAY(names, names_len + 1);
        names[names_len] = NULL;
        *namesp = names;
 }

diff --git a/reftable/basics.h b/reftable/basics.h
index 096b36862b9f4ebf14be7c3074d6f86d5e92c22c..2f855cd724b35713fe35ab3635f87bbdc345b2d0 100644 (file)
--- a/reftable/basics.h
+++ b/reftable/basics.h
@@ -53,6 +53,17 @@ void *reftable_realloc(void *p, size_t sz);
 void reftable_free(void *p);
 void *reftable_calloc(size_t sz);
 
+#define REFTABLE_REALLOC_ARRAY(x, alloc) (x) = reftable_realloc((x), st_mult(sizeof(*(x)), (alloc)))
+#define REFTABLE_ALLOC_GROW(x, nr, alloc) \
+       do { \
+               if ((nr) > alloc) { \
+                       alloc = 2 * (alloc) + 1; \
+                       if (alloc < (nr)) \
+                               alloc = (nr); \
+                       REFTABLE_REALLOC_ARRAY(x, alloc); \
+               } \
+       } while (0)
+
 /* Find the longest shared prefix size of `a` and `b` */
 struct strbuf;
 int common_prefix_size(struct strbuf *a, struct strbuf *b);

diff --git a/reftable/block.c b/reftable/block.c
index 1df3d8a0f09671c74143e655cb2963adda872545..6952d0facf52425f4698dea5683c7d8542001fb1 100644 (file)
--- a/reftable/block.c
+++ b/reftable/block.c
@@ -51,12 +51,7 @@ static int block_writer_register_restart(struct block_writer *w, int n,
        if (2 + 3 * rlen + n > w->block_size - w->next)
                return -1;
        if (is_restart) {
-               if (w->restart_len == w->restart_cap) {
-                       w->restart_cap = w->restart_cap * 2 + 1;
-                       w->restarts = reftable_realloc(
-                               w->restarts, sizeof(uint32_t) * w->restart_cap);
-               }
-
+               REFTABLE_ALLOC_GROW(w->restarts, w->restart_len + 1, w->restart_cap);
                w->restarts[w->restart_len++] = w->next;
        }
 

diff --git a/reftable/merged_test.c b/reftable/merged_test.c
index 46908f738f770f4be920a65df85b96178ddbc70d..e05351e035fc089fe752ce75ca319e60f5b0b24f 100644 (file)
--- a/reftable/merged_test.c
+++ b/reftable/merged_test.c
@@ -231,14 +231,10 @@ static void test_merged(void)
        while (len < 100) { /* cap loops/recursion. */
                struct reftable_ref_record ref = { NULL };
                int err = reftable_iterator_next_ref(&it, &ref);
-               if (err > 0) {
+               if (err > 0)
                        break;
-               }
-               if (len == cap) {
-                       cap = 2 * cap + 1;
-                       out = reftable_realloc(
-                               out, sizeof(struct reftable_ref_record) * cap);
-               }
+
+               REFTABLE_ALLOC_GROW(out, len + 1, cap);
                out[len++] = ref;
        }
        reftable_iterator_destroy(&it);
@@ -368,14 +364,10 @@ static void test_merged_logs(void)
        while (len < 100) { /* cap loops/recursion. */
                struct reftable_log_record log = { NULL };
                int err = reftable_iterator_next_log(&it, &log);
-               if (err > 0) {
+               if (err > 0)
                        break;
-               }
-               if (len == cap) {
-                       cap = 2 * cap + 1;
-                       out = reftable_realloc(
-                               out, sizeof(struct reftable_log_record) * cap);
-               }
+
+               REFTABLE_ALLOC_GROW(out, len + 1, cap);
                out[len++] = log;
        }
        reftable_iterator_destroy(&it);

diff --git a/reftable/pq.c b/reftable/pq.c
index dcefeb793a9051b75a276732a32d88809bb86d94..2461daf5ff49c9748031a75041d81b1fa30a3421 100644 (file)
--- a/reftable/pq.c
+++ b/reftable/pq.c
@@ -75,13 +75,9 @@ void merged_iter_pqueue_add(struct merged_iter_pqueue *pq, const struct pq_entry
 {
        int i = 0;
 
-       if (pq->len == pq->cap) {
-               pq->cap = 2 * pq->cap + 1;
-               pq->heap = reftable_realloc(pq->heap,
-                                           pq->cap * sizeof(struct pq_entry));
-       }
-
+       REFTABLE_ALLOC_GROW(pq->heap, pq->len + 1, pq->cap);
        pq->heap[pq->len++] = *e;
+
        i = pq->len - 1;
        while (i > 0) {
                int j = (i - 1) / 2;

diff --git a/reftable/stack.c b/reftable/stack.c
index bf3869ce70dc7af837d29aabb985ee71e3e0c757..1dfab99e96fffc823e23091f01ca200f85fa71f8 100644 (file)
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -551,7 +551,7 @@ struct reftable_addition {
        struct reftable_stack *stack;
 
        char **new_tables;
-       int new_tables_len;
+       size_t new_tables_len, new_tables_cap;
        uint64_t next_update_index;
 };
 
@@ -602,8 +602,9 @@ done:
 
 static void reftable_addition_close(struct reftable_addition *add)
 {
-       int i = 0;
        struct strbuf nm = STRBUF_INIT;
+       size_t i;
+
        for (i = 0; i < add->new_tables_len; i++) {
                stack_filename(&nm, add->stack, add->new_tables[i]);
                unlink(nm.buf);
@@ -613,6 +614,7 @@ static void reftable_addition_close(struct reftable_addition *add)
        reftable_free(add->new_tables);
        add->new_tables = NULL;
        add->new_tables_len = 0;
+       add->new_tables_cap = 0;
 
        delete_tempfile(&add->lock_file);
        strbuf_release(&nm);
@@ -631,8 +633,8 @@ int reftable_addition_commit(struct reftable_addition *add)
 {
        struct strbuf table_list = STRBUF_INIT;
        int lock_file_fd = get_tempfile_fd(add->lock_file);
-       int i = 0;
        int err = 0;
+       size_t i;
 
        if (add->new_tables_len == 0)
                goto done;
@@ -660,12 +662,12 @@ int reftable_addition_commit(struct reftable_addition *add)
        }
 
        /* success, no more state to clean up. */
-       for (i = 0; i < add->new_tables_len; i++) {
+       for (i = 0; i < add->new_tables_len; i++)
                reftable_free(add->new_tables[i]);
-       }
        reftable_free(add->new_tables);
        add->new_tables = NULL;
        add->new_tables_len = 0;
+       add->new_tables_cap = 0;
 
        err = reftable_stack_reload_maybe_reuse(add->stack, 1);
        if (err)
@@ -792,11 +794,9 @@ int reftable_addition_add(struct reftable_addition *add,
                goto done;
        }
 
-       add->new_tables = reftable_realloc(add->new_tables,
-                                          sizeof(*add->new_tables) *
-                                                  (add->new_tables_len + 1));
-       add->new_tables[add->new_tables_len] = strbuf_detach(&next_name, NULL);
-       add->new_tables_len++;
+       REFTABLE_ALLOC_GROW(add->new_tables, add->new_tables_len + 1,
+                           add->new_tables_cap);
+       add->new_tables[add->new_tables_len++] = strbuf_detach(&next_name, NULL);
 done:
        if (tab_fd > 0) {
                close(tab_fd);
@@ -1367,17 +1367,12 @@ static int stack_check_addition(struct reftable_stack *st,
        while (1) {
                struct reftable_ref_record ref = { NULL };
                err = reftable_iterator_next_ref(&it, &ref);
-               if (err > 0) {
+               if (err > 0)
                        break;
-               }
                if (err < 0)
                        goto done;
 
-               if (len >= cap) {
-                       cap = 2 * cap + 1;
-                       refs = reftable_realloc(refs, cap * sizeof(refs[0]));
-               }
-
+               REFTABLE_ALLOC_GROW(refs, len + 1, cap);
                refs[len++] = ref;
        }
 

diff --git a/reftable/writer.c b/reftable/writer.c
index ee4590e20f84dd442fde4f7506e27e568f9ad509..4483bb21c3d6a86440a7877e926124bfaec0b3d4 100644 (file)
--- a/reftable/writer.c
+++ b/reftable/writer.c
@@ -200,12 +200,7 @@ static void writer_index_hash(struct reftable_writer *w, struct strbuf *hash)
                return;
        }
 
-       if (key->offset_len == key->offset_cap) {
-               key->offset_cap = 2 * key->offset_cap + 1;
-               key->offsets = reftable_realloc(
-                       key->offsets, sizeof(uint64_t) * key->offset_cap);
-       }
-
+       REFTABLE_ALLOC_GROW(key->offsets, key->offset_len + 1, key->offset_cap);
        key->offsets[key->offset_len++] = off;
 }
 
@@ -674,12 +669,7 @@ static int writer_flush_nonempty_block(struct reftable_writer *w)
        if (err < 0)
                return err;
 
-       if (w->index_cap == w->index_len) {
-               w->index_cap = 2 * w->index_cap + 1;
-               w->index = reftable_realloc(
-                       w->index,
-                       sizeof(struct reftable_index_record) * w->index_cap);
-       }
+       REFTABLE_ALLOC_GROW(w->index, w->index_len + 1, w->index_cap);
 
        ir.offset = w->next;
        strbuf_reset(&ir.last_key);