| Message ID | f797feff8dec383f1db9ae403cd89b80d1743432.1702047081.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | reftable: small set of fixes | expand |
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.) Thanks, Taylor
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
On Mon, Dec 11, 2023 at 10:08 AM Patrick Steinhardt <ps@pks.im> wrote: > > 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. In C++ it is expected that assignment operators clear the destination before executing the assignment, so it depends on your expectations. If this is confusing, maybe another name is in order?
diff --git a/reftable/stack.c b/reftable/stack.c index f5d18a842a..2dd2373360 100644 --- a/reftable/stack.c +++ b/reftable/stack.c @@ -204,6 +204,7 @@ static int reftable_stack_reload_once(struct reftable_stack *st, char **names, reftable_calloc(sizeof(struct reftable_table) * names_len); int new_readers_len = 0; struct reftable_merged_table *new_merged = NULL; + struct strbuf table_path = STRBUF_INIT; int i; while (*names) { @@ -223,13 +224,10 @@ static int reftable_stack_reload_once(struct reftable_stack *st, char **names, if (!rd) { struct reftable_block_source src = { NULL }; - struct strbuf table_path = STRBUF_INIT; stack_filename(&table_path, st, name); err = reftable_block_source_from_file(&src, table_path.buf); - strbuf_release(&table_path); - if (err < 0) goto done; @@ -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); reader_close(cur[i]); reftable_reader_free(cur[i]); /* On Windows, can only unlink after closing. */ - unlink(filename.buf); - - strbuf_release(&filename); + unlink(table_path.buf); } } @@ -288,6 +283,7 @@ static int reftable_stack_reload_once(struct reftable_stack *st, char **names, reftable_free(new_readers); reftable_free(new_tables); reftable_free(cur); + strbuf_release(&table_path); return err; }
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. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- reftable/stack.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-)