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

From: Derrick Stolee <stolee@gmail.com>
Date: Feb 24, 2021, 17:50 UTC
Message-ID: <22419bc6-d35a-7537-d388-d51c24138f5f@gmail.com>
In-Reply-To: <pull.844.v2.git.1614123848.gitgitgadget@gmail.com>

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

Show 5 quoted lines

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

Show 20 quoted lines

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