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

>
> On 2/23/2021 6:43 PM, Elijah Newren via GitGitGadget wrote:
> > 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.
>
> > === 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.

> 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.