On Fri, Dec 08, 2023 at 05:17:21PM -0500, Taylor Blau wrote:
> On Fri, Dec 08, 2023 at 03:53:18PM +0100, Patrick Steinhardt wrote:
> > In `reftable_stack_reload_once()` we iterate over all the tables added
> > to the stack in order to figure out whether any of the tables needs to
> > be reloaded. We use a set of buffers in this context to compute the
> > paths of these tables, but discard those buffers on every iteration.
> > This is quite wasteful given that we do not need to transfer ownership
> > of the allocated buffer outside of the loop.
> >
> > Refactor the code to instead reuse the buffers to reduce the number of
> > allocations we need to do.
>
> > @@ -267,16 +265,13 @@ static int reftable_stack_reload_once(struct reftable_stack *st, char **names,
> > for (i = 0; i < cur_len; i++) {
> > if (cur[i]) {
> > const char *name = reader_name(cur[i]);
> > - struct strbuf filename = STRBUF_INIT;
> > - stack_filename(&filename, st, name);
> > + stack_filename(&table_path, st, name);
>
> This initially caught me by surprise, but on closer inspection I agree
> that this is OK, since stack_filename() calls strbuf_reset() before
> adjusting the buffer contents.
>
> (As a side-note, I do find the side-effect of stack_filename() to be a
> little surprising, but that's not the fault of this series and not worth
> changing here.)
Agreed, I also found this to be a bit confusing at first. I'll amend the
commit message with "Note that we do not have to manually reset the
buffer because `stack_filename()` does this for us already." to help
future readers.
Patrick
Attachment:
signature.asc
Description: PGP signature
