Stream: t-compiler

Topic: Review request for #68241


ecstatic-morse (Feb 08 2020 at 22:47, on Zulip):

#68241 been blocked on review from @pnkfelix for a few weeks now. That PR is in turn blocking significant perf improvements in #68528. Would someone be willing to review all or part of #68241? I say "part" because that PR includes several small, independent changes involving and debugging that are closely related to the borrowck dataflow changes but could be split out. cc @T-compiler

pnkfelix (Feb 09 2020 at 01:30, on Zulip):

Yeah I’m sorry, a bunch of my review requests have gone mishandled for far too long

pnkfelix (Feb 09 2020 at 01:31, on Zulip):

It didn’t help that I didn’t get as much free time at the Mozilla all hands as I was expected

pnkfelix (Feb 09 2020 at 01:32, on Zulip):

I won’t get to it until Monday at the earliest

ecstatic-morse (Feb 09 2020 at 04:23, on Zulip):

No worries. Just hoping to find someone who could work steal.

Wesley Wiser (Feb 09 2020 at 17:53, on Zulip):

@ecstatic-morse If you split out the first 5 commits into a separate PR, I'd be happy to r+ that. I may be able to review additional parts later as well.

ecstatic-morse (Feb 09 2020 at 18:42, on Zulip):

Thanks @Wesley Wiser! I split the graphviz changes into #69005.

Wesley Wiser (Feb 09 2020 at 18:44, on Zulip):

Reviewed & approved

pnkfelix (Feb 10 2020 at 20:44, on Zulip):

@ecstatic-morse just so we're on the same page regarding expectations for this PR : when I followed the perf.rlo link, the the task-clock and wall-times show a fair amount of red (i.e. regressions). They might be acceptable regressions, but reading the description of the PR itself, I wasn't expecting to see much regression at all.

pnkfelix (Feb 10 2020 at 20:44, on Zulip):

@ecstatic-morse am I misunderstanding the perf results? Or looking at an outdated link:?

ecstatic-morse (Feb 10 2020 at 21:01, on Zulip):

I expect this PR to be roughly performance neutral: some regressions due to additional boilerplate and a more fine-grained cursor offset by skipping block-transfer function caching.

cc @simulacrum I am assuming that the changes in wall-time measurements were due to variance. For example, here's the task-clock results for an initial version of the PR. This is without the MIR visitor or the block transfer function skipping. I don't think these results represent a true improvement, even though there is more green.

ecstatic-morse (Feb 10 2020 at 21:02, on Zulip):

Btw, lemme rebase away the graphviz changes

ecstatic-morse (Feb 10 2020 at 21:07, on Zulip):

If you like I can split out the block transfer function caching optimization and/or do another perf run

ecstatic-morse (Feb 10 2020 at 21:08, on Zulip):

Otherwise, we can always revert this if it actually causes a persistent wall-time regression. We'll be able to see it in the graphs.

pnkfelix (Feb 11 2020 at 01:45, on Zulip):

I mostly wanted clarity about your expectations

Last update: Jun 07 2020 at 10:40UTC