Skip to content

Enable ThinLTO for rustc on x64 msvc - #103591

Merged
bors merged 1 commit into
rust-lang:masterfrom
lqd:win-lto
Dec 11, 2022
Merged

Enable ThinLTO for rustc on x64 msvc#103591
bors merged 1 commit into
rust-lang:masterfrom
lqd:win-lto

Conversation

@lqd

@lqd lqd commented Oct 26, 2022

Copy link
Copy Markdown
Member

This applies the great work from @bjorn3 and @Kobzol in #101403 to x64 msvc.

Here are the local results for the try build 68c5c85ed759334a11f0b0e586f5032a23f85ce4, compared to its parent 0a6b941df354c59b546ec4c0d27f2b9b0cb1162c. Looking better than my previous local builds.

image

(I can't show cycle counts, as that option is failing on the windows version of the perf collector, but I'll try to analyze and debug this soon)

This will be the first of a few tests for rustc / llvm / both ThinLTO on the windows and mac targets.

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Oct 26, 2022
@lqd

lqd commented Oct 26, 2022

Copy link
Copy Markdown
Member Author

@bors try

@bors

bors commented Oct 26, 2022

Copy link
Copy Markdown
Collaborator

⌛ Trying commit 0abf95bdd7e7f98b53c310649ecb03bc60d986f6 with merge 00df0000c5e43d256786737e9f49379e1b3054c4...

@bors

bors commented Oct 26, 2022

Copy link
Copy Markdown
Collaborator

💔 Test failed - checks-actions

@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 26, 2022
@rust-log-analyzer

This comment was marked as outdated.

@lqd

lqd commented Oct 26, 2022

Copy link
Copy Markdown
Member Author

@bors try

@bors

bors commented Oct 26, 2022

Copy link
Copy Markdown
Collaborator

⌛ Trying commit 0e16a37a9f376091f4573feb33d0bcd603c811e8 with merge 68c5c85ed759334a11f0b0e586f5032a23f85ce4...

@bors

bors commented Oct 26, 2022

Copy link
Copy Markdown
Collaborator

☀️ Try build successful - checks-actions
Build commit: 68c5c85ed759334a11f0b0e586f5032a23f85ce4 (68c5c85ed759334a11f0b0e586f5032a23f85ce4)

@lqd

lqd commented Oct 26, 2022

Copy link
Copy Markdown
Member Author

To compare with the size increase we see on linux:

  • rustc_driver.dll: 3% increase (≈4MB), from 124 069 888 to 128 485 888
  • rustc_driver.pdb: 5% increase (≈4MB), from 87 977 984 to 92 418 048

@lqd lqd changed the title [perf] Enable ThinLTO for rustc on x64 msvc Enable ThinLTO for rustc on x64 msvc Oct 26, 2022
@lqd

lqd commented Oct 26, 2022

Copy link
Copy Markdown
Member Author

I've updated the PR description with the results of the try build, and dropped the temporary CI hacks.

I will look at CI time tomorrow, as well as the sizes of the rustc-dev component, just in case we see an increase like we did on linux. Otherwise, I think this is ready to review, feel free to re-roll:

r? @Mark-Simulacrum

@lqd
lqd marked this pull request as ready for review October 26, 2022 23:41
@lqd

lqd commented Oct 27, 2022

Copy link
Copy Markdown
Member Author

For CI time, it's going to be hard to say with a sample of 1, and t-infra should have better data like the variance on that builder, but here goes: on the 5 most recent merged PRs on the dist-x86_64-msvc builder, the "run the build" step took: 1h46, 2h09, 1h45, 2h10, 2h02. The try build above took 2h15.

For the rustup rustc-dev component:

  • parent 0a6b941df354c59b546ec4c0d27f2b9b0cb1162c: 175.03 MB
  • try build 68c5c85ed759334a11f0b0e586f5032a23f85ce4: 253.46 MB

So it's a similar scale to #103538, which saw an 80MB increase. The possible fix mentioned in that issue would work on all targets I assume.

I wouldn't think it's a blocking issue though.

@lqd

lqd commented Dec 7, 2022

Copy link
Copy Markdown
Member Author

@rustbot ready

I don't have access to the MSVC builder times graph that t-infra has, but from the tests above, it seems the additional time wouldn't necessarily be impactful on CI times, and that we could land this change @Mark-Simulacrum ?

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 7, 2022
@Mark-Simulacrum

Copy link
Copy Markdown
Member

This is the current state of the world:

image

So msvc dist is ~3rd slowest today. I think this should be OK to merge though, we can adjust further as we iterate on the CI times -- I expect our planned improvements around LLVM caching on Linux should fairly cleanly map directly to the msvc builder here.

@bors r+

@bors

bors commented Dec 8, 2022

Copy link
Copy Markdown
Collaborator

📌 Commit 684663e has been approved by Mark-Simulacrum

It is now in the queue for this repository.

@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 Dec 8, 2022
@bors

bors commented Dec 8, 2022

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 684663e with merge da2270c1464b4af6c57a1193a1600cd7dd352c98...

@bors

bors commented Dec 8, 2022

Copy link
Copy Markdown
Collaborator

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 8, 2022
@ChrisDenton

Copy link
Copy Markdown
Member

@bors retry spurious toolstate failure (see Zulip thread)

@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 Dec 8, 2022
@rust-log-analyzer

This comment was marked as outdated.

@bors

bors commented Dec 11, 2022

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 684663e with merge 657eefe...

@bors

bors commented Dec 11, 2022

Copy link
Copy Markdown
Collaborator

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 657eefe to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 11, 2022
@bors
bors merged commit 657eefe into rust-lang:master Dec 11, 2022
@rustbot rustbot added this to the 1.68.0 milestone Dec 11, 2022
@rust-timer

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (657eefe): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
3.0% [3.0%, 3.0%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-1.3% [-2.0%, -0.7%] 2
All ❌✅ (primary) - - 0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.1% [-2.1%, -2.1%] 1
All ❌✅ (primary) - - 0

@lqd
lqd deleted the win-lto branch December 11, 2022 22:06
@nnethercote

Copy link
Copy Markdown
Contributor

Note: the perf CI result showed no change because it only measures on Windows. See the image at the top of this PR for some previously-measured Windows results.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-testsuite Area: The testsuite used to check the correctness of rustc 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-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants