Compute lint levels by definition #102236

cjgillot · 2022-09-24T14:22:11Z

Second attempt to #101620.

I think that I have removed the perf regression.

rust-highfive · 2022-09-24T14:22:14Z

r? @eholk

(rust-highfive has picked a reviewer for you, use r? to override)

cjgillot · 2022-09-24T14:22:18Z

@bors try @rust-timer queue

rust-timer · 2022-09-24T14:22:20Z

Awaiting bors try build completion.

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

bors · 2022-09-24T14:22:26Z

⌛ Trying commit 84c767f38c89a9a6ac7023512744c1da175f0ccc with merge 4c9d2084bc3b3c56b4cd8dae0f12ea3740d2a9c5...

bors · 2022-09-24T15:58:28Z

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

rust-timer · 2022-09-24T15:58:30Z

Queued 4c9d2084bc3b3c56b4cd8dae0f12ea3740d2a9c5 with parent bb5a016, future comparison URL.

rust-timer · 2022-09-24T17:14:31Z

Finished benchmarking commit (4c9d2084bc3b3c56b4cd8dae0f12ea3740d2a9c5): 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.4%	[0.2%, 2.8%]	26
Improvements ✅ (primary)	-4.1%	[-28.4%, -0.2%]	63
Improvements ✅ (secondary)	-1.1%	[-2.3%, -0.3%]	51
All ❌✅ (primary)	-2.7%	[-28.4%, 2.6%]	87

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)	0.7%	[0.6%, 0.9%]	3
Regressions ❌ (secondary)	2.7%	[1.8%, 3.6%]	2
Improvements ✅ (primary)	-4.3%	[-10.9%, -0.9%]	25
Improvements ✅ (secondary)	-4.1%	[-4.1%, -4.1%]	1
All ❌✅ (primary)	-3.8%	[-10.9%, 0.9%]	28

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)	2.7%	[2.3%, 3.0%]	4
Regressions ❌ (secondary)	3.2%	[3.2%, 3.2%]	1
Improvements ✅ (primary)	-12.8%	[-33.4%, -3.0%]	23
Improvements ✅ (secondary)	-2.4%	[-2.8%, -2.1%]	2
All ❌✅ (primary)	-10.5%	[-33.4%, 3.0%]	27

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

bors · 2022-09-25T01:48:25Z

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

bors · 2022-09-26T01:02:59Z

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

eholk · 2022-09-27T23:36:03Z

I started to take a brief look, but I'm not familiar with this code so it'd be better for someone else to review it. Maybe Oli, since he signed off on the last version?

@bors r? @oli-obk

oli-obk · 2022-09-29T08:13:14Z

hyper and libc in incremental-unchanged mode still have regressions that look like they are due to query recomputations even though nothing changed. I'm not sure we can do much better... I worry making caching better here may have adverse effects elsewhere, but I'll leave the decision on what to do to you, we can definitely merge this, as it massively improves the "normal" case (something got modified). I'll give the code another review now

oli-obk

impl seems good to me. Modulo a comment or a fix of the perf, r=me

compiler/rustc_lint/src/levels.rs

oli-obk · 2022-09-29T08:35:24Z

compiler/rustc_lint/src/levels.rs

+    }
+}
+
+impl<'tcx> intravisit::Visitor<'tcx> for LintLevelsBuilder<'_, QueryMapExpectationsWrapper<'tcx>> {


it's a bit hard to review the details here since it was moved around. Either move back or lmk if this is just a code move and not actually a functional change

I did what I could to separate the code motion from the functional change there.

cjgillot · 2022-09-29T19:40:29Z

@bors try @rust-timer queue

rust-timer · 2022-09-29T19:40:31Z

Awaiting bors try build completion.

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

cjgillot · 2022-10-01T15:00:17Z

@bors r=oli-obk

bors · 2022-10-01T15:00:19Z

📌 Commit fec53fd has been approved by oli-obk

It is now in the queue for this repository.

bors · 2022-10-01T19:54:58Z

⌛ Testing commit fec53fd with merge 57f097e...

bors · 2022-10-01T22:39:23Z

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 57f097e to master...

rust-timer · 2022-10-02T05:09:48Z

Finished benchmarking commit (57f097e): 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)	0.6%	[0.3%, 0.9%]	14
Regressions ❌ (secondary)	1.0%	[0.2%, 2.0%]	20
Improvements ✅ (primary)	-3.2%	[-30.2%, -0.2%]	84
Improvements ✅ (secondary)	-1.1%	[-2.8%, -0.3%]	52
All ❌✅ (primary)	-2.7%	[-30.2%, 0.9%]	98

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)	0.6%	[0.6%, 0.6%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-4.2%	[-11.1%, -0.9%]	27
Improvements ✅ (secondary)	-2.6%	[-2.9%, -2.2%]	2
All ❌✅ (primary)	-4.0%	[-11.1%, 0.6%]	28

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)	2.8%	[2.0%, 3.6%]	14
Improvements ✅ (primary)	-13.4%	[-35.0%, -3.3%]	22
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-13.4%	[-35.0%, -3.3%]	22

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

pnkfelix · 2022-10-05T02:32:29Z

If I understand the description inherited from #101620, this was an important step for making incremental compilation apply to a much broader set of input code. And the performance gains reported by perf (e.g. Twenty primary incr-patched scenarios are seeing improvements of 4% or more) seem well worth the cost that it registered.

@rustbot label: +perf-regression-triaged

rust-highfive assigned eholk Sep 24, 2022

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 24, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 24, 2022

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

cjgillot force-pushed the compute_lint_levels_by_def branch from 84c767f to c81d834 Compare September 24, 2022 16:34

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Sep 24, 2022

cjgillot force-pushed the compute_lint_levels_by_def branch from c81d834 to 71840cc Compare September 25, 2022 09:45

cjgillot force-pushed the compute_lint_levels_by_def branch from 71840cc to 822b91b Compare September 26, 2022 18:10

rust-highfive assigned oli-obk and unassigned eholk Sep 27, 2022

oli-obk requested changes Sep 29, 2022

View reviewed changes

cjgillot force-pushed the compute_lint_levels_by_def branch 2 times, most recently from 2268c93 to a64bb3f Compare September 29, 2022 19:21

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

cjgillot added 15 commits October 1, 2022 16:12

Remove unused tool_name.

107170b

Move code to rustc_lint.

41db9b1

Reduce visibilities and remove dead code.

6977f7d

Move lint level computation to rustc_middle::lint.

34bc5c8

Comment LintLevelSets.

af495f8

Add FIXME.

273b54d

Correct Key impl for HirId.

6e76599

Do not fetch HIR node when iterating to find lint.

26e5fe9

Compute by owner instead of HirId.

d08669c

Simplify LintLevelsProvider.

e78dd6d

Add fast path without visiting.

c2d5dd2

Replace retain with assertion.

1360871

Use a SortedMap instead of a VecMap.

9f2ab5b

Add FIXME.

299e10d

Add sanity Drop impl.

fec53fd

cjgillot force-pushed the compute_lint_levels_by_def branch from 4753491 to fec53fd Compare October 1, 2022 14:25

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 1, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 1, 2022

bors merged commit 57f097e into rust-lang:master Oct 1, 2022

rustbot added this to the 1.66.0 milestone Oct 1, 2022

bors mentioned this pull request Oct 1, 2022

Perform unused assignment and unused variables lints on MIR. #101500

Open

cjgillot deleted the compute_lint_levels_by_def branch October 2, 2022 08:51

rustbot added the perf-regression-triaged The performance regression has been triaged. label Oct 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compute lint levels by definition #102236

Compute lint levels by definition #102236

cjgillot commented Sep 24, 2022

rust-highfive commented Sep 24, 2022

cjgillot commented Sep 24, 2022

rust-timer commented Sep 24, 2022

bors commented Sep 24, 2022

bors commented Sep 24, 2022

rust-timer commented Sep 24, 2022

rust-timer commented Sep 24, 2022

bors commented Sep 25, 2022

bors commented Sep 26, 2022

eholk commented Sep 27, 2022

oli-obk commented Sep 29, 2022

oli-obk left a comment

oli-obk Sep 29, 2022

cjgillot Oct 1, 2022

cjgillot commented Sep 29, 2022

rust-timer commented Sep 29, 2022

cjgillot commented Oct 1, 2022

bors commented Oct 1, 2022

bors commented Oct 1, 2022

bors commented Oct 1, 2022

rust-timer commented Oct 2, 2022

pnkfelix commented Oct 5, 2022

Compute lint levels by definition #102236

Compute lint levels by definition #102236

Conversation

cjgillot commented Sep 24, 2022

rust-highfive commented Sep 24, 2022

cjgillot commented Sep 24, 2022

rust-timer commented Sep 24, 2022

bors commented Sep 24, 2022

bors commented Sep 24, 2022

rust-timer commented Sep 24, 2022

rust-timer commented Sep 24, 2022

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Footnotes

bors commented Sep 25, 2022

bors commented Sep 26, 2022

eholk commented Sep 27, 2022

oli-obk commented Sep 29, 2022

oli-obk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjgillot commented Sep 29, 2022

rust-timer commented Sep 29, 2022

cjgillot commented Oct 1, 2022

bors commented Oct 1, 2022

bors commented Oct 1, 2022

bors commented Oct 1, 2022

rust-timer commented Oct 2, 2022

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Footnotes

pnkfelix commented Oct 5, 2022