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