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.
Thanks,
-Stolee
