reftable: drop generic `reftable_table` interface
cover reftable: drop generic `reftable_table` interface
Hi,
this patch series performs some simplification of the reftable library by dropping the generic reftable_table interface. The intent of this interface is to abstract away whether the underlying table is either a single reftable, or a merged reftable. Arguably though, it is not needed anymore starting with my recent refactorings [1] that made the reftable iterator itself seekable, as that now provides such a generic interface already.
So this patch series rips out the reftable_table interface, both to remove unused clutter, but more importantly to make the reftable library easier to understand overall. There is also a tiny performance gain of 1-3% by requiring one less vtable function call. But while that speedup is consistent, I didn't include any benchmarks as it is rather close to noise and not the primary motivation of this patch series.
Patrick
[1]: <[email protected]>
Patrick Steinhardt (10):
reftable/merged: expose functions to initialize iterators reftable/merged: rename reftable_new_merged_table() reftable/merged: stop using generic tables in the merged table reftable/stack: open-code reading refs
reftable/iter: drop double-checking logic reftable/generic: move generic iterator code into iterator interface reftable/dump: drop unused compact_stack() reftable/dump: drop unused printing functionality reftable/dump: move code into "t/helper/test-reftable.c" reftable/generic: drop interface
Makefile | 2 - reftable/dump.c | 111 --------------- reftable/generic.c | 229 ------------------------------- reftable/generic.h | 37 ----- reftable/iter.c | 127 ++++++++++++++--- reftable/iter.h | 30 +++- reftable/merged.c | 72 ++++------ reftable/merged.h | 4 +- reftable/reader.c | 70 +--------- reftable/reader.h | 4 + reftable/record.c | 127 ----------------- reftable/record.h | 4 - reftable/reftable-generic.h | 47 ------- reftable/reftable-merged.h | 26 ++-- reftable/reftable-reader.h | 9 -- reftable/reftable-record.h | 8 -- reftable/reftable-stack.h | 3 - reftable/stack.c | 94 ++++++------- reftable/stack_test.c | 29 ++-- t/helper/test-reftable.c | 47 ++++++- t/unit-tests/t-reftable-merged.c | 17 +-- 21 files changed, 281 insertions(+), 816 deletions(-) delete mode 100644 reftable/dump.c delete mode 100644 reftable/generic.c delete mode 100644 reftable/generic.h delete mode 100644 reftable/reftable-generic.h
Signature · 1 line
2.46.0.46.g406f326d27.dirty
PATCH 1/10 reftable/merged: expose functions to initialize iterators
We do not expose any functions via our public headers that would allow a caller to initialize a reftable iterator from a merged table. Instead, they are expected to go via the generic reftable_table interface, which is somewhat roundabout.
Implement two new functions to initialize iterators for ref and log records to plug this gap.
--- reftable/merged.c | 12 ++++++++++++ reftable/reftable-merged.h | 8 ++++++++ 2 files changed, 20 insertions(+)
reftable/merged.c +12 -0
@@ -254,6 +254,18 @@ void merged_table_init_iter(struct reftable_merged_table *mt,iterator_from_merged_iter (it, mi); } +void reftable_merged_table_init_ref_iterator (struct reftable_merged_table *mt, +struct reftable_iterator *it) +{ +merged_table_init_iter (mt, it,BLOCK_TYPE_REF ); +} + +void reftable_merged_table_init_log_iterator (struct reftable_merged_table *mt, +struct reftable_iterator *it) +{ +merged_table_init_iter (mt, it,BLOCK_TYPE_LOG ); +} +uint32_t reftable_merged_table_hash_id (struct reftable_merged_table *mt) {return mt->hash_id;
reftable/reftable-merged.h +8 -0
@@ -36,6 +36,14 @@ int reftable_new_merged_table(struct reftable_merged_table **dest, struct reftable_table *stack, size_t n, uint32_t hash_id); +/* Initialize a merged table iterator for reading refs. */ +void reftable_merged_table_init_ref_iterator (struct reftable_merged_table *mt, +struct reftable_iterator *it); + +/* Initialize a merged table iterator for reading logs. */ +void reftable_merged_table_init_log_iterator (struct reftable_merged_table *mt, +struct reftable_iterator *it); +/* returns the max update_index covered by this merged table. */ uint64_t reftable_merged_table_max_update_index (struct reftable_merged_table *mt);
Signature · 1 line
2.46.0.46.g406f326d27.dirty
PATCH 2/10 reftable/merged: rename `reftable_new_merged_table()`
In reply to Patrick SteinhardtRename reftable_new_merged_table() to reftable_merged_table_new() such that the name matches our coding style.
--- reftable/merged.c | 2 +- reftable/reftable-merged.h | 9 +++++---- reftable/stack.c | 4 ++-- t/unit-tests/t-reftable-merged.c | 8 ++++---- 4 files changed, 12 insertions(+), 11 deletions(-)
reftable/merged.c +1 -1
@@ -192,7 +192,7 @@ static void iterator_from_merged_iter(struct reftable_iterator *it, it->ops = &merged_iter_vtable; } -int reftable_new_merged_table (struct reftable_merged_table **dest, +int reftable_merged_table_new (struct reftable_merged_table **dest,struct reftable_table *stack,size_t n,uint32_t hash_id) {
reftable/reftable-merged.h +5 -4
@@ -29,10 +29,11 @@ struct reftable_merged_table;/* A generic reftable; see below. */ struct reftable_table ; -/* reftable_new_merged_table creates a new merged table. It takes ownership of -the stack array. -*/ -int reftable_new_merged_table (struct reftable_merged_table **dest, +/* +* reftable_merged_table_new creates a new merged table. It takes ownership of +* the stack array. +*/ +int reftable_merged_table_new (struct reftable_merged_table **dest,struct reftable_table *stack,size_t n,uint32_t hash_id);
reftable/stack.c +2 -2
@@ -272,7 +272,7 @@ static int reftable_stack_reload_once(struct reftable_stack *st, }/* success! */ - err =reftable_new_merged_table (&new_merged, new_tables, + err =reftable_merged_table_new (&new_merged, new_tables, new_readers_len, st->opts.hash_id);if (err <0 ) goto done;
@@ -924,7 +924,7 @@ static int stack_write_compact(struct reftable_stack *st,reftable_writer_set_limits (wr, st->readers[first]->min_update_index, st->readers[last]->max_update_index); - err =reftable_new_merged_table (&mt, subtabs, subtabs_len, + err =reftable_merged_table_new (&mt, subtabs, subtabs_len, st->opts.hash_id);if (err <0 ) {reftable_free (subtabs);
t/unit-tests/t-reftable-merged.c +4 -4
@@ -111,7 +111,7 @@ merged_table_from_records(struct reftable_ref_record **refs,reftable_table_from_reader (&tabs[i], (*readers)[i]); } - err =reftable_new_merged_table (&mt, tabs, n,GIT_SHA1_FORMAT_ID ); + err =reftable_merged_table_new (&mt, tabs, n,GIT_SHA1_FORMAT_ID );check (!err);return mt; }
@@ -289,7 +289,7 @@ merged_table_from_log_records(struct reftable_log_record **logs,reftable_table_from_reader (&tabs[i], (*readers)[i]); } - err =reftable_new_merged_table (&mt, tabs, n,GIT_SHA1_FORMAT_ID ); + err =reftable_merged_table_new (&mt, tabs, n,GIT_SHA1_FORMAT_ID );check (!err);return mt; }
@@ -441,9 +441,9 @@ static void t_default_write_opts(void) check_int(hash_id, ==, GIT_SHA1_FORMAT_ID);reftable_table_from_reader (&tab[0 ], rd); - err =reftable_new_merged_table (&merged, tab,1 ,GIT_SHA256_FORMAT_ID ); + err =reftable_merged_table_new (&merged, tab,1 ,GIT_SHA256_FORMAT_ID ); check_int(err, ==, REFTABLE_FORMAT_ERROR); - err =reftable_new_merged_table (&merged, tab,1 ,GIT_SHA1_FORMAT_ID ); + err =reftable_merged_table_new (&merged, tab,1 ,GIT_SHA1_FORMAT_ID );check (!err);reftable_reader_free (rd);
Signature · 1 line
2.46.0.46.g406f326d27.dirty
PATCH 3/10 reftable/merged: stop using generic tables in the merged table
In reply to Patrick SteinhardtThe 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.
--- reftable/merged.c | 28 +++++++++---------- reftable/merged.h | 4 +-- reftable/reader.c | 6 ++-- reftable/reader.h | 4 +++ reftable/reftable-merged.h | 7 +++-- reftable/stack.c | 48 ++++++++++++-------------------- reftable/stack_test.c | 22 +++++++-------- t/unit-tests/t-reftable-merged.c | 16 +++-------- 8 files changed, 60 insertions(+), 75 deletions(-)
reftable/merged.c +14 -14
@@ -11,6 +11,7 @@ license that can be found in the LICENSE file or at#include "constants.h" #include "iter.h" #include "pq.h" +#include "reader.h" #include "record.h" #include "generic.h" #include "reftable-merged.h"
@@ -25,7 +26,7 @@ struct merged_subiter {struct merged_iter {struct merged_subiter *subiters;struct merged_iter_pqueue pq; -size_t stack_len; +size_t subiters_len;int suppress_deletions;ssize_t advance_index; };
@@ -38,12 +39,12 @@ static void merged_iter_init(struct merged_iter *mi, mi->advance_index =-1 ; mi->suppress_deletions = mt->suppress_deletions; -REFTABLE_CALLOC_ARRAY (mi->subiters, mt->stack_len); -for (size_t i =0 ; i < mt->stack_len; i++) { +REFTABLE_CALLOC_ARRAY (mi->subiters, mt->readers_len); +for (size_t i =0 ; i < mt->readers_len; i++) {reftable_record_init (&mi->subiters[i].rec, typ); -table_init_iter (&mt->stack[i], &mi->subiters[i].iter, typ); +reader_init_iter (mt->readers[i], &mi->subiters[i].iter, typ); } - mi->stack_len = mt->stack_len; + mi->subiters_len = mt->readers_len; }static void merged_iter_close (void *p)
@@ -51,7 +52,7 @@ static void merged_iter_close(void *p)struct merged_iter *mi = p;merged_iter_pqueue_release (&mi->pq); -for (size_t i =0 ; i < mi->stack_len; i++) { +for (size_t i =0 ; i < mi->subiters_len; i++) {reftable_iterator_destroy (&mi->subiters[i].iter);reftable_record_release (&mi->subiters[i].rec); }
@@ -80,7 +81,7 @@ static int merged_iter_seek(struct merged_iter *mi, struct reftable_record *want mi->advance_index =-1 ; -for (size_t i =0 ; i < mi->stack_len; i++) { +for (size_t i =0 ; i < mi->subiters_len; i++) { err =iterator_seek (&mi->subiters[i].iter, want);if (err <0 )return err;
@@ -193,7 +194,7 @@ static void iterator_from_merged_iter(struct reftable_iterator *it, }int reftable_merged_table_new (struct reftable_merged_table **dest, -struct reftable_table *stack,size_t n, +struct reftable_reader **readers,size_t n,uint32_t hash_id) {struct reftable_merged_table *m =NULL ;
@@ -201,10 +202,10 @@ int reftable_merged_table_new(struct reftable_merged_table **dest,uint64_t first_min =0 ;for (size_t i =0 ; i < n; i++) { -uint64_t min =reftable_table_min_update_index (&stack[i]); -uint64_t max =reftable_table_max_update_index (&stack[i]); +uint64_t min =reftable_reader_min_update_index (readers[i]); +uint64_t max =reftable_reader_max_update_index (readers[i]); -if (reftable_table_hash_id (&stack[i]) != hash_id) { +if (reftable_reader_hash_id (readers[i]) != hash_id) {return REFTABLE_FORMAT_ERROR ; }if (i ==0 || min < first_min) {
@@ -216,8 +217,8 @@ int reftable_merged_table_new(struct reftable_merged_table **dest, }REFTABLE_CALLOC_ARRAY (m,1 ); - m->stack = stack; - m->stack_len = n; + m->readers = readers; + m->readers_len = n; m->min = first_min; m->max = last_max; m->hash_id = hash_id;
@@ -229,7 +230,6 @@ void reftable_merged_table_free(struct reftable_merged_table *mt) {if (!mt)return ; -FREE_AND_NULL (mt->stack);reftable_free (mt); }
reftable/merged.h +2 -2
@@ -12,8 +12,8 @@ license that can be found in the LICENSE file or at#include "system.h" struct reftable_merged_table { -struct reftable_table *stack; -size_t stack_len; +struct reftable_reader **readers; +size_t readers_len;uint32_t hash_id;/* 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, it->ops = &table_iter_vtable; } -static void reader_init_iter (struct reftable_reader *r, -struct reftable_iterator *it, -uint8_t typ) +void reader_init_iter (struct reftable_reader *r, +struct reftable_iterator *it, +uint8_t typ) {struct reftable_reader_offsets *offs =reader_offsets_for (r, typ);
reftable/reader.h +4 -0
@@ -57,6 +57,10 @@ int init_reader(struct reftable_reader *r, struct reftable_block_source *source,void reader_close (struct reftable_reader *r);const char *reader_name (struct reftable_reader *r); +void reader_init_iter (struct reftable_reader *r, +struct reftable_iterator *it, +uint8_t typ); +/* initialize a block reader to read from `r` */ int reader_init_block_reader (struct reftable_reader *r,struct block_reader *br,uint64_t next_off,uint8_t want_typ);
reftable/reftable-merged.h +4 -3
@@ -28,13 +28,14 @@ struct reftable_merged_table;/* A generic reftable; see below. */ struct reftable_table ; +struct reftable_reader ;/* -* reftable_merged_table_new creates a new merged table. It takes ownership of -* the stack array. +* reftable_merged_table_new creates a new merged table. The readers must be +* kept alive as long as the merged table is still in use. */ int reftable_merged_table_new (struct reftable_merged_table **dest, -struct reftable_table *stack,size_t n, +struct reftable_reader **readers,size_t n,uint32_t hash_id);/* Initialize a merged table iterator for reading refs. */
reftable/stack.c +18 -30
146 lines not on this page · raw message
reftable/stack_test.c +11 -11
@@ -347,9 +347,9 @@ static void test_reftable_stack_transaction_api_performs_auto_compaction(void)* all tables in the stack. */ if (i != n) -EXPECT (st->merged->stack_len == i +1 ); +EXPECT (st->merged->readers_len == i +1 );else -EXPECT (st->merged->stack_len ==1 ); +EXPECT (st->merged->readers_len ==1 ); }reftable_stack_destroy (st);
@@ -375,7 +375,7 @@ static void test_reftable_stack_auto_compaction_fails_gracefully(void) err =reftable_stack_add (st, write_test_ref, &ref);EXPECT_ERR (err); -EXPECT (st->merged->stack_len ==1 ); +EXPECT (st->merged->readers_len ==1 );EXPECT (st->stats.attempts ==0 );EXPECT (st->stats.failures ==0 );
@@ -390,7 +390,7 @@ static void test_reftable_stack_auto_compaction_fails_gracefully(void) ref.update_index =2 ; err =reftable_stack_add (st, write_test_ref, &ref);EXPECT_ERR (err); -EXPECT (st->merged->stack_len ==2 ); +EXPECT (st->merged->readers_len ==2 );EXPECT (st->stats.attempts ==1 );EXPECT (st->stats.failures ==1 );
@@ -881,7 +881,7 @@ static void test_reftable_stack_auto_compaction(void) err =reftable_stack_auto_compact (st);EXPECT_ERR (err); -EXPECT (i <3 || st->merged->stack_len <2 *fastlog2 (i)); +EXPECT (i <3 || st->merged->readers_len <2 *fastlog2 (i)); } EXPECT(reftable_stack_compaction_stats(st)->entries_written <
@@ -905,7 +905,7 @@ static void test_reftable_stack_auto_compaction_with_locked_tables(void)EXPECT_ERR (err);write_n_ref_tables (st,5 ); -EXPECT (st->merged->stack_len ==5 ); +EXPECT (st->merged->readers_len ==5 );/* * 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) err =reftable_stack_auto_compact (st);EXPECT_ERR (err);EXPECT (st->stats.failures ==0 ); -EXPECT (st->merged->stack_len ==4 ); +EXPECT (st->merged->readers_len ==4 );reftable_stack_destroy (st);strbuf_release (&buf);
@@ -970,9 +970,9 @@ static void test_reftable_stack_add_performs_auto_compaction(void)* all tables in the stack. */ if (i != n) -EXPECT (st->merged->stack_len == i +1 ); +EXPECT (st->merged->readers_len == i +1 );else -EXPECT (st->merged->stack_len ==1 ); +EXPECT (st->merged->readers_len ==1 ); }reftable_stack_destroy (st);
@@ -994,7 +994,7 @@ static void test_reftable_stack_compaction_with_locked_tables(void)EXPECT_ERR (err);write_n_ref_tables (st,3 ); -EXPECT (st->merged->stack_len ==3 ); +EXPECT (st->merged->readers_len ==3 );/* Lock one of the tables that we're about to compact. */ strbuf_reset (&buf);
@@ -1008,7 +1008,7 @@ static void test_reftable_stack_compaction_with_locked_tables(void) err =reftable_stack_compact_all (st,NULL );EXPECT (err ==REFTABLE_LOCK_ERROR );EXPECT (st->stats.failures ==1 ); -EXPECT (st->merged->stack_len ==3 ); +EXPECT (st->merged->readers_len ==3 );reftable_stack_destroy (st);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, struct strbuf *buf, const size_t n) {struct reftable_merged_table *mt =NULL ; -struct reftable_table *tabs;int err; -REFTABLE_CALLOC_ARRAY (tabs, n);REFTABLE_CALLOC_ARRAY (*readers, n);REFTABLE_CALLOC_ARRAY (*source, n);
@@ -108,10 +106,9 @@ merged_table_from_records(struct reftable_ref_record **refs, err =reftable_new_reader (&(*readers)[i], &(*source)[i],"name" );check (!err); -reftable_table_from_reader (&tabs[i], (*readers)[i]); } - err =reftable_merged_table_new (&mt, tabs, n,GIT_SHA1_FORMAT_ID ); + err =reftable_merged_table_new (&mt, *readers, n,GIT_SHA1_FORMAT_ID );check (!err);return mt; }
@@ -272,10 +269,8 @@ merged_table_from_log_records(struct reftable_log_record **logs, struct strbuf *buf, const size_t n) {struct reftable_merged_table *mt =NULL ; -struct reftable_table *tabs;int err; -REFTABLE_CALLOC_ARRAY (tabs, n);REFTABLE_CALLOC_ARRAY (*readers, n);REFTABLE_CALLOC_ARRAY (*source, n);
@@ -286,10 +281,9 @@ merged_table_from_log_records(struct reftable_log_record **logs, err =reftable_new_reader (&(*readers)[i], &(*source)[i],"name" );check (!err); -reftable_table_from_reader (&tabs[i], (*readers)[i]); } - err =reftable_merged_table_new (&mt, tabs, n,GIT_SHA1_FORMAT_ID ); + err =reftable_merged_table_new (&mt, *readers, n,GIT_SHA1_FORMAT_ID );check (!err);return mt; }
@@ -418,7 +412,6 @@ static void t_default_write_opts(void) };int err;struct reftable_block_source source = {0 }; -struct reftable_table *tab =reftable_calloc (1 ,sizeof (*tab));uint32_t hash_id;struct reftable_reader *rd =NULL ;struct reftable_merged_table *merged =NULL ;
@@ -440,10 +433,9 @@ static void t_default_write_opts(void) hash_id =reftable_reader_hash_id (rd); check_int(hash_id, ==, GIT_SHA1_FORMAT_ID); -reftable_table_from_reader (&tab[0 ], rd); - err =reftable_merged_table_new (&merged, tab,1 ,GIT_SHA256_FORMAT_ID ); + err =reftable_merged_table_new (&merged, &rd,1 ,GIT_SHA256_FORMAT_ID ); check_int(err, ==, REFTABLE_FORMAT_ERROR); - err =reftable_merged_table_new (&merged, tab,1 ,GIT_SHA1_FORMAT_ID ); + err =reftable_merged_table_new (&merged, &rd,1 ,GIT_SHA1_FORMAT_ID );check (!err);reftable_reader_free (rd);
Signature · 1 line
2.46.0.46.g406f326d27.dirty
PATCH 4/10 reftable/stack: open-code reading refs
In reply to Patrick SteinhardtTo read a reference for the reftable stack, we first create a generic reftable_table from the merged table and then read the reference via a convenience function. We are about to remove these generic interfaces, so let's instead open-code the logic to prepare for this removal.
--- reftable/stack.c | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-)
reftable/stack.c +22 -3
@@ -1468,9 +1468,28 @@ reftable_stack_compaction_stats(struct reftable_stack *st)int reftable_stack_read_ref (struct reftable_stack *st,const char *refname,struct reftable_ref_record *ref) { -struct reftable_table tab = {NULL }; -reftable_table_from_merged_table (&tab,reftable_stack_merged_table (st)); -return reftable_table_read_ref (&tab, refname, ref); +struct reftable_iterator it = {0 }; +int ret; + +reftable_merged_table_init_ref_iterator (st->merged, &it); + ret =reftable_iterator_seek_ref (&it, refname); +if (ret) + goto out; + + ret =reftable_iterator_next_ref (&it, ref); +if (ret) + goto out; + +if (strcmp (ref->refname, refname) || +reftable_ref_record_is_deletion (ref)) { +reftable_ref_record_release (ref); + ret =1 ; + goto out; + } + +out: +reftable_iterator_destroy (&it); +return ret; } int reftable_stack_read_log(struct reftable_stack *st, const char *refname,
Signature · 1 line
2.46.0.46.g406f326d27.dirty
PATCH 5/10 reftable/iter: drop double-checking logic
In reply to Patrick SteinhardtThe filtering ref iterator can be used to only yield refs which are not in a specific skip list. This iterator has an option to double-check the results it returns, which causes us to seek tho reference we are about to yield via a separate table such that we detect whether the reference that the first iterator has yielded actually exists.
The value of this is somewhat dubious, and I cannot think of any usecase where this functionality should be required. Furthermore, this option is never set in our codebase, which means that it is essentially untested. And last but not least, the struct reftable_table that is used to implement it is about to go away.
So while we could refactor the code to not use a reftable_table, it very much feels like a wasted effort. Let's just drop this code.
--- reftable/iter.c | 20 -------------------- reftable/iter.h | 2 -- reftable/reader.c | 2 -- 3 files changed, 24 deletions(-)
reftable/iter.c +0 -20
@@ -42,26 +42,6 @@ static int filtering_ref_iterator_next(void *iter_arg,break ; } -if (fri->double_check) { -struct reftable_iterator it = {NULL }; - -reftable_table_init_ref_iter (&fri->tab, &it); - - err =reftable_iterator_seek_ref (&it, ref->refname); -if (err ==0 ) - err =reftable_iterator_next_ref (&it, ref); - -reftable_iterator_destroy (&it); - -if (err <0 ) { -break ; - } - -if (err >0 ) { -continue ; - } - } - if (ref->value_type == REFTABLE_REF_VAL2 && (!memcmp(fri->oid.buf, ref->value.val2.target_value, fri->oid.len) ||
reftable/iter.h +0 -2
@@ -18,8 +18,6 @@ license that can be found in the LICENSE file or at/* iterator that produces only ref records that point to `oid` */ struct filtering_ref_iterator { -int double_check; -struct reftable_table tab;struct strbuf oid;struct reftable_iterator it; };
reftable/reader.c +0 -2
@@ -735,8 +735,6 @@ static int reftable_reader_refs_for_unindexed(struct reftable_reader *r, *filter = empty;strbuf_add (&filter->oid, oid, oid_len); -reftable_table_from_reader (&filter->tab, r); - filter->double_check =0 ;iterator_from_table_iter (&filter->it, ti);iterator_from_filtering_ref_iterator (it, filter);
Signature · 1 line
2.46.0.46.g406f326d27.dirty
PATCH 6/10 reftable/generic: move generic iterator code into iterator interface
In reply to Patrick SteinhardtMove functions relating to the reftable iterator from "generic.c" into "iter.c". This prepares for the removal of the former subsystem.
--- reftable/generic.c | 107 +-------------------------------------------- reftable/generic.h | 10 ----- reftable/iter.c | 106 ++++++++++++++++++++++++++++++++++++++++++++ reftable/iter.h | 27 ++++++++++++ 4 files changed, 134 insertions(+), 116 deletions(-)
reftable/generic.c +1 -106
@@ -9,6 +9,7 @@ license that can be found in the LICENSE file or at#include "constants.h" #include "record.h" #include "generic.h" +#include "iter.h" #include "reftable-iterator.h" #include "reftable-generic.h"
@@ -32,37 +33,6 @@ void reftable_table_init_log_iter(struct reftable_table *tab,table_init_iter (tab, it,BLOCK_TYPE_LOG ); } -int reftable_iterator_seek_ref (struct reftable_iterator *it, -const char *name) -{ -struct reftable_record want = { - .type =BLOCK_TYPE_REF , - .u.ref = { - .refname = (char *)name, - }, - }; -return it->ops->seek (it->iter_arg, &want); -} - -int reftable_iterator_seek_log_at (struct reftable_iterator *it, -const char *name,uint64_t update_index) -{ -struct reftable_record want = { - .type =BLOCK_TYPE_LOG , - .u.log = { - .refname = (char *)name, - .update_index = update_index, - }, - }; -return it->ops->seek (it->iter_arg, &want); -} - -int reftable_iterator_seek_log (struct reftable_iterator *it, -const char *name) -{ -return reftable_iterator_seek_log_at (it, name, ~((uint64_t )0 )); -} -int reftable_table_read_ref (struct reftable_table *tab,const char *name,struct reftable_ref_record *ref) {
@@ -152,78 +122,3 @@ uint32_t reftable_table_hash_id(struct reftable_table *tab) {return tab->ops->hash_id (tab->table_arg); } - -void reftable_iterator_destroy (struct reftable_iterator *it) -{ -if (!it->ops) { -return ; - } - it->ops->close (it->iter_arg); - it->ops =NULL ; -FREE_AND_NULL (it->iter_arg); -} - -int reftable_iterator_next_ref (struct reftable_iterator *it, -struct reftable_ref_record *ref) -{ -struct reftable_record rec = { - .type =BLOCK_TYPE_REF , - .u = { - .ref = *ref - }, - }; -int err =iterator_next (it, &rec); - *ref = rec.u.ref; -return err; -} - -int reftable_iterator_next_log (struct reftable_iterator *it, -struct reftable_log_record *log) -{ -struct reftable_record rec = { - .type =BLOCK_TYPE_LOG , - .u = { - .log = *log, - }, - }; -int err =iterator_next (it, &rec); - *log = rec.u.log; -return err; -} - -int iterator_seek (struct reftable_iterator *it,struct reftable_record *want) -{ -return it->ops->seek (it->iter_arg, want); -} - -int iterator_next (struct reftable_iterator *it,struct reftable_record *rec) -{ -return it->ops->next (it->iter_arg, rec); -} - -static int empty_iterator_seek (void *arg,struct reftable_record *want) -{ -return 0 ; -} - -static int empty_iterator_next (void *arg,struct reftable_record *rec) -{ -return 1 ; -} - -static void empty_iterator_close (void *arg) -{ -} - -static struct reftable_iterator_vtable empty_vtable = { - .seek = &empty_iterator_seek, - .next = &empty_iterator_next, - .close = &empty_iterator_close, -}; - -void iterator_set_empty (struct reftable_iterator *it) -{ -assert (!it->ops); - it->iter_arg =NULL ; - it->ops = &empty_vtable; -}
reftable/generic.h +0 -10
@@ -24,14 +24,4 @@ void table_init_iter(struct reftable_table *tab,struct reftable_iterator *it, uint8_t typ); -struct reftable_iterator_vtable { -int (*seek)(void *iter_arg,struct reftable_record *want); -int (*next)(void *iter_arg,struct reftable_record *rec); -void (*close)(void *iter_arg); -}; - -void iterator_set_empty (struct reftable_iterator *it); -int iterator_seek (struct reftable_iterator *it,struct reftable_record *want); -int iterator_next (struct reftable_iterator *it,struct reftable_record *rec); -#endif
reftable/iter.c +106 -0
115 lines not on this page · raw message
reftable/iter.h +27 -0
@@ -16,6 +16,33 @@ license that can be found in the LICENSE file or at#include "reftable-iterator.h" #include "reftable-generic.h" +/* +* The virtual function table for implementing generic reftable iterators. +*/ +struct reftable_iterator_vtable { +int (*seek)(void *iter_arg,struct reftable_record *want); +int (*next)(void *iter_arg,struct reftable_record *rec); +void (*close)(void *iter_arg); +}; + +/* +* Position the iterator at the wanted record such that a call to +* `iterator_next()` would return that record, if it exists. +*/ +int iterator_seek (struct reftable_iterator *it,struct reftable_record *want); + +/* +* Yield the next record and advance the iterator. Returns <0 on error, 0 when +* a record was yielded, and >0 when the iterator hit an error. +*/ +int iterator_next (struct reftable_iterator *it,struct reftable_record *rec); + +/* +* Set up the iterator such that it behaves the same as an iterator with no +* entries. +*/ +void iterator_set_empty (struct reftable_iterator *it); +/* iterator that produces only ref records that point to `oid` */ struct filtering_ref_iterator {struct strbuf oid;
Signature · 1 line
2.46.0.46.g406f326d27.dirty
PATCH 7/10 reftable/dump: drop unused `compact_stack()`
In reply to Patrick SteinhardtThe compact_stack() function is exposed via reftable_dump_main(), which ultimately ends up being wired into "test-tool reftable". It is never used by our tests though, and nowadays we have wired up support for stack compaction into git-pack-refs(1).
Remove the code.
--- reftable/dump.c | 24 ------------------------ 1 file changed, 24 deletions(-)
reftable/dump.c +0 -24
@@ -24,25 +24,6 @@ license that can be found in the LICENSE file or at#include <unistd.h> #include <string.h> -static int compact_stack (const char *stackdir) -{ -struct reftable_stack *stack =NULL ; -struct reftable_write_options opts = {0 }; - -int err =reftable_new_stack (&stack, stackdir, &opts); -if (err <0 ) - goto done; - - err =reftable_stack_compact_all (stack,NULL ); -if (err <0 ) - goto done; -done: -if (stack) { -reftable_stack_destroy (stack); - } -return err; -} -static void print_help (void ) { printf("usage: dump [-cst] arg\n\n"
@@ -62,7 +43,6 @@ int reftable_dump_main(int argc, char *const *argv)int opt_dump_blocks =0 ;int opt_dump_table =0 ;int opt_dump_stack =0 ; -int opt_compact =0 ;uint32_t opt_hash_id =GIT_SHA1_FORMAT_ID ;const char *arg =NULL , *argv0 = argv[0 ];
@@ -77,8 +57,6 @@ int reftable_dump_main(int argc, char *const *argv) opt_hash_id =GIT_SHA256_FORMAT_ID ; else if (!strcmp("-s", argv[1])) opt_dump_stack =1 ; - else if (!strcmp("-c", argv[1])) - opt_compact =1 ; else if (!strcmp("-?", argv[1]) || !strcmp("-h", argv[1])) {print_help ();return 2 ;
@@ -98,8 +76,6 @@ int reftable_dump_main(int argc, char *const *argv) err =reftable_reader_print_file (arg); }else if (opt_dump_stack) { err =reftable_stack_print_directory (arg, opt_hash_id); - }else if (opt_compact) { - err =compact_stack (arg); }if (err <0 ) {
Signature · 1 line
2.46.0.46.g406f326d27.dirty
PATCH 8/10 reftable/dump: drop unused printing functionality
In reply to Patrick SteinhardtWe have a bunch of infrastructure wired up that allows us to print reftable records, tables and stacks. While this functionality is wired up via various "test-tool reftable" options, it is never used. It also feels kind of dubious whether any other eventual user of the reftable library should use it as it very much feels like a debugging aid rather than something sensible. The format itself is somewhat inscrutable and the infrastructure is non-extensible.
Drop this code. The only remaining function in this context is reftable_reader_print_blocks(), which we do use in our tests.
--- reftable/dump.c | 16 +---- reftable/generic.c | 47 ------------- reftable/reader.c | 21 ------ reftable/record.c | 127 ------------------------------------ reftable/record.h | 4 -- reftable/reftable-generic.h | 3 - reftable/reftable-reader.h | 2 - reftable/reftable-record.h | 8 --- reftable/reftable-stack.h | 3 - reftable/stack.c | 20 ------ reftable/stack_test.c | 7 -- 11 files changed, 1 insertion(+), 257 deletions(-)
reftable/dump.c +1 -15
@@ -41,9 +41,6 @@ int reftable_dump_main(int argc, char *const *argv) {int err =0 ;int opt_dump_blocks =0 ; -int opt_dump_table =0 ; -int opt_dump_stack =0 ; -uint32_t opt_hash_id =GIT_SHA1_FORMAT_ID ;const char *arg =NULL , *argv0 = argv[0 ];for (; argc >1 ; argv++, argc--)
@@ -51,12 +48,6 @@ int reftable_dump_main(int argc, char *const *argv)break ; else if (!strcmp("-b", argv[1])) opt_dump_blocks =1 ; - else if (!strcmp("-t", argv[1])) - opt_dump_table =1 ; - else if (!strcmp("-6", argv[1])) - opt_hash_id =GIT_SHA256_FORMAT_ID ; - else if (!strcmp("-s", argv[1])) - opt_dump_stack =1 ; else if (!strcmp("-?", argv[1]) || !strcmp("-h", argv[1])) {print_help ();return 2 ;
@@ -70,13 +61,8 @@ int reftable_dump_main(int argc, char *const *argv) arg = argv[1 ]; -if (opt_dump_blocks) { +if (opt_dump_blocks) err =reftable_reader_print_blocks (arg); - }else if (opt_dump_table) { - err =reftable_reader_print_file (arg); - }else if (opt_dump_stack) { - err =reftable_stack_print_directory (arg, opt_hash_id); - }if (err <0 ) { fprintf(stderr, "%s: %s: %s\n", argv0, arg,
reftable/generic.c +0 -47
53 lines not on this page · raw message
reftable/reader.c +0 -21
@@ -798,27 +798,6 @@ void reftable_table_from_reader(struct reftable_table *tab, tab->table_arg = reader; } - -int reftable_reader_print_file (const char *tablename) -{ -struct reftable_block_source src = {NULL }; -int err =reftable_block_source_from_file (&src, tablename); -struct reftable_reader *r =NULL ; -struct reftable_table tab = {NULL }; -if (err <0 ) - goto done; - - err =reftable_new_reader (&r, &src, tablename); -if (err <0 ) - goto done; - -reftable_table_from_reader (&tab, r); - err =reftable_table_print (&tab); -done: -reftable_reader_free (r); -return err; -} -int reftable_reader_print_blocks (const char *tablename) {struct {
reftable/record.c +0 -127
186 lines not on this page · raw message
reftable/record.h +0 -4
@@ -72,9 +72,6 @@ struct reftable_record_vtable {* the same type. */ int (*cmp)(const void *a,const void *b); - -/* Print on stdout, for debugging. */ -void (*print)(const void *rec,int hash_size); };/* returns true for recognized block types. Block start with the block type. */
@@ -136,7 +133,6 @@ void reftable_record_init(struct reftable_record *rec, uint8_t typ);/* see struct record_vtable */ int reftable_record_cmp (struct reftable_record *a,struct reftable_record *b);int reftable_record_equal (struct reftable_record *a,struct reftable_record *b,int hash_size); -void reftable_record_print (struct reftable_record *rec,int hash_size);void reftable_record_key (struct reftable_record *rec,struct strbuf *dest);void reftable_record_copy_from (struct reftable_record *rec,struct reftable_record *src,int hash_size);
reftable/reftable-generic.h +0 -3
@@ -41,7 +41,4 @@ uint64_t reftable_table_min_update_index(struct reftable_table *tab);int reftable_table_read_ref (struct reftable_table *tab,const char *name,struct reftable_ref_record *ref); -/* dump table contents onto stdout for debugging */ -int reftable_table_print (struct reftable_table *tab); -#endif
reftable/reftable-reader.h +0 -2
@@ -64,8 +64,6 @@ uint64_t reftable_reader_min_update_index(struct reftable_reader *r);void reftable_table_from_reader (struct reftable_table *tab,struct reftable_reader *reader); -/* print table onto stdout for debugging. */ -int reftable_reader_print_file (const char *tablename);/* print blocks onto stdout for debugging. */ int reftable_reader_print_blocks (const char *tablename);
reftable/reftable-record.h +0 -8
@@ -60,10 +60,6 @@ const unsigned char *reftable_ref_record_val2(const struct reftable_ref_record */* returns whether 'ref' represents a deletion */ int reftable_ref_record_is_deletion (const struct reftable_ref_record *ref); -/* prints a reftable_ref_record onto stdout. Useful for debugging. */ -void reftable_ref_record_print (const struct reftable_ref_record *ref, -uint32_t hash_id); -/* frees and nulls all pointer values inside `ref`. */ void reftable_ref_record_release (struct reftable_ref_record *ref);
@@ -111,8 +107,4 @@ void reftable_log_record_release(struct reftable_log_record *log);int reftable_log_record_equal (const struct reftable_log_record *a,const struct reftable_log_record *b,int hash_size); -/* dumps a reftable_log_record on stdout, for debugging/testing. */ -void reftable_log_record_print (struct reftable_log_record *log, -uint32_t hash_id); -#endif
reftable/reftable-stack.h +0 -3
@@ -140,7 +140,4 @@ struct reftable_compaction_stats {struct reftable_compaction_stats *reftable_stack_compaction_stats (struct reftable_stack *st); -/* print the entire stack represented by the directory */ -int reftable_stack_print_directory (const char *stackdir,uint32_t hash_id); -#endif
reftable/stack.c +0 -20
@@ -1603,23 +1603,3 @@ int reftable_stack_clean(struct reftable_stack *st)reftable_addition_destroy (add);return err; } - -int reftable_stack_print_directory (const char *stackdir,uint32_t hash_id) -{ -struct reftable_stack *stack =NULL ; -struct reftable_write_options opts = { .hash_id = hash_id }; -struct reftable_merged_table *merged =NULL ; -struct reftable_table table = {NULL }; - -int err =reftable_new_stack (&stack, stackdir, &opts); -if (err <0 ) - goto done; - - merged =reftable_stack_merged_table (stack); -reftable_table_from_merged_table (&table, merged); - err =reftable_table_print (&table); -done: -if (stack) -reftable_stack_destroy (stack); -return err; -}
reftable/stack_test.c +0 -7
@@ -179,13 +179,6 @@ static void test_reftable_stack_add_one(void)EXPECT (0 ==strcmp ("master" , dest.value.symref));EXPECT (st->readers_len >0 ); -printf ("testing print functionality:\n" ); - err =reftable_stack_print_directory (dir,GIT_SHA1_FORMAT_ID ); -EXPECT_ERR (err); - - err =reftable_stack_print_directory (dir,GIT_SHA256_FORMAT_ID ); -EXPECT (err ==REFTABLE_FORMAT_ERROR ); -#ifndef GIT_WINDOWS_NATIVE strbuf_addstr (&scratch, dir);strbuf_addstr (&scratch,"/tables.list" );
Signature · 1 line
2.46.0.46.g406f326d27.dirty
PATCH 9/10 reftable/dump: move code into "t/helper/test-reftable.c"
In reply to Patrick SteinhardtThe code in "reftable/dump.c" is only ever used by our test-tool to implement the "reftable" subcommand. It also feels very unlikely that it will ever be useful to any other potential user of the reftable library, which makes it a weird candidate to have in the library interface.
Inline the code into "t/helper/test-reftable.c".
--- Makefile | 1 - reftable/dump.c | 73 ---------------------------------------- t/helper/test-reftable.c | 47 +++++++++++++++++++++++++- 3 files changed, 46 insertions(+), 75 deletions(-) delete mode 100644 reftable/dump.c
Makefile +0 -1
@@ -2680,7 +2680,6 @@ REFTABLE_OBJS += reftable/tree.o REFTABLE_OBJS += reftable/writer.o REFTABLE_TEST_OBJS += reftable/block_test.o -REFTABLE_TEST_OBJS += reftable/dump.o REFTABLE_TEST_OBJS += reftable/pq_test.o REFTABLE_TEST_OBJS += reftable/readwrite_test.o REFTABLE_TEST_OBJS += reftable/stack_test.o
reftable/dump.c deleted +0 -73
73 lines not on this page · raw message
t/helper/test-reftable.c +46 -1
57 lines not on this page · raw message
Signature · 1 line
2.46.0.46.g406f326d27.dirty
PATCH 10/10 reftable/generic: drop interface
In reply to Patrick SteinhardtThe reftable_table interface provides a generic infrastructure that can abstract away whether the underlying table is a single table, or a merged table. This abstraction can make it rather hard to reason about the code. We didn't ever use it to implement the reftable backend, and with the preceding patches in this patch series we in fact don't use it at all anymore. Furthermore, it became somewhat useless with the recent refactorings that made it possible to seek reftable iterators multiple times, as these now provide generic access to tables for us. The interface is thus redundant and only brings unnecessary complexity with it.
Remove the struct reftable_table interface and its associated functions.
--- Makefile | 1 - reftable/generic.c | 77 -------------------------------- reftable/generic.h | 27 ----------- reftable/iter.c | 1 - reftable/iter.h | 1 - reftable/merged.c | 38 ---------------- reftable/reader.c | 41 ----------------- reftable/reftable-generic.h | 44 ------------------ reftable/reftable-merged.h | 6 --- reftable/reftable-reader.h | 7 --- reftable/stack.c | 1 - t/unit-tests/t-reftable-merged.c | 1 - 12 files changed, 245 deletions(-) delete mode 100644 reftable/generic.c delete mode 100644 reftable/generic.h delete mode 100644 reftable/reftable-generic.h
Makefile +0 -1
@@ -2674,7 +2674,6 @@ REFTABLE_OBJS += reftable/merged.o REFTABLE_OBJS += reftable/pq.o REFTABLE_OBJS += reftable/reader.o REFTABLE_OBJS += reftable/record.o -REFTABLE_OBJS += reftable/generic.o REFTABLE_OBJS += reftable/stack.o REFTABLE_OBJS += reftable/tree.o REFTABLE_OBJS += reftable/writer.o
reftable/generic.c deleted +0 -77
77 lines not on this page · raw message
reftable/generic.h deleted +0 -27
@@ -1,27 +0,0 @@ -/* -Copyright 2020 Google LLC - -Use of this source code is governed by a BSD-style -license that can be found in the LICENSE file or at -https://developers.google.com/open-source/licenses/bsd -*/ - -#ifndef GENERIC_H -#define GENERIC_H - -#include "record.h" -#include "reftable-generic.h" - -/* generic interface to reftables */ -struct reftable_table_vtable { -void (*init_iter)(void *tab,struct reftable_iterator *it,uint8_t typ); -uint32_t (*hash_id)(void *tab); -uint64_t (*min_update_index)(void *tab); -uint64_t (*max_update_index)(void *tab); -}; - -void table_init_iter (struct reftable_table *tab, -struct reftable_iterator *it, -uint8_t typ); - -#endif
reftable/iter.c +0 -1
@@ -11,7 +11,6 @@ license that can be found in the LICENSE file or at#include "system.h" #include "block.h" -#include "generic.h" #include "constants.h" #include "reader.h" #include "reftable-error.h"
reftable/iter.h +0 -1
@@ -14,7 +14,6 @@ license that can be found in the LICENSE file or at#include "record.h" #include "reftable-iterator.h" -#include "reftable-generic.h" /* * The virtual function table for implementing generic reftable iterators.
reftable/merged.c +0 -38
@@ -13,7 +13,6 @@ license that can be found in the LICENSE file or at#include "pq.h" #include "reader.h" #include "record.h" -#include "generic.h" #include "reftable-merged.h" #include "reftable-error.h" #include "system.h"
@@ -270,40 +269,3 @@ uint32_t reftable_merged_table_hash_id(struct reftable_merged_table *mt) {return mt->hash_id; } - -static void reftable_merged_table_init_iter_void (void *tab, -struct reftable_iterator *it, -uint8_t typ) -{ -merged_table_init_iter (tab, it, typ); -} - -static uint32_t reftable_merged_table_hash_id_void (void *tab) -{ -return reftable_merged_table_hash_id (tab); -} - -static uint64_t reftable_merged_table_min_update_index_void (void *tab) -{ -return reftable_merged_table_min_update_index (tab); -} - -static uint64_t reftable_merged_table_max_update_index_void (void *tab) -{ -return reftable_merged_table_max_update_index (tab); -} - -static struct reftable_table_vtable merged_table_vtable = { - .init_iter = reftable_merged_table_init_iter_void, - .hash_id = reftable_merged_table_hash_id_void, - .min_update_index = reftable_merged_table_min_update_index_void, - .max_update_index = reftable_merged_table_max_update_index_void, -}; - -void reftable_table_from_merged_table (struct reftable_table *tab, -struct reftable_merged_table *merged) -{ -assert (!tab->ops); - tab->ops = &merged_table_vtable; - tab->table_arg = merged; -}
reftable/reader.c +0 -41
56 lines not on this page · raw message
reftable/reftable-generic.h deleted +0 -44
44 lines not on this page · raw message
reftable/reftable-merged.h +0 -6
@@ -26,8 +26,6 @@ license that can be found in the LICENSE file or at/* A merged table is implements seeking/iterating over a stack of tables. */ struct reftable_merged_table ; -/* A generic reftable; see below. */ -struct reftable_table ;struct reftable_reader ;/*
@@ -60,8 +58,4 @@ void reftable_merged_table_free(struct reftable_merged_table *m);/* return the hash ID of the merged table. */ uint32_t reftable_merged_table_hash_id (struct reftable_merged_table *m); -/* create a generic table from reftable_merged_table */ -void reftable_table_from_merged_table (struct reftable_table *tab, -struct reftable_merged_table *table); -#endif
reftable/reftable-reader.h +0 -7
@@ -23,9 +23,6 @@/* The reader struct is a handle to an open reftable file. */ struct reftable_reader ; -/* Generic table. */ -struct reftable_table ; -/* reftable_new_reader opens a reftable for reading. If successful, * returns 0 code and sets pp. The name is used for creating a * stack. Typically, it is the basename of the file. The block source
@@ -60,10 +57,6 @@ uint64_t reftable_reader_max_update_index(struct reftable_reader *r);/* return the min_update_index for a table */ uint64_t reftable_reader_min_update_index (struct reftable_reader *r); -/* creates a generic table from a file reader. */ -void reftable_table_from_reader (struct reftable_table *tab, -struct reftable_reader *reader); -/* print blocks onto stdout for debugging. */ int reftable_reader_print_blocks (const char *tablename);
reftable/stack.c +0 -1
@@ -14,7 +14,6 @@ license that can be found in the LICENSE file or at#include "merged.h" #include "reader.h" #include "reftable-error.h" -#include "reftable-generic.h" #include "reftable-record.h" #include "reftable-merged.h" #include "writer.h"
t/unit-tests/t-reftable-merged.c +0 -1
@@ -12,7 +12,6 @@ license that can be found in the LICENSE file or at#include "reftable/merged.h" #include "reftable/reader.h" #include "reftable/reftable-error.h" -#include "reftable/reftable-generic.h" #include "reftable/reftable-merged.h" #include "reftable/reftable-writer.h"
Signature · 1 line
2.46.0.46.g406f326d27.dirty
Re: [PATCH 05/10] reftable/iter: drop double-checking logic
In reply to Patrick SteinhardtQuoted 3 lines · Patrick Steinhardt
The filtering ref iterator can be used to only yield refs which are not in a specific skip list. This iterator has an option to double-check the results it returns, which causes us to seek tho reference we are about
s/tho/the/
Quoted 13 lines
to yield via a separate table such that we detect whether the reference that the first iterator has yielded actually exists.
The value of this is somewhat dubious, and I cannot think of any usecase where this functionality should be required. Furthermore, this option is never set in our codebase, which means that it is essentially untested. And last but not least, the
struct reftable_tablethat is used to implement it is about to go away.So while we could refactor the code to not use a
reftable_table, it very much feels like a wasted effort. Let's just drop this code.Signed-off-by Patrick Steinhardt <[email protected]>
Re: [PATCH 01/10] reftable/merged: expose functions to initialize iterators
In reply to Patrick SteinhardtQuoted 24 lines · Patrick Steinhardt
We do not expose any functions via our public headers that would allow a caller to initialize a reftable iterator from a merged table. Instead, they are expected to go via the generic
reftable_tableinterface, which is somewhat roundabout.Implement two new functions to initialize iterators for ref and log records to plug this gap.
Signed-off-by Patrick Steinhardt <[email protected]>--- reftable/merged.c | 12 ++++++++++++ reftable/reftable-merged.h | 8 ++++++++ 2 files changed, 20 insertions(+)reftable/merged.c +2
-0@@ -254,6 +254,18 @@ void merged_table_init_iter(struct reftable_merged_table *mt,iterator_from_merged_iter (it, mi); } +void reftable_merged_table_init_ref_iterator (struct reftable_merged_table *mt, +struct reftable_iterator *it)
reftable/merged.c void merged_table_init_iter(struct reftable_merged_table *mt, · line 268
Quoted 10 lines
reftable/merged.c +10
-0+{ +merged_table_init_iter (mt, it,BLOCK_TYPE_REF ); +} + +void reftable_merged_table_init_log_iterator (struct reftable_merged_table *mt, +struct reftable_iterator *it) +{ +merged_table_init_iter (mt, it,BLOCK_TYPE_LOG ); +} +
These too look similar to static void reftable_merged_table_init_iter_void defined a little below, I wonder if we could have simply exposed that? Perhaps we remove it in the upcoming patches.
[snip]
Re: [PATCH 03/10] reftable/merged: stop using generic tables in the merged table
In reply to Patrick SteinhardtPatrick Steinhardt <[email protected]> writes:
[snip]
Quoted 7 lines
reftable/stack.c +0
-0@@ -225,13 +225,11 @@ static int reftable_stack_reload_once(struct reftable_stack *st,const char **names, int reuse_open)
reftable/stack.c static int reftable_stack_reload_once(struct reftable_stack *st, · line 233
Quoted 10 lines
reftable/stack.c +1
-3{ -size_t cur_len = !st->merged ?0 : st->merged->stack_len; +size_t cur_len = !st->merged ?0 : st->merged->readers_len;struct reftable_reader **cur =stack_copy_readers (st, cur_len);size_t names_len =names_length (names);struct reftable_reader **new_readers =reftable_calloc (names_len,sizeof (*new_readers)); -struct reftable_table *new_tables = -reftable_calloc (names_len,sizeof (*new_tables));
Before we had both reftable_reader and reftable_table here and we used to set reftable_table from the reader and use it to create a merged table. Now we will simply use the readers directly.
reftable/stack.c static int reftable_stack_reload_once(struct reftable_stack *st, · line 270
Quoted 8 lines
reftable/stack.c +0
-0size_t new_readers_len =0 ;struct reftable_merged_table *new_merged =NULL ;reftable/stack.c +0
-1struct strbuf table_path =STRBUF_INIT ;@@ -267,17 +265,15 @@ static int reftable_stack_reload_once(struct reftable_stack *st, } new_readers[new_readers_len] = rd; -reftable_table_from_reader (&new_tables[new_readers_len], rd);
This being the part where we convert reftable_reader array to reftable_table array.
reftable/stack.c static int reftable_stack_reload_once(struct reftable_stack *st, · line 276
Quoted 8 lines
reftable/stack.c +1
-1new_readers_len++; }/* success! */ - err =reftable_merged_table_new (&new_merged, new_tables, + err =reftable_merged_table_new (&new_merged, new_readers, new_readers_len, st->opts.hash_id);
And this being the part where we create the merged table.
reftable/stack.c static int reftable_stack_reload_once(struct reftable_stack *st,
Quoted 8 lines
reftable/stack.c +0
-1if (err <0 ) goto done; - new_tables =NULL ; st->readers_len = new_readers_len;if (st->merged)reftable/stack.c +0
-0reftable_merged_table_free (st->merged);@@ -309,7 +305,6 @@ static int reftable_stack_reload_once(struct reftable_stack *st,
[snip]
The entire patch is along the lines of what I explained above, so looks good!
Re: [PATCH 06/10] reftable/generic: move generic iterator code into iterator interface
In reply to Patrick SteinhardtPatrick Steinhardt <[email protected]> writes:
[snip]
reftable/iter.c void iterator_from_indexed_table_ref_iter(struct reftable_iterator *it, · line 226
Quoted 7 lines
reftable/iter.c +6
-0+ +void reftable_iterator_destroy (struct reftable_iterator *it) +{ +if (!it->ops) { +return ; + }
I know this commit is to move, but I couldn't help noticing that we should remove the curly braces here.
Seems like the CI caught it too [1].
reftable/iter.c void iterator_from_indexed_table_ref_iter(struct reftable_iterator *it, · line 231
Quoted 5 lines
reftable/iter.c +5
-0+ it->ops->close (it->iter_arg); + it->ops =NULL ; +FREE_AND_NULL (it->iter_arg); +} +
[snip]
Re: [PATCH 08/10] reftable/dump: drop unused printing functionality
In reply to Patrick Steinhardtreftable/dump.c int reftable_dump_main(int argc, char *const *argv)
Quoted 54 lines · Patrick Steinhardt
We have a bunch of infrastructure wired up that allows us to print reftable records, tables and stacks. While this functionality is wired up via various "test-tool reftable" options, it is never used. It also feels kind of dubious whether any other eventual user of the reftable library should use it as it very much feels like a debugging aid rather than something sensible. The format itself is somewhat inscrutable and the infrastructure is non-extensible.
Drop this code. The only remaining function in this context is
reftable_reader_print_blocks(), which we do use in our tests.Signed-off-by Patrick Steinhardt <[email protected]>--- reftable/dump.c | 16 +---- reftable/generic.c | 47 ------------- reftable/reader.c | 21 ------ reftable/record.c | 127 ------------------------------------ reftable/record.h | 4 -- reftable/reftable-generic.h | 3 - reftable/reftable-reader.h | 2 - reftable/reftable-record.h | 8 --- reftable/reftable-stack.h | 3 - reftable/stack.c | 20 ------ reftable/stack_test.c | 7 -- 11 files changed, 1 insertion(+), 257 deletions(-)reftable/dump.c +0
-9@@ -41,9 +41,6 @@ int reftable_dump_main(int argc, char *const *argv) {int err =0 ;int opt_dump_blocks =0 ; -int opt_dump_table =0 ; -int opt_dump_stack =0 ; -uint32_t opt_hash_id =GIT_SHA1_FORMAT_ID ;const char *arg =NULL , *argv0 = argv[0 ];for (; argc >1 ; argv++, argc--)@@ -51,12 +48,6 @@ int reftable_dump_main(int argc, char *const *argv)break ; else if (!strcmp("-b", argv[1])) opt_dump_blocks =1 ; - else if (!strcmp("-t", argv[1])) - opt_dump_table =1 ; - else if (!strcmp("-6", argv[1])) - opt_hash_id =GIT_SHA256_FORMAT_ID ; - else if (!strcmp("-s", argv[1])) - opt_dump_stack =1 ; else if (!strcmp("-?", argv[1]) || !strcmp("-h", argv[1])) {print_help ();return 2 ;@@ -70,13 +61,8 @@ int reftable_dump_main(int argc, char *const *argv)
I'm a bit skeptical about this change because I definitely have used the -t and -s options a bunch of times to understand what a table holds. Since the reftable format is binary, this is the only tooling we have which allows us to read this format from a plumbing point of view. I'd keep them. I guess the stack printing just iterates over the tables and prints them and could be removed, but I'd keep the option to dump a table.
Also this patch misses cleaning up print_help if we go down this route.
[snip]
Quoted 10 lines · Patrick Steinhardt
Hi,
this patch series performs some simplification of the reftable library by dropping the generic
reftable_tableinterface. The intent of this interface is to abstract away whether the underlying table is either a single reftable, or a merged reftable. Arguably though, it is not needed anymore starting with my recent refactorings [1] that made the reftable iterator itself seekable, as that now provides such a generic interface already.
Quoted 9 lines
So this patch series rips out the
reftable_tableinterface, both to remove unused clutter, but more importantly to make the reftable library easier to understand overall. There is also a tiny performance gain of 1-3% by requiring one less vtable function call. But while that speedup is consistent, I didn't include any benchmarks as it is rather close to noise and not the primary motivation of this patch series.Patrick
This is a very welcome change, the indirection was a cause for confusion before and this clears it up.
My only concern is the loss of dumping reftables for debug purposes in patch 8, I suggest we keep them until we have alternatives. Apart from that everything looks great.
Thanks!
Re: [PATCH 01/10] reftable/merged: expose functions to initialize iterators
In reply to karthik nayakQuoted 33 lines · karthik nayak
Quoted 31 lines · Patrick Steinhardt
We do not expose any functions via our public headers that would allow a caller to initialize a reftable iterator from a merged table. Instead, they are expected to go via the generic
reftable_tableinterface, which is somewhat roundabout.Implement two new functions to initialize iterators for ref and log records to plug this gap.
Signed-off-by Patrick Steinhardt <[email protected]>--- reftable/merged.c | 12 ++++++++++++ reftable/reftable-merged.h | 8 ++++++++ 2 files changed, 20 insertions(+)reftable/merged.c +9
-0@@ -254,6 +254,18 @@ void merged_table_init_iter(struct reftable_merged_table *mt,iterator_from_merged_iter (it, mi); } +void reftable_merged_table_init_ref_iterator (struct reftable_merged_table *mt, +struct reftable_iterator *it) +{ +merged_table_init_iter (mt, it,BLOCK_TYPE_REF ); +} + +void reftable_merged_table_init_log_iterator (struct reftable_merged_table *mt, +struct reftable_iterator *it) +{
Quoted 8 lines
reftable/merged.c void merged_table_init_iter(struct reftable_merged_table *mt, · line 268
Quoted 3 lines
reftable/merged.c +3
-0+merged_table_init_iter (mt, it,BLOCK_TYPE_LOG ); +} +These too look similar to
static void reftable_merged_table_init_iter_voiddefined a little below, I wonder if we could have simply exposed that? Perhaps we remove it in the upcoming patches.
Yup, reftable_merged_table_init_iter_void() is about to go away. But regardless of that, even if it didn't it would make sense to have both functions. The iter_void() one takes an arbitrary block type as input, which we never expose via our public interfaces. So it has to stay an implementation detail. The new ones added here do not expose the block type.
Patrick
Re: [PATCH 06/10] reftable/generic: move generic iterator code into iterator interface
In reply to karthik nayakQuoted 15 lines · karthik nayak
Patrick Steinhardt <[email protected]> writes:
[snip]
reftable/iter.c void iterator_from_indexed_table_ref_iter(struct reftable_iterator *it, · line 226
Quoted 7 lines
reftable/iter.c +6
-0+ +void reftable_iterator_destroy (struct reftable_iterator *it) +{ +if (!it->ops) { +return ; + }I know this commit is to move, but I couldn't help noticing that we should remove the curly braces here.
Quoted 1 line
Seems like the CI caught it too [1].
Fair enough, let's remove them.
Patrick
Re: [PATCH 08/10] reftable/dump: drop unused printing functionality
In reply to karthik nayakQuoted 57 lines · karthik nayak
reftable/dump.c int reftable_dump_main(int argc, char *const *argv)
Quoted 54 lines · Patrick Steinhardt
We have a bunch of infrastructure wired up that allows us to print reftable records, tables and stacks. While this functionality is wired up via various "test-tool reftable" options, it is never used. It also feels kind of dubious whether any other eventual user of the reftable library should use it as it very much feels like a debugging aid rather than something sensible. The format itself is somewhat inscrutable and the infrastructure is non-extensible.
Drop this code. The only remaining function in this context is
reftable_reader_print_blocks(), which we do use in our tests.Signed-off-by Patrick Steinhardt <[email protected]>--- reftable/dump.c | 16 +---- reftable/generic.c | 47 ------------- reftable/reader.c | 21 ------ reftable/record.c | 127 ------------------------------------ reftable/record.h | 4 -- reftable/reftable-generic.h | 3 - reftable/reftable-reader.h | 2 - reftable/reftable-record.h | 8 --- reftable/reftable-stack.h | 3 - reftable/stack.c | 20 ------ reftable/stack_test.c | 7 -- 11 files changed, 1 insertion(+), 257 deletions(-)reftable/dump.c +0
-9@@ -41,9 +41,6 @@ int reftable_dump_main(int argc, char *const *argv) {int err =0 ;int opt_dump_blocks =0 ; -int opt_dump_table =0 ; -int opt_dump_stack =0 ; -uint32_t opt_hash_id =GIT_SHA1_FORMAT_ID ;const char *arg =NULL , *argv0 = argv[0 ];for (; argc >1 ; argv++, argc--)@@ -51,12 +48,6 @@ int reftable_dump_main(int argc, char *const *argv)break ; else if (!strcmp("-b", argv[1])) opt_dump_blocks =1 ; - else if (!strcmp("-t", argv[1])) - opt_dump_table =1 ; - else if (!strcmp("-6", argv[1])) - opt_hash_id =GIT_SHA256_FORMAT_ID ; - else if (!strcmp("-s", argv[1])) - opt_dump_stack =1 ; else if (!strcmp("-?", argv[1]) || !strcmp("-h", argv[1])) {print_help ();return 2 ;@@ -70,13 +61,8 @@ int reftable_dump_main(int argc, char *const *argv)
Quoted 7 lines
I'm a bit skeptical about this change because I definitely have used the
-tand-soptions a bunch of times to understand what a table holds. Since the reftable format is binary, this is the only tooling we have which allows us to read this format from a plumbing point of view. I'd keep them. I guess the stack printing just iterates over the tables and prints them and could be removed, but I'd keep the option to dump a table.
Well, I have to say that I find the output to be rather useless. But you're making a valid point: we don't have anything else to peek at the table's contents.
I'll keep this in v2, but make it an internal implementation detail of the test-tool. It certainly has no place in the reftable library in my opinion.
Quoted 2 lines
Also this patch misses cleaning up
print_helpif we go down this route.
Indeed, will fix.
Patrick
Re: [PATCH 06/10] reftable/generic: move generic iterator code into iterator interface
In reply to karthik nayakQuoted 11 lines · karthik nayak
Seems like the CI caught it too [1].
reftable/iter.c void iterator_from_indexed_table_ref_iter(struct reftable_iterator *it, · line 231
Quoted 5 lines
reftable/iter.c +5
-0+ it->ops->close (it->iter_arg); + it->ops =NULL ; +FREE_AND_NULL (it->iter_arg); +} +[snip]
Looking at it, the suggestions the CI job makes look hit-and-miss.
For examle, this one
-static int stack_compact_range_stats(struct reftable_stack *st,
- size_t first, size_t last,
+static int stack_compact_range_stats(struct reftable_stack *st, size_t first,
+ size_t last,
struct reftable_log_expiry_config *config, unsigned int flags)
is a degradation of readability. "first" and "last" pretty much act as a pair and they read better sitting together next to each other. The rewrite is reducing neither the total number of lines or the maximum column width.
But this one
-static void write_n_ref_tables(struct reftable_stack *st,
- size_t n)
+static void write_n_ref_tables(struct reftable_stack *st, size_t n)
is certainly an improvement.
This one
struct reftable_record rec = {
.type = BLOCK_TYPE_REF,
- .u = {
- .ref = *ref
- },
+ .u = { .ref = *ref },
};
is hard to generalize but in this case it made a good suggestion.
If we _expect_ that we will enumerate more members of .u, then the way originally written (plus trailing comma after the ".ref = *ref") would be easier to maintain. But since .u is a union, we won't be choosing more than one member to initialize by definition, so what was suggested by the check-style linter is certainly much better.
Thanks.
Re: [PATCH 06/10] reftable/generic: move generic iterator code into iterator interface
Quoted 12 lines · Junio C Hamano
Quoted 10 lines · karthik nayak
Seems like the CI caught it too [1].
Quoted 5 lines
reftable/iter.c +5
-0+ it->ops->close (it->iter_arg); + it->ops =NULL ; +FREE_AND_NULL (it->iter_arg); +} +[snip]
Quoted 4 lines
Quoted 1 line
Looking at it, the suggestions the CI job makes look hit-and-miss.
Yup, this is kinda what I was expecting and why I also wanted it to be allowed to fail. So we have data points on what we should change to make it better.
Quoted 9 lines
For examle, this one
(quoted diff) +2
-2-static int stack_compact_range_stats(struct reftable_stack *st, - size_t first, size_t last, +static int stack_compact_range_stats(struct reftable_stack *st, size_t first, + size_t last, struct reftable_log_expiry_config *config, unsigned int flags)
Quoted 5 lines
is a degradation of readability. "first" and "last" pretty much act as a pair and they read better sitting together next to each other. The rewrite is reducing neither the total number of lines or the maximum column width.
Totally agreed, This is set by ColumnLimit: 80 and I think most of the misses I see are because of this. This works with the Penalties we set at the bottom. We need to see what combination works best for us since our focus would be more on the readability front.
Quoted 20 lines
But this one
(quoted diff) +1
-2-static void write_n_ref_tables(struct reftable_stack *st, - size_t n) +static void write_n_ref_tables(struct reftable_stack *st, size_t n)is certainly an improvement.
This one
(quoted diff) +1
-3struct reftable_record rec = { .type = BLOCK_TYPE_REF, - .u = { - .ref = *ref - }, + .u = { .ref = *ref }, };is hard to generalize but in this case it made a good suggestion.
Quoted 7 lines
If we _expect_ that we will enumerate more members of .u, then the way originally written (plus trailing comma after the ".ref = *ref") would be easier to maintain. But since .u is a union, we won't be choosing more than one member to initialize by definition, so what was suggested by the check-style linter is certainly much better.
Thanks.
Yup these two do look much nicer indeed. There are weird bugs in clang-format however and I was able to fool it to accept
struct reftable_record rec = {
.type = BLOCK_TYPE_REF,
.u = {
.ref = *ref },
};But this could also be an issue with our config too.
I'll try playing around for an hour today to see if I can find some improvements we can make to the formatter config.
Thanks
Re: [PATCH 03/10] reftable/merged: stop using generic tables in the merged table
In reply to Patrick SteinhardtQuoted 7 lines · Patrick Steinhardt
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.
Quoted 6 lines
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.
Agreed! When I was reading through the reftable code for the first time I remember being rather confused by this interface. It left me wondering if merge tables could also be composed of other merge tables, but I couldn't think of a valid reason to ever do so.
Quoted 3 lines
Refactor the merged tables to use a
struct reftable_readerfor each of the subtables instead, which gives us direct access to the underlying tables. Adjust names accordingly.
Using struct reftable_reader directly instead of the generic struct reftable_table sounds like the right decision and the flexibility it provids is unneeded and only adds complexity.
Quoted 2 lines
Signed-off-by Patrick Steinhardt <[email protected]>---
[snip]
reftable/merged.h license that can be found in the LICENSE file or at · line 16
Quoted 12 lines
reftable/merged.h +2
-2@@ -12,8 +12,8 @@ license that can be found in the LICENSE file or at#include "system.h" struct reftable_merged_table { -struct reftable_table *stack; -size_t stack_len; +struct reftable_reader **readers; +size_t readers_len;
The merged table is being made to directly reference the table readers instead of going through the generic struct reftable_table. Makes sense.
Quoted 25 lines
reftable/merged.h +0
-0uint32_t hash_id;/* 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, it->ops = &table_iter_vtable; } -static void reader_init_iter (struct reftable_reader *r, -struct reftable_iterator *it, -uint8_t typ) +void reader_init_iter (struct reftable_reader *r, +struct reftable_iterator *it, +uint8_t typ) {struct reftable_reader_offsets *offs =reader_offsets_for (r, typ);reftable/reader.h +0
-0
reftable/reader.h int init_reader(struct reftable_reader *r, struct reftable_block_source *source, · line 62
Quoted 6 lines
reftable/reader.h +3
-0@@ -57,6 +57,10 @@ int init_reader(struct reftable_reader *r, struct reftable_block_source *source,void reader_close (struct reftable_reader *r);const char *reader_name (struct reftable_reader *r); +void reader_init_iter (struct reftable_reader *r, +struct reftable_iterator *it, +uint8_t typ);
At first I was wondering if we should prefix the function name with reftable_, but this is only exposed as part of the internal reftable interface and matches the format of other "reftable/reader.h" functions.
Re: [PATCH 01/10] reftable/merged: expose functions to initialize iterators
In reply to Patrick SteinhardtQuoted 10 lines · Patrick Steinhardt
We do not expose any functions via our public headers that would allow a caller to initialize a reftable iterator from a merged table. Instead, they are expected to go via the generic
reftable_tableinterface, which is somewhat roundabout.Implement two new functions to initialize iterators for ref and log records to plug this gap.
Signed-off-by Patrick Steinhardt <[email protected]>---
[snip]
Quoted 5 lines
reftable/reftable-merged.h +0
-0
reftable/reftable-merged.h int reftable_new_merged_table(struct reftable_merged_table **dest, · line 45
Quoted 10 lines
reftable/reftable-merged.h +7
-0@@ -36,6 +36,14 @@ int reftable_new_merged_table(struct reftable_merged_table **dest, struct reftable_table *stack, size_t n, uint32_t hash_id); +/* Initialize a merged table iterator for reading refs. */ +void reftable_merged_table_init_ref_iterator (struct reftable_merged_table *mt, +struct reftable_iterator *it); + +/* Initialize a merged table iterator for reading logs. */ +void reftable_merged_table_init_log_iterator (struct reftable_merged_table *mt, +struct reftable_iterator *it);
I might have missed it, but I do not see reftable_merged_table_init_log_iterator() used anywhere in the later patches. Does this need to be added? Or are we just adding it because we want a companion function to the ref iterator to be more consistent?
-Justin
Re: [PATCH 01/10] reftable/merged: expose functions to initialize iterators
Quoted 20 lines · Justin Tobler
Quoted 10 lines · Patrick Steinhardt
We do not expose any functions via our public headers that would allow a caller to initialize a reftable iterator from a merged table. Instead, they are expected to go via the generic
reftable_tableinterface, which is somewhat roundabout.Implement two new functions to initialize iterators for ref and log records to plug this gap.
Signed-off-by Patrick Steinhardt <[email protected]>---
[snip]
Quoted 8 lines
reftable/reftable-merged.h +0
-0@@ -36,6 +36,14 @@ int reftable_new_merged_table(struct reftable_merged_table **dest, struct reftable_table *stack, size_t n, uint32_t hash_id);
Quoted 12 lines
reftable/reftable-merged.h int reftable_new_merged_table(struct reftable_merged_table **dest, · line 45
Quoted 7 lines
reftable/reftable-merged.h +7
-0+/* Initialize a merged table iterator for reading refs. */ +void reftable_merged_table_init_ref_iterator (struct reftable_merged_table *mt, +struct reftable_iterator *it); + +/* Initialize a merged table iterator for reading logs. */ +void reftable_merged_table_init_log_iterator (struct reftable_merged_table *mt, +struct reftable_iterator *it);I might have missed it, but I do not see
reftable_merged_table_init_log_iterator()used anywhere in the later patches. Does this need to be added? Or are we just adding it because we want a companion function to the ref iterator to be more consistent?
The latter. It really should exist to make the public interface complete.
Patrick