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

From: Elijah Newren <newren@gmail.com>
To: Derrick Stolee <stolee@gmail.com>
Cc: Elijah Newren via GitGitGadget <gitgitgadget@gmail.com>,
	Git Mailing List <git@vger.kernel.org>
Subject: Re: [PATCH v2 00/10] Optimization batch 8: use file basenames even more
Date: Wed, 24 Feb 2021 17:38:52 -0800	[thread overview]
Message-ID: <CABPp-BH0NABbHqY1ZfcatiEryrqXLbv=qQKJKKzsGp-4=tWTEA@mail.gmail.com> (raw)
In-Reply-To: <22419bc6-d35a-7537-d388-d51c24138f5f@gmail.com>

On Wed, Feb 24, 2021 at 9:50 AM Derrick Stolee <stolee@gmail.com> wrote:
>
> 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.

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.