Perform ThinLTO on x86_64-pc-windows-msvc dist builder

[Zoxc]

View all comments

This reenables ThinLTO on x86_64-pc-windows-msvc.

try-job: dist-x86_64-apple

[rustbot]

r? @TaKO8Ki

rustbot has assigned @TaKO8Ki.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[jieyouxu]

FYI @rust-lang/infra

[lqd]

I don't think we're in a situation where we can yet have a definite yes/no answer for this PR yet.

Marking as blocked on #122790. The discussions and underlying issues there shouldn't be split amongst PRs.

@rustbot blocked

[bors]

☔ The latest upstream changes (presumably #130506) made this pull request unmergeable. Please resolve the merge conflicts.

[Zoxc]

@rustbot ready

This is no longer blocked, at least not on my previous PR.

Are the binaries built by dist jobs properly tested now?
Are there any other knowns issues with ThinLTO on Windows?

[Kobzol]

They are tested, not sure if properly :) Last time we tried it, one test was still miscompiled, we could try again.

• comment out DIST_TRY_BUILD insrc/ci/github-actions/jobs.yml
• add a line try-job: dist-x86_64-msvc to the PR description
• run a try build

[jieyouxu]

They are tested, not sure if properly :)

As far as I know, Windows dist artifacts are not tested at all (note that dist artifacts are not necessarily equivalent to test job artifacts; and usually they are not equivalent due to configuration differences). We do have Windows test jobs that run the full test suites but they aren't testing the dist artifacts.

EDIT: x86_64-pc-windows-msvc goes through opt-dist but not the other i686-pc-windows-msvc apparently.

Recently we broke i686-pc-windows-gnu rustc binaries in a way that caused rustc binary to completely not work (rustc -vV failed) but no tests caught it.

[lqd]

As far as I know, Windows dist artifacts are not tested at all

We do test dist x64 msvc artifacts in opt-dist.

[jieyouxu]

Huh. I must be thinking of x86_64 windows-gnu or maybe the 32 bit windows-msvc target?

EDIT: in any case you're right:

https://github.com/rust-lang/rust/blob/master/src%2Fci%2Fgithub-actions%2Fjobs.yml#L584

[Kobzol]

@bors try

[bors]

⌛ Trying commit 3d3aba0 with merge b9d7ae0...

[bors]

💔 Test failed - checks-actions

[Kobzol]

@bors try

[bors]

⌛ Trying commit a832199 with merge 359c9af...

[JonathanBrouwer]

@bors retry

[JonathanBrouwer]

@bors p=6
Scheduling

[rust-bors]

💔 Test for d031dae failed: CI. Failed job:

• auto - dist-x86_64-apple (web logs, enhanced plaintext logs)

[JonathanBrouwer]

^ Real failure
@bors r-

[rust-bors]

View changes since this unapproval

[Zoxc]

@bors try

[rust-bors]

☀️ Try build successful (CI)
Build commit: b3461f1 (b3461f11e8d00c8e9852917508ecab543cd07cce)
Base parent: 84b36a7 (84b36a78a28a63f134171c670be1932ffa2485f8)

[Kobzol]

@bors try jobs=dist-x86_64-linux @rust-timer queue

@bjorn3 Is it OK to also pass LTO flags when building proc macros?

[Zoxc]

Is it OK to also pass LTO flags when building proc macros?

We do pass this flag for rustc already, so presumedly it works. I don't think Cargo has a way to avoid flags for proc macros / build scripts, as LTO is likely overkill for them.

[rust-bors]

☀️ Try build successful (CI)
Build commit: f8b60f9 (f8b60f9d3af16a4d97c770bde4f0e28ed0591ac0)
Base parent: 88f7399 (88f7399cb4912680976e1c342ea7661b9dc84940)

[rust-timer]

Finished benchmarking commit (f8b60f9): comparison URL.

Overall result: ❌ regressions - no action needed

Benchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up.

@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

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

Max RSS (memory usage)

Results (primary 1.4%, secondary 0.2%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	3.3%	[1.5%, 5.1%]	2
Regressions ❌ (secondary)	4.3%	[0.4%, 6.8%]	3
Improvements ✅ (primary)	-2.5%	[-2.5%, -2.5%]	1
Improvements ✅ (secondary)	-0.7%	[-1.2%, -0.4%]	13
All ❌✅ (primary)	1.4%	[-2.5%, 5.1%]	3

Cycles

Results (secondary -0.7%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.8%	[0.6%, 1.2%]	4
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.0%	[-2.8%, -0.4%]	16
All ❌✅ (primary)	-	-	0

Binary size

Results (primary -0.0%, secondary -0.0%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.0%	[-0.0%, -0.0%]	14
Improvements ✅ (secondary)	-0.0%	[-0.1%, -0.0%]	5
All ❌✅ (primary)	-0.0%	[-0.0%, -0.0%]	14

Bootstrap: 460.09s -> 462.329s (0.49%)
Artifact size: 398.62 MiB -> 398.65 MiB (0.01%)