@matheusbsbs / matheus-bagliano / commits / b8ca235ca5

reftable/merged: stop using generic tables in the merged table

The merged table provides access to a reftable stack by merging the contents of those tables into a virtual table. These subtables are being tracked via `struct reftable_table`, which is a generic interface for accessing either a single reftable or a merged reftable. So in theory, it would be possible for the merged table to merge together other merged tables. This is somewhat nonsensical though: we only ever set up a merged table over normal reftables, and there is no reason to do otherwise. This generic interface thus makes the code way harder to follow and reason about than really necessary. The abstraction layer may also have an impact on performance, even though the extra set of vtable function calls probably doesn't really matter. Refactor the merged tables to use a `struct reftable_reader` for each of the subtables instead, which gives us direct access to the underlying tables. Adjust names accordingly. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Patrick Steinhardt committed Aug 22, 2024 at 08:34 UTC b8ca235ca5fad0421766db7fe2c0f52c6707bf8e
8 files changed +60 -75
reftable/merged.c
+14 -14
@@ -11,6 +11,7 @@ https://developers.google.com/open-source/licenses/bsd
11 #include "constants.h"
12 #include "iter.h"
13 #include "pq.h"
14 +#include "reader.h"
15 #include "record.h"
16 #include "generic.h"
17 #include "reftable-merged.h"
@@ -25,7 +26,7 @@ struct merged_subiter {
26 struct merged_iter {
27 struct merged_subiter *subiters;
28 struct merged_iter_pqueue pq;
28 - size_t stack_len;
29 + size_t subiters_len;
30 int suppress_deletions;
31 ssize_t advance_index;
32 };
@@ -38,12 +39,12 @@ static void merged_iter_init(struct merged_iter *mi,
39 mi->advance_index = -1;
40 mi->suppress_deletions = mt->suppress_deletions;
41
41 - REFTABLE_CALLOC_ARRAY(mi->subiters, mt->stack_len);
42 - for (size_t i = 0; i < mt->stack_len; i++) {
42 + REFTABLE_CALLOC_ARRAY(mi->subiters, mt->readers_len);
43 + for (size_t i = 0; i < mt->readers_len; i++) {
44 reftable_record_init(&mi->subiters[i].rec, typ);
44 - table_init_iter(&mt->stack[i], &mi->subiters[i].iter, typ);
45 + reader_init_iter(mt->readers[i], &mi->subiters[i].iter, typ);
46 }
46 - mi->stack_len = mt->stack_len;
47 + mi->subiters_len = mt->readers_len;
48 }
49
50 static void merged_iter_close(void *p)
@@ -51,7 +52,7 @@ static void merged_iter_close(void *p)
52 struct merged_iter *mi = p;
53
54 merged_iter_pqueue_release(&mi->pq);
54 - for (size_t i = 0; i < mi->stack_len; i++) {
55 + for (size_t i = 0; i < mi->subiters_len; i++) {
56 reftable_iterator_destroy(&mi->subiters[i].iter);
57 reftable_record_release(&mi->subiters[i].rec);
58 }
@@ -80,7 +81,7 @@ static int merged_iter_seek(struct merged_iter *mi, struct reftable_record *want
81
82 mi->advance_index = -1;
83
83 - for (size_t i = 0; i < mi->stack_len; i++) {
84 + for (size_t i = 0; i < mi->subiters_len; i++) {
85 err = iterator_seek(&mi->subiters[i].iter, want);
86 if (err < 0)
87 return err;
@@ -193,7 +194,7 @@ static void iterator_from_merged_iter(struct reftable_iterator *it,
194 }
195
196 int reftable_merged_table_new(struct reftable_merged_table **dest,
196 - struct reftable_table *stack, size_t n,
197 + struct reftable_reader **readers, size_t n,
198 uint32_t hash_id)
199 {
200 struct reftable_merged_table *m = NULL;
@@ -201,10 +202,10 @@ int reftable_merged_table_new(struct reftable_merged_table **dest,
202 uint64_t first_min = 0;
203
204 for (size_t i = 0; i < n; i++) {
204 - uint64_t min = reftable_table_min_update_index(&stack[i]);
205 - uint64_t max = reftable_table_max_update_index(&stack[i]);
205 + uint64_t min = reftable_reader_min_update_index(readers[i]);
206 + uint64_t max = reftable_reader_max_update_index(readers[i]);
207
207 - if (reftable_table_hash_id(&stack[i]) != hash_id) {
208 + if (reftable_reader_hash_id(readers[i]) != hash_id) {
209 return REFTABLE_FORMAT_ERROR;
210 }
211 if (i == 0 || min < first_min) {
@@ -216,8 +217,8 @@ int reftable_merged_table_new(struct reftable_merged_table **dest,
217 }
218
219 REFTABLE_CALLOC_ARRAY(m, 1);
219 - m->stack = stack;
220 - m->stack_len = n;
220 + m->readers = readers;
221 + m->readers_len = n;
222 m->min = first_min;
223 m->max = last_max;
224 m->hash_id = hash_id;
@@ -229,7 +230,6 @@ void reftable_merged_table_free(struct reftable_merged_table *mt)
230 {
231 if (!mt)
232 return;
232 - FREE_AND_NULL(mt->stack);
233 reftable_free(mt);
234 }
235
reftable/merged.h
+2 -2
@@ -12,8 +12,8 @@ https://developers.google.com/open-source/licenses/bsd
12 #include "system.h"
13
14 struct reftable_merged_table {
15 - struct reftable_table *stack;
16 - size_t stack_len;
15 + struct reftable_reader **readers;
16 + size_t readers_len;
17 uint32_t hash_id;
18
19 /* If unset, produce deletions. This is useful for compaction. For the
reftable/reader.c
+3 -3
@@ -605,9 +605,9 @@ static void iterator_from_table_iter(struct reftable_iterator *it,
605 it->ops = &table_iter_vtable;
606 }
607
608 -static void reader_init_iter(struct reftable_reader *r,
609 - struct reftable_iterator *it,
610 - uint8_t typ)
608 +void reader_init_iter(struct reftable_reader *r,
609 + struct reftable_iterator *it,
610 + uint8_t typ)
611 {
612 struct reftable_reader_offsets *offs = reader_offsets_for(r, typ);
613
reftable/reader.h
+4
@@ -57,6 +57,10 @@ int init_reader(struct reftable_reader *r, struct reftable_block_source *source,
57 void reader_close(struct reftable_reader *r);
58 const char *reader_name(struct reftable_reader *r);
59
60 +void reader_init_iter(struct reftable_reader *r,
61 + struct reftable_iterator *it,
62 + uint8_t typ);
63 +
64 /* initialize a block reader to read from `r` */
65 int reader_init_block_reader(struct reftable_reader *r, struct block_reader *br,
66 uint64_t next_off, uint8_t want_typ);
reftable/reftable-merged.h
+4 -3
@@ -28,13 +28,14 @@ struct reftable_merged_table;
28
29 /* A generic reftable; see below. */
30 struct reftable_table;
31 +struct reftable_reader;
32
33 /*
33 - * reftable_merged_table_new creates a new merged table. It takes ownership of
34 - * the stack array.
34 + * reftable_merged_table_new creates a new merged table. The readers must be
35 + * kept alive as long as the merged table is still in use.
36 */
37 int reftable_merged_table_new(struct reftable_merged_table **dest,
37 - struct reftable_table *stack, size_t n,
38 + struct reftable_reader **readers, size_t n,
39 uint32_t hash_id);
40
41 /* Initialize a merged table iterator for reading refs. */
reftable/stack.c
+18 -30
@@ -225,13 +225,11 @@ static int reftable_stack_reload_once(struct reftable_stack *st,
225 const char **names,
226 int reuse_open)
227 {
228 - size_t cur_len = !st->merged ? 0 : st->merged->stack_len;
228 + size_t cur_len = !st->merged ? 0 : st->merged->readers_len;
229 struct reftable_reader **cur = stack_copy_readers(st, cur_len);
230 size_t names_len = names_length(names);
231 struct reftable_reader **new_readers =
232 reftable_calloc(names_len, sizeof(*new_readers));
233 - struct reftable_table *new_tables =
234 - reftable_calloc(names_len, sizeof(*new_tables));
233 size_t new_readers_len = 0;
234 struct reftable_merged_table *new_merged = NULL;
235 struct strbuf table_path = STRBUF_INIT;
@@ -267,17 +265,15 @@ static int reftable_stack_reload_once(struct reftable_stack *st,
265 }
266
267 new_readers[new_readers_len] = rd;
270 - reftable_table_from_reader(&new_tables[new_readers_len], rd);
268 new_readers_len++;
269 }
270
271 /* success! */
275 - err = reftable_merged_table_new(&new_merged, new_tables,
272 + err = reftable_merged_table_new(&new_merged, new_readers,
273 new_readers_len, st->opts.hash_id);
274 if (err < 0)
275 goto done;
276
280 - new_tables = NULL;
277 st->readers_len = new_readers_len;
278 if (st->merged)
279 reftable_merged_table_free(st->merged);
@@ -309,7 +305,6 @@ done:
305 reftable_reader_free(new_readers[i]);
306 }
307 reftable_free(new_readers);
312 - reftable_free(new_tables);
308 reftable_free(cur);
309 strbuf_release(&table_path);
310 return err;
@@ -520,7 +515,7 @@ static int stack_uptodate(struct reftable_stack *st)
515 }
516 }
517
523 - if (names[st->merged->stack_len]) {
518 + if (names[st->merged->readers_len]) {
519 err = 1;
520 goto done;
521 }
@@ -659,7 +654,7 @@ int reftable_addition_commit(struct reftable_addition *add)
654 if (add->new_tables_len == 0)
655 goto done;
656
662 - for (i = 0; i < add->stack->merged->stack_len; i++) {
657 + for (i = 0; i < add->stack->merged->readers_len; i++) {
658 strbuf_addstr(&table_list, add->stack->readers[i]->name);
659 strbuf_addstr(&table_list, "\n");
660 }
@@ -839,7 +834,7 @@ done:
834
835 uint64_t reftable_stack_next_update_index(struct reftable_stack *st)
836 {
842 - int sz = st->merged->stack_len;
837 + int sz = st->merged->readers_len;
838 if (sz > 0)
839 return reftable_reader_max_update_index(st->readers[sz - 1]) +
840 1;
@@ -906,30 +901,23 @@ static int stack_write_compact(struct reftable_stack *st,
901 size_t first, size_t last,
902 struct reftable_log_expiry_config *config)
903 {
909 - size_t subtabs_len = last - first + 1;
910 - struct reftable_table *subtabs = reftable_calloc(
911 - last - first + 1, sizeof(*subtabs));
904 struct reftable_merged_table *mt = NULL;
905 struct reftable_iterator it = { NULL };
906 struct reftable_ref_record ref = { NULL };
907 struct reftable_log_record log = { NULL };
908 + size_t subtabs_len = last - first + 1;
909 uint64_t entries = 0;
910 int err = 0;
911
919 - for (size_t i = first, j = 0; i <= last; i++) {
920 - struct reftable_reader *t = st->readers[i];
921 - reftable_table_from_reader(&subtabs[j++], t);
922 - st->stats.bytes += t->size;
923 - }
912 + for (size_t i = first; i <= last; i++)
913 + st->stats.bytes += st->readers[i]->size;
914 reftable_writer_set_limits(wr, st->readers[first]->min_update_index,
915 st->readers[last]->max_update_index);
916
927 - err = reftable_merged_table_new(&mt, subtabs, subtabs_len,
917 + err = reftable_merged_table_new(&mt, st->readers + first, subtabs_len,
918 st->opts.hash_id);
929 - if (err < 0) {
930 - reftable_free(subtabs);
919 + if (err < 0)
920 goto done;
932 - }
921
922 merged_table_init_iter(mt, &it, BLOCK_TYPE_REF);
923 err = reftable_iterator_seek_ref(&it, "");
@@ -1207,7 +1195,7 @@ static int stack_compact_range(struct reftable_stack *st,
1195 * have compacted them.
1196 */
1197 for (size_t j = 1; j < last - first + 1; j++) {
1210 - const char *old = first + j < st->merged->stack_len ?
1198 + const char *old = first + j < st->merged->readers_len ?
1199 st->readers[first + j]->name : NULL;
1200 const char *new = names[i + j];
1201
@@ -1248,10 +1236,10 @@ static int stack_compact_range(struct reftable_stack *st,
1236 * `fd_read_lines()` uses a `NULL` sentinel to indicate that
1237 * the array is at its end. As we use `free_names()` to free
1238 * the array, we need to include this sentinel value here and
1251 - * thus have to allocate `stack_len + 1` many entries.
1239 + * thus have to allocate `readers_len + 1` many entries.
1240 */
1253 - REFTABLE_CALLOC_ARRAY(names, st->merged->stack_len + 1);
1254 - for (size_t i = 0; i < st->merged->stack_len; i++)
1241 + REFTABLE_CALLOC_ARRAY(names, st->merged->readers_len + 1);
1242 + for (size_t i = 0; i < st->merged->readers_len; i++)
1243 names[i] = xstrdup(st->readers[i]->name);
1244 first_to_replace = first;
1245 last_to_replace = last;
@@ -1358,7 +1346,7 @@ static int stack_compact_range_stats(struct reftable_stack *st,
1346 int reftable_stack_compact_all(struct reftable_stack *st,
1347 struct reftable_log_expiry_config *config)
1348 {
1361 - size_t last = st->merged->stack_len ? st->merged->stack_len - 1 : 0;
1349 + size_t last = st->merged->readers_len ? st->merged->readers_len - 1 : 0;
1350 return stack_compact_range_stats(st, 0, last, config, 0);
1351 }
1352
@@ -1449,9 +1437,9 @@ static uint64_t *stack_table_sizes_for_compaction(struct reftable_stack *st)
1437 int overhead = header_size(version) - 1;
1438 uint64_t *sizes;
1439
1452 - REFTABLE_CALLOC_ARRAY(sizes, st->merged->stack_len);
1440 + REFTABLE_CALLOC_ARRAY(sizes, st->merged->readers_len);
1441
1454 - for (size_t i = 0; i < st->merged->stack_len; i++)
1442 + for (size_t i = 0; i < st->merged->readers_len; i++)
1443 sizes[i] = st->readers[i]->size - overhead;
1444
1445 return sizes;
@@ -1461,7 +1449,7 @@ int reftable_stack_auto_compact(struct reftable_stack *st)
1449 {
1450 uint64_t *sizes = stack_table_sizes_for_compaction(st);
1451 struct segment seg =
1464 - suggest_compaction_segment(sizes, st->merged->stack_len,
1452 + suggest_compaction_segment(sizes, st->merged->readers_len,
1453 st->opts.auto_compaction_factor);
1454 reftable_free(sizes);
1455 if (segment_size(&seg) > 0)
reftable/stack_test.c
+11 -11
@@ -347,9 +347,9 @@ static void test_reftable_stack_transaction_api_performs_auto_compaction(void)
347 * all tables in the stack.
348 */
349 if (i != n)
350 - EXPECT(st->merged->stack_len == i + 1);
350 + EXPECT(st->merged->readers_len == i + 1);
351 else
352 - EXPECT(st->merged->stack_len == 1);
352 + EXPECT(st->merged->readers_len == 1);
353 }
354
355 reftable_stack_destroy(st);
@@ -375,7 +375,7 @@ static void test_reftable_stack_auto_compaction_fails_gracefully(void)
375
376 err = reftable_stack_add(st, write_test_ref, &ref);
377 EXPECT_ERR(err);
378 - EXPECT(st->merged->stack_len == 1);
378 + EXPECT(st->merged->readers_len == 1);
379 EXPECT(st->stats.attempts == 0);
380 EXPECT(st->stats.failures == 0);
381
@@ -390,7 +390,7 @@ static void test_reftable_stack_auto_compaction_fails_gracefully(void)
390 ref.update_index = 2;
391 err = reftable_stack_add(st, write_test_ref, &ref);
392 EXPECT_ERR(err);
393 - EXPECT(st->merged->stack_len == 2);
393 + EXPECT(st->merged->readers_len == 2);
394 EXPECT(st->stats.attempts == 1);
395 EXPECT(st->stats.failures == 1);
396
@@ -881,7 +881,7 @@ static void test_reftable_stack_auto_compaction(void)
881
882 err = reftable_stack_auto_compact(st);
883 EXPECT_ERR(err);
884 - EXPECT(i < 3 || st->merged->stack_len < 2 * fastlog2(i));
884 + EXPECT(i < 3 || st->merged->readers_len < 2 * fastlog2(i));
885 }
886
887 EXPECT(reftable_stack_compaction_stats(st)->entries_written <
@@ -905,7 +905,7 @@ static void test_reftable_stack_auto_compaction_with_locked_tables(void)
905 EXPECT_ERR(err);
906
907 write_n_ref_tables(st, 5);
908 - EXPECT(st->merged->stack_len == 5);
908 + EXPECT(st->merged->readers_len == 5);
909
910 /*
911 * Given that all tables we have written should be roughly the same
@@ -925,7 +925,7 @@ static void test_reftable_stack_auto_compaction_with_locked_tables(void)
925 err = reftable_stack_auto_compact(st);
926 EXPECT_ERR(err);
927 EXPECT(st->stats.failures == 0);
928 - EXPECT(st->merged->stack_len == 4);
928 + EXPECT(st->merged->readers_len == 4);
929
930 reftable_stack_destroy(st);
931 strbuf_release(&buf);
@@ -970,9 +970,9 @@ static void test_reftable_stack_add_performs_auto_compaction(void)
970 * all tables in the stack.
971 */
972 if (i != n)
973 - EXPECT(st->merged->stack_len == i + 1);
973 + EXPECT(st->merged->readers_len == i + 1);
974 else
975 - EXPECT(st->merged->stack_len == 1);
975 + EXPECT(st->merged->readers_len == 1);
976 }
977
978 reftable_stack_destroy(st);
@@ -994,7 +994,7 @@ static void test_reftable_stack_compaction_with_locked_tables(void)
994 EXPECT_ERR(err);
995
996 write_n_ref_tables(st, 3);
997 - EXPECT(st->merged->stack_len == 3);
997 + EXPECT(st->merged->readers_len == 3);
998
999 /* Lock one of the tables that we're about to compact. */
1000 strbuf_reset(&buf);
@@ -1008,7 +1008,7 @@ static void test_reftable_stack_compaction_with_locked_tables(void)
1008 err = reftable_stack_compact_all(st, NULL);
1009 EXPECT(err == REFTABLE_LOCK_ERROR);
1010 EXPECT(st->stats.failures == 1);
1011 - EXPECT(st->merged->stack_len == 3);
1011 + EXPECT(st->merged->readers_len == 3);
1012
1013 reftable_stack_destroy(st);
1014 strbuf_release(&buf);
t/unit-tests/t-reftable-merged.c
+4 -12
@@ -94,10 +94,8 @@ merged_table_from_records(struct reftable_ref_record **refs,
94 struct strbuf *buf, const size_t n)
95 {
96 struct reftable_merged_table *mt = NULL;
97 - struct reftable_table *tabs;
97 int err;
98
100 - REFTABLE_CALLOC_ARRAY(tabs, n);
99 REFTABLE_CALLOC_ARRAY(*readers, n);
100 REFTABLE_CALLOC_ARRAY(*source, n);
101
@@ -108,10 +106,9 @@ merged_table_from_records(struct reftable_ref_record **refs,
106 err = reftable_new_reader(&(*readers)[i], &(*source)[i],
107 "name");
108 check(!err);
111 - reftable_table_from_reader(&tabs[i], (*readers)[i]);
109 }
110
114 - err = reftable_merged_table_new(&mt, tabs, n, GIT_SHA1_FORMAT_ID);
111 + err = reftable_merged_table_new(&mt, *readers, n, GIT_SHA1_FORMAT_ID);
112 check(!err);
113 return mt;
114 }
@@ -272,10 +269,8 @@ merged_table_from_log_records(struct reftable_log_record **logs,
269 struct strbuf *buf, const size_t n)
270 {
271 struct reftable_merged_table *mt = NULL;
275 - struct reftable_table *tabs;
272 int err;
273
278 - REFTABLE_CALLOC_ARRAY(tabs, n);
274 REFTABLE_CALLOC_ARRAY(*readers, n);
275 REFTABLE_CALLOC_ARRAY(*source, n);
276
@@ -286,10 +281,9 @@ merged_table_from_log_records(struct reftable_log_record **logs,
281 err = reftable_new_reader(&(*readers)[i], &(*source)[i],
282 "name");
283 check(!err);
289 - reftable_table_from_reader(&tabs[i], (*readers)[i]);
284 }
285
292 - err = reftable_merged_table_new(&mt, tabs, n, GIT_SHA1_FORMAT_ID);
286 + err = reftable_merged_table_new(&mt, *readers, n, GIT_SHA1_FORMAT_ID);
287 check(!err);
288 return mt;
289 }
@@ -418,7 +412,6 @@ static void t_default_write_opts(void)
412 };
413 int err;
414 struct reftable_block_source source = { 0 };
421 - struct reftable_table *tab = reftable_calloc(1, sizeof(*tab));
415 uint32_t hash_id;
416 struct reftable_reader *rd = NULL;
417 struct reftable_merged_table *merged = NULL;
@@ -440,10 +433,9 @@ static void t_default_write_opts(void)
433 hash_id = reftable_reader_hash_id(rd);
434 check_int(hash_id, ==, GIT_SHA1_FORMAT_ID);
435
443 - reftable_table_from_reader(&tab[0], rd);
444 - err = reftable_merged_table_new(&merged, tab, 1, GIT_SHA256_FORMAT_ID);
436 + err = reftable_merged_table_new(&merged, &rd, 1, GIT_SHA256_FORMAT_ID);
437 check_int(err, ==, REFTABLE_FORMAT_ERROR);
446 - err = reftable_merged_table_new(&merged, tab, 1, GIT_SHA1_FORMAT_ID);
438 + err = reftable_merged_table_new(&merged, &rd, 1, GIT_SHA1_FORMAT_ID);
439 check(!err);
440
441 reftable_reader_free(rd);