Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

attempt to recover perf by removing exports_all_green #71267

Merged

Conversation

pnkfelix
Copy link
Member

@pnkfelix pnkfelix commented Apr 18, 2020

attempt to recover perf by removing exports_all_green flag.

cc #71248

(My hypothesis is that my use of this flag was an overly conservative generalization of PR #67020.)

…ll_green` flag.

(My hypothesis is that my use of this flag was an overly conservative
generalization of PR 67020.)
@rust-highfive
Copy link
Collaborator

r? @petrochenkov

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 18, 2020
@pnkfelix pnkfelix added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-incr-comp Area: Incremental compilation beta-nominated Nominated for backporting to the compiler in the beta channel. labels Apr 18, 2020
@pnkfelix
Copy link
Member Author

we should probably at least consider rolling this in with the current beta, if we can, considering it is a high priority follow-up to PR #71131

On the other hand, given that we were willing to let PR #71131 land without the performance issue addressed, it would also be entirely fine if we let this PR ride the trains.

still, marking as beta-nominated to make it clear that that this is a question that we should try to resolve across the team, if we can do so quickly...

cc @rust-lang/compiler

@Mark-Simulacrum
Copy link
Member

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Apr 18, 2020

⌛ Trying commit 1abfd4a with merge afdbd8d1f31ef963fbfe54dc8a987f5bee3f1a29...

@nagisa
Copy link
Member

nagisa commented Apr 18, 2020

@bors r+, but it would perhaps be great if we could explain why it is fine to remove this in a way that isn’t just a hunch.

@nagisa
Copy link
Member

nagisa commented Apr 18, 2020

err, @bors r-, but r=me on implementation

@bors
Copy link
Contributor

bors commented Apr 18, 2020

☀️ Try build successful - checks-azure
Build commit: afdbd8d1f31ef963fbfe54dc8a987f5bee3f1a29 (afdbd8d1f31ef963fbfe54dc8a987f5bee3f1a29)

@rust-timer
Copy link
Collaborator

Queued afdbd8d1f31ef963fbfe54dc8a987f5bee3f1a29 with parent ce93331, future comparison URL.

@petrochenkov
Copy link
Contributor

r? @nagisa

@rust-highfive rust-highfive assigned nagisa and unassigned petrochenkov Apr 18, 2020
@MaloJaffre
Copy link
Contributor

Perf results have landed, but there was no notification from the bot.

@eddyb
Copy link
Member

eddyb commented Apr 18, 2020

Assuming I've done this right, this is the outer/inner range breakdown:

The difference between those two should be the remaining regressions, and they are:

  • 2.9% on script-servo-opt "patched incremental: debugging println in dependency"
  • 1.2% on issue-46449-opt "patched incremental: u32 3072"

@pnkfelix
Copy link
Member Author

pnkfelix commented Apr 20, 2020

@bors r+, but it would perhaps be great if we could explain why it is fine to remove this in a way that isn’t just a hunch.

Yeah, I had some idle moments over the past few days wondering if this was truly justifiable, and if so, what the justification is.

With the code prior to this PR, the LTO optimization of a module is sensitive to (a.) what collection of modules it imports from (just as a set of names), (b.) what collection of modules it exports to (again, just as a set of names), (c.) whether the content of any of its imports has changed, and (d.) whether the content of any of the modules it exports to has changed.

With this PR in place, we keep conditions (a.), (b.) and (c.); only (d.) is removed.

We have lots of attached discussion justifying why we needed to add (a.) and (b.).

Condition (c.) has been there since the beginning, I think. The justification for (c.) is that LLVM is allowed to use the content of the modules we import from in the LTO optimization of the current module. (Otherwise there wouldn't be much point in doing link-time optimization, right?)

So the question is, can LLVM do optimizations based on the content of the modules that the current module exports to?

  • (What kind of optimization am I imagining here? One example would be if the current module is defining a function foo that takes a boolean flag as one of its argument, and then LLVM analyzes all of the client modules and determines that every one of them passes false for the value of flag; that knowledge would enable treating flag as a constant during LTO.)
  • I cannot help but link to one of my favorite (joke-y) papers on this topic: https://www.cs.princeton.edu/~appel/papers/conteq.pdf

Do I expect LLVM might be using this kind of optimization? I wasn't sure, so I decided to go check the docs. The docs then point to the LLVM blog post on ThinLTO, where it literally says:

any particular ThinLTO backend must be redone if and only if:

  1. The corresponding (primary) module’s bitcode changed
  2. The list of imports into or exports from the module changed
  3. The bitcode for any module being imported from has changed
  4. Any global analysis result affecting either the primary module or anything it imports has changed.

Note how items 2 and 3 there are written: item 2 corresponds exactly to conditions (a.) and (b.). Item 3 corresponds to condition (c.). And they do not say anything about changes to modules that we export to; that is, I think that serves as a promise from LLVM that we can drop condition (d.)

@pnkfelix
Copy link
Member Author

I'll add a commit with a comment that restates the above, perhaps a bit more tersely.

@pnkfelix
Copy link
Member Author

pnkfelix commented Apr 20, 2020

@bors r=nagisa rollup=never

(performance sensitive PR, thus rollup=never)

@bors
Copy link
Contributor

bors commented Apr 20, 2020

📌 Commit 19e5a65 has been approved by nagisa

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 20, 2020
@bors
Copy link
Contributor

bors commented Apr 20, 2020

💡 This pull request was already approved, no need to approve it again.

@bors
Copy link
Contributor

bors commented Apr 20, 2020

📌 Commit 19e5a65 has been approved by nagisa

@pnkfelix
Copy link
Member Author

pnkfelix commented Apr 20, 2020

Assuming I've done this right, this is the outer/inner range breakdown:

The difference between those two should be the remaining regressions, and they are:

  • 2.9% on script-servo-opt "patched incremental: debugging println in dependency"

  • 1.2% on issue-46449-opt "patched incremental: u32 3072"

Thanks @eddyb ! I'll try to look into these cases next!

(But I think its safe to say that these regressions we are probably willing to stomach in exchange for correctness...)

@Dylan-DPC-zz
Copy link

@bors p=1

@pnkfelix
Copy link
Member Author

after discussion with @Mark-Simulacrum , I'm going to insta-beta-accept this (that is, I'm bypassing the beta-nomination process).

I'm also marking this I-nominated so that we remember to discuss this at this week's Thursday meeting, so that if someone wants to object (either to the PR itself or to the process I followed, we will give the opportunity to do so there.

(Or they can just post comments here if they object too...)

@pnkfelix pnkfelix added beta-accepted Accepted for backporting to the compiler in the beta channel. I-nominated labels Apr 20, 2020
@Mark-Simulacrum Mark-Simulacrum removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Apr 20, 2020
@Mark-Simulacrum
Copy link
Member

Note that by that meeting it is highly likely we'll have already released this into stable -- so @rust-lang/compiler -- please chime in before then if you object to this getting released. (We can rebuild stable as late as ~Wednesday, most likely, though obviously the earlier the better).

bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 20, 2020
…imulacrum

[stable] 1.43.0 release

Includes a last minute backport of rust-lang#71267.
@bors
Copy link
Contributor

bors commented Apr 21, 2020

⌛ Testing commit 19e5a65 with merge 25f070d...

@bors
Copy link
Contributor

bors commented Apr 21, 2020

☀️ Test successful - checks-azure
Approved by: nagisa
Pushing 25f070d to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 21, 2020
@bors bors merged commit 25f070d into rust-lang:master Apr 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-incr-comp Area: Incremental compilation beta-accepted Accepted for backporting to the compiler in the beta channel. merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants