Enable polonius alpha on nightly - #159343
Conversation
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Enable polonius alpha on nightly
This comment has been minimized.
This comment has been minimized.
|
@craterbot check p=1 (priority bump b/c we want to get this enabled for testing) |
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
Finished benchmarking commit (24e1a22): comparison URL. Overall result: ❌✅ regressions and improvements - please read:Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf. Next, please: If you can, justify the regressions found in this try perf run in writing along with @bors rollup=never rustc-perf Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 4.2%, secondary 3.2%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 4.2%, secondary 4.6%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 487.651s -> 492.438s (0.98%) |
|
The way we enabled nightly-only for LLD was through a build configuration in bootstrap, I think that's a more explicit way, and will be easier to revert/modify on beta/stable, than to have that code in the compiler. If we did it like this PR does currently, then Polonius would be used by default if you use If we did it through bootstrap, then there would be an env variable or a cfg that would be set by bootstrap at rustc build time, which would select the default mode and hardcode it when we build rustc. |
|
@lqd said something similar to me. I'll take stab at it. As to whether |
|
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
🎉 Experiment
Footnotes
|
|
@craterbot check p=2 crates=https://crater-reports.s3.amazonaws.com/pr-159343/retry-regressed-list.txt name=pr-159343-retry Let's recheck this to see how we're doing. Wondering if these are performance-related. |
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
0006999 to
3379bb5
Compare
|
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
3379bb5 to
f7d91a7
Compare
|
🎉 Experiment
Footnotes
|
|
@bors try @rust-timer queue Just to check whether it still works in the distributed artifacts. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Enable polonius alpha on nightly
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (ecff631): comparison URL. Overall result: ❌✅ regressions and improvements - please read:Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf. Next, please: If you can, justify the regressions found in this try perf run in writing along with @bors rollup=never rustc-perf Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 3.5%, secondary 1.6%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 3.2%, secondary 2.7%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 489.577s -> 493.82s (0.87%) |
|
@bors p=1 Bump because it has been announced Bumping this slightly, because the blog post about Polonius being enabled on default already went out. |
This comment has been minimized.
This comment has been minimized.
This comment was marked as off-topic.
This comment was marked as off-topic.
|
📣 Toolstate changed by #159343! Tested on commit 7608eb7. 💔 nomicon on windows: test-pass → test-fail (cc @traviscross @JohnTitor). |
Tested on commit rust-lang/rust@7608eb7. Direct link to PR: <rust-lang/rust#159343> 💔 nomicon on windows: test-pass → test-fail (cc @traviscross @JohnTitor). 💔 nomicon on linux: test-pass → test-fail (cc @traviscross @JohnTitor).
|
A job failed! Check out the build log: (web) (plain enhanced) (plain) Click to see the possible cause of the failure (guessed by this bot) |
|
Finished benchmarking commit (7608eb7): comparison URL. Overall result: ❌✅ regressions and improvements - please read:Our benchmarks found a performance regression caused by this PR. Next Steps:
@rustbot label: +perf-regression Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 4.3%, secondary 19.3%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 4.6%, secondary 11.5%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary 0.0%, secondary 0.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 456.607s -> 459.618s (0.66%) |
I know I know. It's not great, but also not terrible on average, and we're doing more work for hopefully improvements in precision (sometimes a lot more work as shown in a few benchmarks or the outliers we're finding on crates.io in a similar manner to the new solver). It's only on nightly for now, and things should improve before stabilization, especially if we get help from @rust-lang/wg-compiler-performance cough @nnethercote cough as things are generally in a good enough state for perf work to be possible, and impactful. Just doing that work also has yielded performance improvements in previous PRs that we can't see in this PR. Or possible future work like: trying to avoid doing the computation one loan at a time, improving liveness to also not work one local at a time, improvements to the dense bitsets for small domains, improvements to dataflow, etc. I won't mark this as triaged, but I believe the overall sentiment is that we should take the regressions and improve things. |
|
FWIW I tried to look into serde (the biggest regression) and there's a lot more stuff happening using IntervalSet (which is used in dataflow). I don't have as much time to look at it right now, but here's at least some notes I collected while looking at the cachegrind diff between serde vs serde-polonius Check Full, if somebody is curious: b0a892a I don't think IntervalSet is super optimized for this atm so maybe there's some easy wins and fast paths to add, but in general, I don't actually know the state of Polonius perf work, whether somebody had looked into this before in detail. |
|
I'll take a look on Monday. |
|
Our MCP list also showed walltimes, and https://perf.rust-lang.org/compare.html?start=87212cef77e7bfa92ba0c4850be1d089745ba6fc&end=7608eb7b07eaf93f16d7cf5bcb2098eca87503df&stat=wall-time&showRawData=true&improvements=false has only 25 regressions, with a max of 10% on serde_derive (an absolute difference of 60ms). To me, it’s not as bad as it looks. |
|
Remind me, are |
|
I hear they're quite popular, on stable. Maybe as popular as fighting the borrow checker? But feel free to postpone this by inverting this line though, it's fine. And/or discuss with the actual author of this PR 👍. |
View all comments
See rust-lang/compiler-team#
The first commit here adds an
-Zpolonius=nllargument for tests and so people can disable alpha on nightly.