Re: [PATCH 5/9] reftable/block: move ownership of block reader into `struct table_iter`

> On 24/03/27 07:37AM, Patrick Steinhardt wrote:
> > The table iterator allows the caller to iterate through all records in a
> > reftable table. To do so it iterates through all blocks of the desired
> > type one by one, where for each block it creates a new block iterator
> > and yields all its entries.
> > 
> > One of the things that is somewhat confusing in this context is who owns
> > the block reader that is being used to read the blocks and pass them to
> > the block iterator. Intuitively, as the table iterator is responsible
> > for iterating through the blocks, one would assume that this iterator is
> > also responsible for managing the lifecycle of the reader. And while it
> > somewhat is, the block reader is ultimately stored inside of the block
> > iterator.
> > 
> > Refactor the code such that the block reader is instead fully managed by
> > the table iterator. Instead of passing the reader to the block iterator,
> > we now only end up passing the block data to it. Despite clearing up the
> > lifecycle of the reader, it will also allow for better reuse of the
> > reader in subsequent patches.
> > 
> > The following benchmark prints a single matching ref out of 1 million
> > refs. Before:
> > 
> >   HEAP SUMMARY:
> >       in use at exit: 13,603 bytes in 125 blocks
> >     total heap usage: 6,607 allocs, 6,482 frees, 509,635 bytes allocated
> > 
> > After:
> > 
> >   HEAP SUMMARY:
> >       in use at exit: 13,603 bytes in 125 blocks
> >     total heap usage: 7,235 allocs, 7,110 frees, 301,481 bytes allocated
> > 
> > Note that while there are more allocation and free calls now, the
> > overall number of bytes allocated is significantly lower. The number of
> > allocations will be reduced significantly by the next patch though.
> > 
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ...
> > @@ -340,14 +344,14 @@ void block_iter_copy_from(struct block_iter *dest, struct block_iter *src)
> >  int block_iter_next(struct block_iter *it, struct reftable_record *rec)
> >  {
> >  	struct string_view in = {
> > -		.buf = it->br->block.data + it->next_off,
> > -		.len = it->br->block_len - it->next_off,
> > +		.buf = (unsigned char *) it->block + it->next_off,
> 
> Would it be best to use the `uint8_t *` type instead of `unsigned char *`
> to match `string_view.buf`? Not sure if it matters in this case.

> > +		.len = it->block_len - it->next_off,
> >  	};
> ...  
> > diff --git a/reftable/block.h b/reftable/block.h
> > index 601a1e0e89..b41efa5042 100644
> > --- a/reftable/block.h
> > +++ b/reftable/block.h
> > @@ -84,16 +84,18 @@ int block_reader_init(struct block_reader *br, struct reftable_block *bl,
> >  void block_reader_release(struct block_reader *br);
> >  
> >  /* Returns the block type (eg. 'r' for refs) */
> > -uint8_t block_reader_type(struct block_reader *r);
> > +uint8_t block_reader_type(const struct block_reader *r);
> >  
> >  /* Decodes the first key in the block */
> > -int block_reader_first_key(struct block_reader *br, struct strbuf *key);
> > +int block_reader_first_key(const struct block_reader *br, struct strbuf *key);
> >  
> >  /* Iterate over entries in a block */
> >  struct block_iter {
> >  	/* offset within the block of the next entry to read. */
> >  	uint32_t next_off;
> > -	struct block_reader *br;
> > +	const unsigned char *block;
> 
> Same question here. Would it be better to use `uint8_t *`? Or does it not
> really matter?

> > +	size_t block_len;
> > +	int hash_size;
> >  
> >  	/* key for last entry we read. */
> >  	struct strbuf last_key;
> > @@ -106,17 +108,22 @@ struct block_iter {
> >  }
> >  
> >  /* Position `it` at start of the block */
> > -void block_iter_seek_start(struct block_iter *it, struct block_reader *br);
> > +void block_iter_seek_start(struct block_iter *it, const struct block_reader *br);
> >  
> >  /* Position `it` to the `want` key in the block */
> > -int block_iter_seek_key(struct block_iter *it, struct block_reader *br,
> > +int block_iter_seek_key(struct block_iter *it, const struct block_reader *br,
> >  			struct strbuf *want);
> >  
> > -void block_iter_copy_from(struct block_iter *dest, struct block_iter *src);
> > +void block_iter_copy_from(struct block_iter *dest, const struct block_iter *src);
> >  
> >  /* return < 0 for error, 0 for OK, > 0 for EOF. */
> >  int block_iter_next(struct block_iter *it, struct reftable_record *rec);
> >  
> > +/*
> > + * Reset the block iterator to pristine state without releasing its memory.
> > + */
> 
> Do we want to make the comment a single line to match the other adjacent
> examples?

> > +void block_iter_reset(struct block_iter *it);
> > +
> >  /* deallocate memory for `it`. The block reader and its block is left intact. */
> >  void block_iter_close(struct block_iter *it);
> >  
> ...
> 
> -Justin