Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(Big performance change) Do not run lints that cannot emit #125116

Merged
merged 7 commits into from
Oct 26, 2024

Conversation

blyxyas
Copy link
Member

@blyxyas blyxyas commented May 14, 2024

Before this change, adding a lint was a difficult matter because it always had some overhead involved. This was because all lints would run, no matter their default level, or if the user had #![allow]ed them. This PR changes that. This change would improve both the Rust lint infrastructure and Clippy, but Clippy will see the most benefit, as it has about 900 registered lints (and growing!)

So yeah, with this little patch we filter all lints pre-linting, and remove any lint that is either:

  • Manually #![allow]ed in the whole crate,
  • Allowed in the command line, or
  • Not manually enabled with #[warn] or similar, and its default level is Allow

As some lints need to run, this PR also adds loadbearing lints. On a lint declaration, you can use the @eval_always = true marker to label it as loadbearing. A loadbearing lint will never be filtered (it will always run)

Fixes #106983

@rustbot
Copy link
Collaborator

rustbot commented May 14, 2024

r? @michaelwoerister

rustbot has assigned @michaelwoerister.
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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels May 14, 2024
@rustbot
Copy link
Collaborator

rustbot commented May 14, 2024

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@blyxyas
Copy link
Member Author

blyxyas commented May 14, 2024

cc @nnethercote @Kobzol, the perf wizards. Could you please give this PR a look and tell me if there are any obvious performance issues on the filtering?

@blyxyas blyxyas marked this pull request as draft May 14, 2024 11:23
@matthiaskrgr
Copy link
Member

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 14, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request May 14, 2024
…r=<try>

(Big performance change) Do not run lints that cannot emit

Before this lint, adding a lint was a difficult matter because it always had some overhead involved. This was because all lints would run, no matter their default level, or if the user had `#![allow]`ed them. This PR changes that. This change would improve both the Rust lint infrastructure and Clippy, but Clippy will see the most benefit, as it has about 900 registered lints (and growing!)

So yeah, with this little patch we filter all lints pre-linting, and remove any lint that is either:
- Manually `#![allow]`ed in the whole crate,
- Allowed in the command line, or
- Not manually enabled with `#[warn]` or similar, and its default level is `Allow`

I open this PR to receive some feedback, mainly related to performance. We have lots of `Lock`s, `with_lock` and similar functions (also lots of cloning), so the filtering performance is not the best.

In an older iteration, instead of doing this in the parsing phase, we developed a visitor with the same function but without so many locks, would reverting to that change help? I'm not sure tbh.
@bors
Copy link
Contributor

bors commented May 14, 2024

⌛ Trying commit 7606f89 with merge cc1d40f...

@rust-log-analyzer

This comment has been minimized.

@Kobzol
Copy link
Contributor

Kobzol commented May 14, 2024

@lqd haven't you tried something like this before? 🤔

@bors
Copy link
Contributor

bors commented May 14, 2024

☀️ Try build successful - checks-actions
Build commit: cc1d40f (cc1d40f134ee8336cbb7c7561deaed4aa5906e0e)

@rust-timer

This comment has been minimized.

@lqd
Copy link
Member

lqd commented May 14, 2024

@lqd haven't you tried something like this before? 🤔

We've tried a few different things yes, and so has blyxyas -- it maybe wasn't exactly like this, but I encountered annoyances like: some slow const eval loadbearing lint that shouldn't be ignored, lints that would be allowed unexpectedly because cargo allows lints unconditionally on dependencies (arguably the most common usage, and where perf gains would show up AFAICT) but some may trigger FCWs or are required to lint on dependencies despite being allowed, et cetera.

Refactoring and fixing all these were too costly compared to the gains at the time, as rustc's lints were fast enough on dependencies, also a "rarer" use-case. That being said, we've added and uplifted more lints since then, including possibly costly ones like the non local impls one, and the situation may also be different for clippy itself (but we won't see that in the perf.rlo results, only locally with the clippy dedicated commands IIUC)

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (cc1d40f): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
0.3% [0.2%, 0.5%] 25
Regressions ❌
(secondary)
0.4% [0.1%, 1.6%] 11
Improvements ✅
(primary)
-0.4% [-0.4%, -0.4%] 1
Improvements ✅
(secondary)
-0.1% [-0.1%, -0.1%] 1
All ❌✅ (primary) 0.3% [-0.4%, 0.5%] 26

Max RSS (memory usage)

Results (primary 2.3%, secondary -0.5%)

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)
2.3% [2.3%, 2.3%] 1
Regressions ❌
(secondary)
3.6% [2.0%, 5.3%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-4.7% [-5.2%, -4.1%] 2
All ❌✅ (primary) 2.3% [2.3%, 2.3%] 1

Cycles

Results (secondary 2.4%)

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)
2.4% [1.5%, 3.1%] 6
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Binary size

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

Bootstrap: 676.788s -> 676.098s (-0.10%)
Artifact size: 316.11 MiB -> 316.15 MiB (0.01%)

@rustbot rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels May 14, 2024
@Centri3
Copy link
Member

Centri3 commented May 14, 2024

The benchmark doesn't check clippy, right? As lqd hinted at as well? And without splitting allow-by-default rustc lints it does nothing without clippy, so I think this just shows how much time it takes to filter them (can someone else confirm this :3c)

Thus, basically nothing it seems :3 (So @blyxyas maybe the cloning is ok?)

@blyxyas
Copy link
Member Author

blyxyas commented May 15, 2024

The benchmark doesn't check clippy, right?

Yeah, the benchmarks currently doesn't check Clippy, that's why I'm currently benchmarking on a different server via SSH (A server that we got explicitly to benchmark Clippy). I'll post the results here when they arrive :)

Also, it currently doesn't check builtin lints because I'm having some issues checking that. That's also part of why I decided to open the PR, maybe someone has some idea (I'll see if I can read the previous attempts by lqd, maybe I can learn something from them)

EDIT: Seems like lqd hasn't pushed their attempts, I'll have to keep trying new approaches by myself.

@blyxyas
Copy link
Member Author

blyxyas commented May 15, 2024

Okis, here are the results (Wall time, Clippy)

Wall time


[ +0.45%, +190.50%]
+9.60% 81 (36)

[-73.80%, -0.44%]
-7.00% 77 (36)
❌,✅
[-73.80%, +190.50%]
+1.51% 158 (44)

Max RSS


[ +0.40%, +2.80%]
+1.17% 21 (16)

[ -5.15%, -0.44%]
-1.60% 39 (24)
❌,✅
[ -5.15%, +2.80%]
-0.63% 60 (31)

Instructions


[ +0.42%, +0.62%]
+0.52% 6 (4)

[ -1.82%, -0.32%]
-0.74% 13 (7)
❌,✅
[ -1.82%, +0.62%]
-0.34% 19 (11)

Cycles

[ +0.42%, +25.48%]
+4.33% 91 (38)

[-14.52%, -0.40%]
-3.33% 59 (32)
❌,✅
[-14.52%, +25.48%]
+1.31% 150 (43)

@blyxyas
Copy link
Member Author

blyxyas commented May 15, 2024

Those wall times are proof that this optimization has a lot of potential, the main drawback is that the filtering / parsing code is not fast enough, so in some scenarios that I'm not really able to determine exactly what do they have in common, the optimization goes backwards.

But a ~70% in Wall time, that's great and we should look more into it.

@Kobzol
Copy link
Contributor

Kobzol commented May 15, 2024

I wouldn't draw too many conclusions from these results, they seem to be quite unstable (there is also a 190% walltime regression). Note that even for PRs that don't have large perf. impacts, we can see ~30% walltime swings even on the stable benchmarking server (https://perf.rust-lang.org/compare.html?start=9e7aff794539aa040362f4424eb29207449ffce0&end=44fa5fd39a1d2af41bd7f43bc246a5e4f6d94696&stat=wall-time&nonRelevant=true).

@blyxyas
Copy link
Member Author

blyxyas commented May 15, 2024

I've changed the system, we're back to using visitors (I've benchmarked this new commit, it should have 0 regressions and about -0.66% improvement)

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 15, 2024
@bors
Copy link
Contributor

bors commented May 15, 2024

⌛ Trying commit 828cd60 with merge 68a9e31...

@blyxyas blyxyas force-pushed the ignore-allowed-lints-final branch from 70e9bc2 to ddad55f Compare October 19, 2024 14:48
@blyxyas blyxyas force-pushed the ignore-allowed-lints-final branch from 64c914b to 1dcfa27 Compare October 21, 2024 17:27
@Dylan-DPC Dylan-DPC 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 Oct 25, 2024
@cjgillot
Copy link
Contributor

Do you intend to investigate https://github.com/rust-lang/rust/pull/125116/files#r1770598991 as a follow-up?

Thanks for the work anyway!
@bors r+

@bors
Copy link
Contributor

bors commented Oct 26, 2024

📌 Commit 1dcfa27 has been approved by cjgillot

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 Oct 26, 2024
@bors
Copy link
Contributor

bors commented Oct 26, 2024

⌛ Testing commit 1dcfa27 with merge 4d88de2...

@bors
Copy link
Contributor

bors commented Oct 26, 2024

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing 4d88de2 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 26, 2024
@bors bors merged commit 4d88de2 into rust-lang:master Oct 26, 2024
7 checks passed
@rustbot rustbot added this to the 1.84.0 milestone Oct 26, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (4d88de2): 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 (secondary -4.7%)

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)
-4.7% [-4.7%, -4.7%] 1
All ❌✅ (primary) - - 0

Cycles

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

Binary size

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

Bootstrap: 781.782s -> 783.187s (0.18%)
Artifact size: 333.71 MiB -> 333.73 MiB (0.01%)

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 5, 2024
Do not filter empty lint passes & re-do CTFE pass

Some structs implement `LintPass` without having a `Lint` associated with them rust-lang#125116 broke that behaviour by filtering them out. This PR ensures that lintless passes are not filtered out.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Nov 6, 2024
Rollup merge of rust-lang#132637 - blyxyas:lint-less-passes, r=flip1995

Do not filter empty lint passes & re-do CTFE pass

Some structs implement `LintPass` without having a `Lint` associated with them rust-lang#125116 broke that behaviour by filtering them out. This PR ensures that lintless passes are not filtered out.
flip1995 pushed a commit to flip1995/rust that referenced this pull request Nov 7, 2024
…r=cjgillot

(Big performance change) Do not run lints that cannot emit

Before this change, adding a lint was a difficult matter because it always had some overhead involved. This was because all lints would run, no matter their default level, or if the user had `#![allow]`ed them. This PR changes that. This change would improve both the Rust lint infrastructure and Clippy, but Clippy will see the most benefit, as it has about 900 registered lints (and growing!)

So yeah, with this little patch we filter all lints pre-linting, and remove any lint that is either:
- Manually `#![allow]`ed in the whole crate,
- Allowed in the command line, or
- Not manually enabled with `#[warn]` or similar, and its default level is `Allow`

As some lints **need** to run, this PR also adds **loadbearing lints**. On a lint declaration, you can use the ``@eval_always` = true` marker to label it as loadbearing. A loadbearing lint will never be filtered (it will always run)

Fixes rust-lang#106983
flip1995 pushed a commit to flip1995/rust that referenced this pull request Nov 7, 2024
Do not filter empty lint passes & re-do CTFE pass

Some structs implement `LintPass` without having a `Lint` associated with them rust-lang#125116 broke that behaviour by filtering them out. This PR ensures that lintless passes are not filtered out.
Comment on lines +118 to +119
fn lints_that_dont_need_to_run(tcx: TyCtxt<'_>, (): ()) -> FxIndexSet<LintId> {
let store = unerased_lint_store(&tcx.sess);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Future-compatibility lints that show up in cargo's reports are supposed to be emitted even if they are allowed in the crate. Isn't that something that this logic needs to account for somehow? Or do we account for this already in a different way?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opened #133108 to fix this.

jhpratt added a commit to jhpratt/rust that referenced this pull request Nov 20, 2024
…n, r=lcnr

lints_that_dont_need_to_run: never skip future-compat-reported lints

Follow-up to rust-lang#125116: future-compat lints show up with `--json=future-incompat` even if they are otherwise allowed in the crate. So let's ensure we do not skip those as part of the `lints_that_dont_need_to_run` logic.

I could not find a current future compat lint that is emitted by a lint pass, so there's no clear way to add a test for this.

Cc `@blyxyas` `@cjgillot`
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Nov 20, 2024
Rollup merge of rust-lang#133108 - RalfJung:future-compat-needs-to-run, r=lcnr

lints_that_dont_need_to_run: never skip future-compat-reported lints

Follow-up to rust-lang#125116: future-compat lints show up with `--json=future-incompat` even if they are otherwise allowed in the crate. So let's ensure we do not skip those as part of the `lints_that_dont_need_to_run` logic.

I could not find a current future compat lint that is emitted by a lint pass, so there's no clear way to add a test for this.

Cc `@blyxyas` `@cjgillot`
RalfJung pushed a commit to RalfJung/miri that referenced this pull request Nov 21, 2024
lints_that_dont_need_to_run: never skip future-compat-reported lints

Follow-up to rust-lang/rust#125116: future-compat lints show up with `--json=future-incompat` even if they are otherwise allowed in the crate. So let's ensure we do not skip those as part of the `lints_that_dont_need_to_run` logic.

I could not find a current future compat lint that is emitted by a lint pass, so there's no clear way to add a test for this.

Cc `@blyxyas` `@cjgillot`
@bjorn3 bjorn3 mentioned this pull request Dec 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Don't run late lints that are disabled in the entire crate