| Message ID | 167f67fad841ad06535a5532088fa6c9125fb1cd.1707726654.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | c68ca7abd30b22404ce59d5133566729c07ffe8f |
| Headers | show |
| Series | reftable: improve ref iteration performance | expand |
Patrick Steinhardt <ps@pks.im> writes: > While working on the optimizations in the preceding patches I stumbled > upon `table_iter_next()` multiple times. It is quite easy to miss the > fact that we don't call `table_iter_next_in_block()` twice, but that the > second call is in fact `table_iter_next_block()`. > > Add comments to explain what exactly is going on here to make things > more obvious. While at it, touch up the code to conform to our code > style better. > > Note that one of the refactorings merges two conditional blocks into > one. Before, we had the following code: > > ``` > err = table_iter_next_block(&next, ti ");"??? > if (err != 0) { > ti->is_finished = 1; > } > table_iter_block_done(ti); > if (err != 0) { > return err; > } > ``` > > As `table_iter_block_done()` does not care about `is_finished`, the > conditional blocks can be merged into one block: > > ``` > err = table_iter_next_block(&next, ti > table_iter_block_done(ti); > if (err != 0) { > ti->is_finished = 1; > return err; > } > ``` > > This is both easier to reason about and more performant because we have > one branch less. > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > --- > reftable/reader.c | 26 +++++++++++++++++--------- > 1 file changed, 17 insertions(+), 9 deletions(-) > > diff --git a/reftable/reader.c b/reftable/reader.c > index 64dc366fb1..add7d57f0b 100644 > --- a/reftable/reader.c > +++ b/reftable/reader.c > @@ -357,24 +357,32 @@ static int table_iter_next(struct table_iter *ti, struct reftable_record *rec) > > while (1) { > struct table_iter next = TABLE_ITER_INIT; > - int err = 0; > - if (ti->is_finished) { > + int err; > + > + if (ti->is_finished) > return 1; > - } > > + /* > + * Check whether the current block still has more records. If > + * so, return it. If the iterator returns positive then the > + * current block has been exhausted. > + */ > err = table_iter_next_in_block(ti, rec); > - if (err <= 0) { > + if (err <= 0) > return err; > - } > > + /* > + * Otherwise, we need to continue to the next block in the > + * table and retry. If there are no more blocks then the > + * iterator is drained. > + */ > err = table_iter_next_block(&next, ti); > - if (err != 0) { > - ti->is_finished = 1; > - } > table_iter_block_done(ti); > - if (err != 0) { > + if (err) { > + ti->is_finished = 1; > return err; > } > + > table_iter_copy_from(ti, &next); > block_iter_close(&next.bi); > }
On Mon, Feb 12, 2024 at 09:19:20AM -0800, Junio C Hamano wrote: > Patrick Steinhardt <ps@pks.im> writes: > > > While working on the optimizations in the preceding patches I stumbled > > upon `table_iter_next()` multiple times. It is quite easy to miss the > > fact that we don't call `table_iter_next_in_block()` twice, but that the > > second call is in fact `table_iter_next_block()`. > > > > Add comments to explain what exactly is going on here to make things > > more obvious. While at it, touch up the code to conform to our code > > style better. > > > > Note that one of the refactorings merges two conditional blocks into > > one. Before, we had the following code: > > > > ``` > > err = table_iter_next_block(&next, ti > > ");"??? Oh, right, seems to be a copy-paste error. I see you already fixed it in ps/reftable-iteration-perf and merged the branch to next. Thanks! Patrick
diff --git a/reftable/reader.c b/reftable/reader.c index 64dc366fb1..add7d57f0b 100644 --- a/reftable/reader.c +++ b/reftable/reader.c @@ -357,24 +357,32 @@ static int table_iter_next(struct table_iter *ti, struct reftable_record *rec) while (1) { struct table_iter next = TABLE_ITER_INIT; - int err = 0; - if (ti->is_finished) { + int err; + + if (ti->is_finished) return 1; - } + /* + * Check whether the current block still has more records. If + * so, return it. If the iterator returns positive then the + * current block has been exhausted. + */ err = table_iter_next_in_block(ti, rec); - if (err <= 0) { + if (err <= 0) return err; - } + /* + * Otherwise, we need to continue to the next block in the + * table and retry. If there are no more blocks then the + * iterator is drained. + */ err = table_iter_next_block(&next, ti); - if (err != 0) { - ti->is_finished = 1; - } table_iter_block_done(ti); - if (err != 0) { + if (err) { + ti->is_finished = 1; return err; } + table_iter_copy_from(ti, &next); block_iter_close(&next.bi); }
While working on the optimizations in the preceding patches I stumbled upon `table_iter_next()` multiple times. It is quite easy to miss the fact that we don't call `table_iter_next_in_block()` twice, but that the second call is in fact `table_iter_next_block()`. Add comments to explain what exactly is going on here to make things more obvious. While at it, touch up the code to conform to our code style better. Note that one of the refactorings merges two conditional blocks into one. Before, we had the following code: ``` err = table_iter_next_block(&next, ti if (err != 0) { ti->is_finished = 1; } table_iter_block_done(ti); if (err != 0) { return err; } ``` As `table_iter_block_done()` does not care about `is_finished`, the conditional blocks can be merged into one block: ``` err = table_iter_next_block(&next, ti table_iter_block_done(ti); if (err != 0) { ti->is_finished = 1; return err; } ``` This is both easier to reason about and more performant because we have one branch less. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- reftable/reader.c | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-)