Re: [PATCH v2 00/10] Optimization batch 8: use file basenames even more

On Wed, Feb 24, 2021 at 9:50 AM Derrick Stolee [off-list ref] wrote:

On 2/23/2021 6:43 PM, Elijah Newren via GitGitGadget wrote:

quoted

This series depends on en/diffcore-rename (a concatenation of what I was
calling ort-perf-batch-6 and ort-perf-batch-7).

There are no changes since v1; it's just a resend a week and a half later to
bump it so it isn't lost.

Thank you for re-sending. I intended to review it before but got redirected
and forgot to pick it up again.

quoted

=== Results ===

For the testcases mentioned in commit 557ac0350d ("merge-ort: begin
performance work; instrument with trace2_region_* calls", 2020-10-28), the
changes in just this series improves the performance as follows:

                     Before Series           After Series
no-renames:       12.775 s ±  0.062 s    12.596 s ±  0.061 s
mega-renames:    188.754 s ±  0.284 s   130.465 s ±  0.259 s
just-one-mega:     5.599 s ±  0.019 s     3.958 s ±  0.010 s


As a reminder, before any merge-ort/diffcore-rename performance work, the
performance results we started with (as noted in the same commit message)
were:

no-renames-am:      6.940 s ±  0.485 s
no-renames:        18.912 s ±  0.174 s
mega-renames:    5964.031 s ± 10.459 s
just-one-mega:    149.583 s ±  0.751 s

These are good results.

I reviewed the patches and believe they do the optimizations claimed. I
only found some nits for comments and whitespace things.

Thanks for taking a look; I'll get those fixed up.  Also, I think your
performance improvement of switching from xstrfmt to a few strbuf
calls, even if small, counts as more than "nits for comments and
whitespace things".  :-)

You are very careful to create the necessary pieces and connect them
from the bottom-up. However, this leads to one big "now everything is
done" commit with performance improvements. It seems that there are
some smaller performance improvements that could be measured if the
logic was instead built from the top-down with stubs for the complicated
logic.

For example, the final patch links the rename logic with a call to
idx_possible_rename(). But, that could just as well always return -1
and the implementation would be correct. Then, it would be good to see
if the performance changes with that non-functional update. It would
also help me read the series in patch order and understand the context
of the methods a bit better before seeing their implementation.

This is _not_ a recommendation that you rewrite the series. Just food
for thought as we continue with similar enhancements in the future.

I can give it a shot for future relevant patch series (some of the
series this wouldn't be relevant for because they just include a
collection of patches implementing separate improvements that are just
batched together).  A couple of the series are already structured this
way, in fact, but the next series after this one has one patch that I
think I could reorder to make it more like this.