Re: [PATCH 09/13] reftable/generic: move seeking of records into the iterator
From: Justin Tobler <hidden>
Date: 2024-05-10 21:46:20
On 24/05/08 01:04PM, Patrick Steinhardt wrote:
Reftable iterators are created by seeking on the parent structure of a corresponding record. For example, to create an iterator for the merged table you would call `reftable_merged_table_seek_ref()`. Most notably, it is not posible to create an iterator and then seek it afterwards. While this may be a bit easier to reason about, it comes with two significant downsides. The first downside is that the logic to find records is split up between the parent data structure and the iterator itself. Conceptually, it is more straight forward if all that logic was contained in a single place, which should be the iterator. The second and more significant downside is that it is impossible to reuse iterators for multiple seeks. Whenever you want to look up a record, you need to re-create the whole infrastructure again, which is quite a waste of time. Furthermore, it is impossible to for example optimize seeks, for example when seeking the same record multiple times.
The last setence could use some rewording. "Furthermore, it is impossible to optimize seeks, such as when seeking the same record multiple times."
quoted hunk ↗ jump to hunk
To address this, we essentially split up the concerns properly such that the parent data structure is responsible for setting up the iterator via a new `init_iter()` callback, whereas the iterator handles seeks via a new `seek()` callback. This will eventually allow us to call `seek()` on the iterator multiple times, where every iterator can potentially optimize for certain cases. Note that at this point in time we are not yet ready to reuse the iterators. This will be left for a future patch series. Signed-off-by: Patrick Steinhardt <redacted> --- reftable/generic.c | 47 +++++++++++++---- reftable/generic.h | 9 +++- reftable/iter.c | 15 ++++++ reftable/merged.c | 97 +++++++++++++++++----------------- reftable/reader.c | 126 +++++++++++++++++++++++++-------------------- 5 files changed, 177 insertions(+), 117 deletions(-)diff --git a/reftable/generic.c b/reftable/generic.c index b9f1c7c18a..1cf68fe124 100644 --- a/reftable/generic.c +++ b/reftable/generic.c@@ -12,25 +12,39 @@ license that can be found in the LICENSE file or at #include "reftable-iterator.h" #include "reftable-generic.h" +void table_init_iter(struct reftable_table *tab,
The following table related functions are prefixed with `reftable_`. Do we want to do the same here?
+ struct reftable_iterator *it,
+ uint8_t typ)
+{
+
+ tab->ops->init_iter(tab->table_arg, it, typ);
+}
+
int reftable_table_seek_ref(struct reftable_table *tab,
struct reftable_iterator *it, const char *name)
{
- struct reftable_record rec = { .type = BLOCK_TYPE_REF,
- .u.ref = {
- .refname = (char *)name,
- } };
- return tab->ops->seek_record(tab->table_arg, it, &rec);
+ struct reftable_record want = {
+ .type = BLOCK_TYPE_REF,
+ .u.ref = {
+ .refname = (char *)name,
+ },
+ };
+ table_init_iter(tab, it, BLOCK_TYPE_REF);
+ return it->ops->seek(it->iter_arg, &want);
}
int reftable_table_seek_log(struct reftable_table *tab,
struct reftable_iterator *it, const char *name)
{
- struct reftable_record rec = { .type = BLOCK_TYPE_LOG,
- .u.log = {
- .refname = (char *)name,
- .update_index = ~((uint64_t)0),
- } };
- return tab->ops->seek_record(tab->table_arg, it, &rec);
+ struct reftable_record want = {
+ .type = BLOCK_TYPE_LOG,
+ .u.log = {
+ .refname = (char *)name,
+ .update_index = ~((uint64_t)0),
+ },
+ };
+ table_init_iter(tab, it, BLOCK_TYPE_LOG);
+ return it->ops->seek(it->iter_arg, &want);
}
int reftable_table_read_ref(struct reftable_table *tab, const char *name,...
quoted hunk ↗ jump to hunk
@@ -23,6 +23,13 @@ static void filtering_ref_iterator_close(void *iter_arg) reftable_iterator_destroy(&fri->it); } +static int filtering_ref_iterator_seek(void *iter_arg, + struct reftable_record *want) +{ + struct filtering_ref_iterator *fri = iter_arg; + return iterator_seek(&fri->it, want); +}
I've found the `filtering_ref_iterator_seek()` here to be a little confusing. At first, I assumed that the `filtering_ref_iterator` would have referenced `filtering_ref_iterator_vtable` thus resulting in a cycle, but on closer inspection this does not seem to be the case and is in face always set to some other iterator operation. Am I understanding this correctly? -Justin