attempt to recover perf by removing exports_all_green

[pnkfelix]

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

[rust-highfive]

r? @petrochenkov

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

[pnkfelix]

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]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion

[bors]

⌛ Trying commit 1abfd4a with merge afdbd8d1f31ef963fbfe54dc8a987f5bee3f1a29...

[nagisa]

@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]

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

[bors]

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

[rust-timer]

Queued afdbd8d1f31ef963fbfe54dc8a987f5bee3f1a29 with parent ce93331, future comparison URL.

[petrochenkov]

r? @nagisa

[MaloJaffre]

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

[eddyb]

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

• "outer", i.e. before the regression to after the fix (this PR)
• "inner", i.e. after the regression to before the fix (this PR)

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]

@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]

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

[pnkfelix]

@bors r=nagisa rollup=never

(performance sensitive PR, thus rollup=never)

[bors]

📌 Commit 19e5a65 has been approved by nagisa

[bors]

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

• There's another pull request that is currently being tested, blocking this pull request: A big options clean-up #70729

[bors]

📌 Commit 19e5a65 has been approved by nagisa

[pnkfelix]

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

• "outer", i.e. before the regression to after the fix (this PR)

• "inner", i.e. after the regression to before the fix (this PR)

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]

@bors p=1

[pnkfelix]

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

[Mark-Simulacrum]

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]

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

[bors]

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