Skippable lints: account for lint capping and use in more places - #160126
Skippable lints: account for lint capping and use in more places#160126joshtriplett wants to merge 5 commits into
Conversation
|
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
|
r? @wesleywiser rustbot has assigned @wesleywiser. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
@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.
Skippable lints: account for lint capping and use in more places
This comment has been minimized.
This comment has been minimized.
|
💥 Test timed out after |
This comment has been minimized.
This comment has been minimized.
Dependencies get built with `--cap-lints allow`, so there's value in allowing capped lints to be skipped, even if the crate itself doesn't allow them.
`late_lint_mod` constructs `LateContext` immediately, before figuring out if the whole pass will be skipped. Move its construction into `late_lint_mod_inner`, so it doesn't get called if the pass gets skipped.
These lints will be skippable when building a crate with `--cap-lints allow`, which occurs for every dependency crate. In such cases, we can skip the whole query.
`-Zfuture-incompat-test` forces non-allow lints to report as future-compat. Unfortunately, rather than doing so by modifying the lints themselves, it requires checking separately. Check it and prevent skipping lints if enabled.
a0268f6 to
da92bde
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
I reverted the skip of |
|
@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.
Skippable lints: account for lint capping and use in more places
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (9162c22): 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 -2.4%, secondary 1.3%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -0.3%, secondary -0.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: 492.09s -> 489.291s (-0.57%) |
|
Perf looks somewhere between neutral and win to me. The bigger win is when building dependencies, which uses Accounting for cap-lints also (for instance) allows crates that explicitly ask for warnings about missing docs to not do the work to produce those warnings when built as a dependency, and various other cases. |
|
I tried something like this a few times, and my conclusion was that the remaining lints to be skipped are so fast that it's not worth it. But that of course depends on the exact crates that I tested it on. |
|
I've also tried things like this (if you remember, Jakub), and similarly it didn't make much of a difference on many crates then, and only on clean builds of course. We may need to look for outliers on crates.io to see if there are others. |
|
☔ The latest upstream changes (presumably #160310) made this pull request unmergeable. Please resolve the merge conflicts by rebasing. |
View all comments
skippable_lintsallows the compiler to skip doing some work if it only supports a lint that the compiler won't emit. However,skippable_lintsdidn't take--cap-lintsinto account. Since dependencies build with--cap-lints allow, it's taking that into account to allow skipping more code when building dependencies.The rest of this PR adds some additional uses of
skippable_lintsto skip some potentially expensive operations.All code in this PR is 100% human-written by me. I used an AI to search for potential performance issues when building an exceptionally large crate (
aws-sdk-ec2).