Compute lint levels by definition #101620

cjgillot · 2022-09-09T17:26:55Z

Lint levels are currently computed once for the whole crate. Any code that wants to emit a lint depends on this single lint_levels(()) query. This query contains the Span for each attribute that participates in the lint level tree, so any code that wants to emit a lint basically depends on the spans in all files in the crate.

Contrary to hard errors, we do not clear the incremental session on lints, so this implicit world dependency pessimizes incremental reuse. (And is furthermore invisible for allowed lints.)

This PR completes #99634 (thanks for the initial work @fee1-dead) and includes it in the dependency graph.

The design is based on 2 queries:

lint_levels_on(HirId) -> FxHashMap<LintId, LevelAndSource> which accesses the attributes at the given HirId and processes them into lint levels. The TyCtxt is responsible for probing the HIR tree to find the user-visible level.
lint_expectations(()) which lists all the #[expect] attributes in the crate.

This PR also introduces the ability to reconstruct a HirId from a DepNode by encoding the local part of the DefPathHash and the ItemLocalId in the two u64 of the fingerprint. This allows for the dep-graph to directly recompute lint_levels_on directly, without having to force the calling query.

Closes #95094.
Supersedes #99634.

cjgillot · 2022-09-09T18:33:28Z

@bors try @rust-timer queue

rust-timer · 2022-09-09T18:33:30Z

Awaiting bors try build completion.

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

bors · 2022-09-09T18:33:37Z

⌛ Trying commit 1c1ad06ffc5d637beb672ef9385cfdd3bbe8554d with merge 4ae314edea36bbec3e6a73de890c1d6c727690ac...

bors · 2022-09-09T20:07:18Z

☀️ Try build successful - checks-actions
Build commit: 4ae314edea36bbec3e6a73de890c1d6c727690ac (4ae314edea36bbec3e6a73de890c1d6c727690ac)

rust-timer · 2022-09-09T20:07:20Z

Queued 4ae314edea36bbec3e6a73de890c1d6c727690ac with parent 98f3001, future comparison URL.

rust-timer · 2022-09-09T21:24:41Z

Finished benchmarking commit (4ae314edea36bbec3e6a73de890c1d6c727690ac): comparison URL.

Overall result: ❌✅ regressions and improvements - 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-review -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)	1.0%	[0.2%, 2.6%]	24
Regressions ❌ (secondary)	1.6%	[0.2%, 4.0%]	35
Improvements ✅ (primary)	-4.4%	[-28.2%, -0.2%]	57
Improvements ✅ (secondary)	-0.9%	[-2.0%, -0.2%]	42
All ❌✅ (primary)	-2.8%	[-28.2%, 2.6%]	81

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)	1.1%	[0.7%, 2.0%]	6
Regressions ❌ (secondary)	0.7%	[0.7%, 0.7%]	1
Improvements ✅ (primary)	-4.0%	[-10.8%, -0.9%]	27
Improvements ✅ (secondary)	-2.4%	[-2.4%, -2.4%]	2
All ❌✅ (primary)	-3.1%	[-10.8%, 2.0%]	33

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)	3.9%	[2.0%, 7.7%]	5
Improvements ✅ (primary)	-12.2%	[-34.2%, -2.2%]	25
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-12.2%	[-34.2%, -2.2%]	25

the arithmetic mean of the percent change ↩ ↩² ↩³
number of relevant changes ↩ ↩² ↩³

jyn514 · 2022-09-09T23:53:54Z

👀

I'm not super familiar with the code, but I can take a shot at reviewing it if you like

jyn514

This is really cool :) I left a bunch of comments, but not sure whether they're actually bugs or I just don't understand the code.

compiler/rustc_lint/src/expect.rs

compiler/rustc_lint/src/levels.rs

compiler/rustc_lint_defs/src/lib.rs

jyn514 · 2022-09-10T00:36:38Z

compiler/rustc_lint/src/levels.rs

+                    let LintExpectationId::Unstable { attr_id, lint_index } = unstable_id
+                        else { bug!("stable id Level::from_attr") };


Do we need to panic here? I think it should be fine to pass in a stable id, right, we just return Level::Expect(id) as is?

compiler/rustc_middle/src/lint.rs

jyn514 · 2022-09-10T01:00:09Z

compiler/rustc_query_impl/src/keys.rs

+    #[inline(always)]
+    fn key_as_def_id(&self) -> Option<DefId> {
+        Some(self.owner.to_def_id())


are you sure this is correct? shouldn't we do tcx.hir().local_def_id(self) instead? ditto for default_span

jyn514 · 2022-09-10T01:05:12Z

compiler/rustc_lint/src/levels.rs

+    let is_crate = hir::CRATE_HIR_ID == hir_id;
+    if is_crate {
+        levels.add_command_line();


Why are we only adding this for top-level crate? Don't the CLI levels also affect all the other items in the crate?

Because the LintLevelsBuilder calls lint_levels_on for all parents.

fee1-dead · 2022-09-10T08:15:33Z

r? rust-lang/compiler

cjgillot · 2022-09-10T09:55:54Z

Besides the great improvement on the incr-patched cases, there is a ~2 % regression on the incr-unchanged case. I suspect this is due to the query overhead. One possible solution would be to change the granularity of the shallow_lint_levels_on query to be per HIR-owner, instead of per HirId. I'd like to leave this as a follow-up.

bors · 2022-09-13T02:07:16Z

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

oli-obk · 2022-09-13T08:22:03Z

r=me after a rebase

cjgillot · 2022-09-13T18:03:53Z

@bors r=oli-obk

bors · 2022-09-13T18:03:55Z

📌 Commit 68ec7901db2efe51948546c2d73ad32dcb06d121 has been approved by oli-obk

It is now in the queue for this repository.

rust-timer · 2022-09-15T04:48:15Z

Finished benchmarking commit (2cb9a65): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

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)	4.1%	[0.4%, 14.5%]	133
Regressions ❌ (secondary)	5.2%	[0.2%, 64.1%]	131
Improvements ✅ (primary)	-5.1%	[-24.3%, -0.4%]	30
Improvements ✅ (secondary)	-1.0%	[-1.2%, -0.5%]	5
All ❌✅ (primary)	2.4%	[-24.3%, 14.5%]	163

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)	4.5%	[1.0%, 14.8%]	102
Regressions ❌ (secondary)	9.1%	[1.4%, 28.7%]	64
Improvements ✅ (primary)	-3.8%	[-8.7%, -1.2%]	12
Improvements ✅ (secondary)	-3.6%	[-4.5%, -2.8%]	2
All ❌✅ (primary)	3.6%	[-8.7%, 14.8%]	114

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)	5.1%	[1.7%, 17.1%]	93
Regressions ❌ (secondary)	10.1%	[2.0%, 44.9%]	72
Improvements ✅ (primary)	-12.6%	[-31.5%, -2.6%]	17
Improvements ✅ (secondary)	-2.4%	[-2.4%, -2.4%]	1
All ❌✅ (primary)	2.3%	[-31.5%, 17.1%]	110

the arithmetic mean of the percent change ↩ ↩² ↩³
number of relevant changes ↩ ↩² ↩³

cjgillot · 2022-09-15T06:12:02Z

Something went wrong between the previous perf measurement and this one. Filed #101839 to revert.

Do not fetch HIR node when iterating to find lint. Addresses the regression in rust-lang#101620

…_by_def, r=oli-obk" This reverts commit 2cb9a65, reversing changes made to 750bd1a.

Revert perf-regression 101620 Reverts rust-lang#101862 rust-lang#101620 r? `@Mark-Simulacrum`

… r=oli-obk Compute lint levels by definition Second attempt to rust-lang#101620. I think that I have removed the perf regression.

…_by_def, r=oli-obk" This reverts commit 2cb9a65, reversing changes made to 750bd1a. (cherry picked from commit fc43df0)

Revert perf-regression 101620 Reverts rust-lang#101862 rust-lang#101620 r? `@Mark-Simulacrum`

rustbot added A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 9, 2022

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Sep 9, 2022

rustbot added perf-regression Performance regression. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Sep 9, 2022

cjgillot changed the title ~~Compute lint levels by def~~ Compute lint levels by definition Sep 9, 2022

cjgillot marked this pull request as ready for review September 9, 2022 23:33

This comment has been minimized.

Sign in to view

jyn514 reviewed Sep 10, 2022

View reviewed changes

fee1-dead mentioned this pull request Sep 10, 2022

Compute lint_levels by definition #99634

Closed

rust-highfive assigned oli-obk Sep 10, 2022

jyn514 approved these changes Sep 10, 2022

View reviewed changes

oli-obk mentioned this pull request Sep 13, 2022

Can we add a new StableLintExpectationId type so we can remove unstable Hash impls #101751

Closed

cjgillot force-pushed the compute_lint_levels_by_def branch from 128646a to 68ec790 Compare September 13, 2022 17:27

bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 13, 2022

cjgillot mentioned this pull request Sep 15, 2022

Revert "Compute lint levels by definition" #101839

Closed

cjgillot mentioned this pull request Sep 15, 2022

Do not fetch HIR node when iterating to find lint. #101862

Merged

bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 18, 2022

Auto merge of rust-lang#101862 - cjgillot:lint-regression, r=oli-obk

bc7b17c

Do not fetch HIR node when iterating to find lint. Addresses the regression in rust-lang#101620

cjgillot mentioned this pull request Sep 20, 2022

Revert perf-regression 101620 #102064

Merged

bors pushed a commit to rust-lang-ci/rust that referenced this pull request Sep 23, 2022

Revert "Auto merge of rust-lang#101620 - cjgillot:compute_lint_levels…

fc43df0

…_by_def, r=oli-obk" This reverts commit 2cb9a65, reversing changes made to 750bd1a.

bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 24, 2022

Auto merge of rust-lang#102064 - cjgillot:revert, r=Mark-Simulacrum

bb5a016

Revert perf-regression 101620 Reverts rust-lang#101862 rust-lang#101620 r? `@Mark-Simulacrum`

cjgillot deleted the compute_lint_levels_by_def branch September 24, 2022 12:47

cjgillot mentioned this pull request Sep 24, 2022

Compute lint levels by definition #102236

Merged

oli-obk pushed a commit to oli-obk/rust that referenced this pull request Sep 28, 2022

Auto merge of rust-lang#102064 - cjgillot:revert, r=Mark-Simulacrum

4889c17

Revert perf-regression 101620 Reverts rust-lang#101862 rust-lang#101620 r? `@Mark-Simulacrum`

Aaron1011 pushed a commit to Aaron1011/rust that referenced this pull request Jan 6, 2023

Auto merge of rust-lang#102064 - cjgillot:revert, r=Mark-Simulacrum

93e726b

Revert perf-regression 101620 Reverts rust-lang#101862 rust-lang#101620 r? `@Mark-Simulacrum`

clubby789 mentioned this pull request May 17, 2023

unused_attributes false positive (top-level unused_crate_dependencies with feature(lint_reasons)) #111682

Closed

michaelwoerister mentioned this pull request Jun 3, 2024

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

Open

xFrednet mentioned this pull request Jul 2, 2024

Let's #[expect] some lints: Stabilize lint_reasons (RFC 2383) #120924

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compute lint levels by definition #101620

Compute lint levels by definition #101620

cjgillot commented Sep 9, 2022 •

edited

Loading

cjgillot commented Sep 9, 2022

rust-timer commented Sep 9, 2022

bors commented Sep 9, 2022

bors commented Sep 9, 2022

rust-timer commented Sep 9, 2022

rust-timer commented Sep 9, 2022

jyn514 commented Sep 9, 2022

This comment has been minimized.

jyn514 left a comment

jyn514 Sep 10, 2022

jyn514 Sep 10, 2022

jyn514 Sep 10, 2022

fee1-dead Sep 10, 2022

fee1-dead commented Sep 10, 2022

cjgillot commented Sep 10, 2022

bors commented Sep 13, 2022

oli-obk commented Sep 13, 2022

cjgillot commented Sep 13, 2022

bors commented Sep 13, 2022

rust-timer commented Sep 15, 2022

cjgillot commented Sep 15, 2022

		let LintExpectationId::Unstable { attr_id, lint_index } = unstable_id
		else { bug!("stable id Level::from_attr") };

Compute lint levels by definition #101620

Compute lint levels by definition #101620

Conversation

cjgillot commented Sep 9, 2022 • edited Loading

cjgillot commented Sep 9, 2022

rust-timer commented Sep 9, 2022

bors commented Sep 9, 2022

bors commented Sep 9, 2022

rust-timer commented Sep 9, 2022

rust-timer commented Sep 9, 2022

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Footnotes

jyn514 commented Sep 9, 2022

This comment has been minimized.

jyn514 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fee1-dead commented Sep 10, 2022

cjgillot commented Sep 10, 2022

bors commented Sep 13, 2022

oli-obk commented Sep 13, 2022

cjgillot commented Sep 13, 2022

bors commented Sep 13, 2022

rust-timer commented Sep 15, 2022

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Footnotes

cjgillot commented Sep 15, 2022

cjgillot commented Sep 9, 2022 •

edited

Loading