Re: [PATCH 09/13] reftable/generic: move seeking of records into the iterator

From: Patrick Steinhardt <ps@pks.im>
Date: May 13, 2024, 08:36 UTC
Message-ID: <ZkHRCKwJiAfx0Z1a@tanuki>
In-Reply-To: <4pyzm53ioaqt5men72ti4ffu7zjbpigytfgcyg4h2q7657zoji@7hlrvlt2cws2>

On Fri, May 10, 2024 at 04:44:54PM -0500, Justin Tobler wrote:

Show 22 quoted lines

> 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."

Done.

[snip]

Show 12 quoted lines

> > 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?

Functions with the `reftable_` prefix are supposed to be public, whereas functions without them are private. So this is intentionally missing the prefix.

[snip]

Show 18 quoted lines

> > @@ -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?

Yes. The filtering ref iterator wraps a _different_ iterator, which is `fri->it` in the above case, and only returns a subset of the records of that wrapped iterator. So we eventually end up calling the callbacks of the wrapped iterator, which are likely not a filtering ref iterator themselves (even though that would in theory be possible).

Patrick