Skip `if-let-rescope` lint unless requested by migration by dingxiangfei2009 · Pull Request #132666 · rust-lang/rust

dingxiangfei2009 · 2024-11-05T23:27:31Z

Tracked by #124085
Related to #131984 (comment)

Given that if-let-rescope is a lint to be enabled globally by an edition migration, there is no point in extracting the precise lint level on the HIR expression. This mitigates the performance regression discovered by the earlier perf-run.

cc @Kobzol @rylev @traviscross I propose a rust-timer run to measure how much performance that we can recover from the mitigation. 🙇

rustbot · 2024-11-05T23:27:40Z

r? @petrochenkov

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

dingxiangfei2009 · 2024-11-05T23:28:32Z

r? @Kobzol

lqd · 2024-11-06T00:26:31Z

@bors try @rust-timer queue

…-lint, r=<try> Skip `if-let-rescope` lint unless requested by migration Tracked by rust-lang#124085 Related to rust-lang#131984 (comment) Given that `if-let-rescope` is a lint to be enabled globally by an edition migration, there is no point in extracting the precise lint level on the HIR expression. This mitigates the performance regression discovered by the earlier perf-run. cc `@Kobzol` `@rylev` `@traviscross` I propose a `rust-timer` run to measure how much performance that we can recover from the mitigation. 🙇

bors · 2024-11-06T00:27:42Z

⌛ Trying commit 193fe5a with merge 748da1d...

bors · 2024-11-06T02:22:09Z

☀️ Try build successful - checks-actions
Build commit: 748da1d (748da1d278d92651895457bc7ba20c125a24fa1f)

rust-timer · 2024-11-06T04:30:00Z

Finished benchmarking commit (748da1d): comparison URL.

Overall result: ✅ improvements - no 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.

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

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.7%	[-1.9%, -0.2%]	97
Improvements ✅ (secondary)	-1.4%	[-4.0%, -0.1%]	67
All ❌✅ (primary)	-0.7%	[-1.9%, -0.2%]	97

Max RSS (memory usage)

Results (primary 1.2%, secondary -0.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)	1.2%	[1.1%, 1.5%]	3
Regressions ❌ (secondary)	0.6%	[0.5%, 1.0%]	4
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-4.2%	[-4.2%, -4.2%]	1
All ❌✅ (primary)	1.2%	[1.1%, 1.5%]	3

Cycles

Results (primary -0.9%, secondary -2.2%)

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.9%	[-0.9%, -0.9%]	1
Improvements ✅ (secondary)	-2.2%	[-2.4%, -2.1%]	3
All ❌✅ (primary)	-0.9%	[-0.9%, -0.9%]	1

Binary size

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

Bootstrap: 779.813s -> 778.613s (-0.15%)
Artifact size: 335.27 MiB -> 335.46 MiB (0.06%)

dingxiangfei2009 · 2024-11-06T09:14:32Z

Looks like a GREAT SUCCESS to me.

Kobzol · 2024-11-06T09:43:59Z

Looks great indeed :) I assume that there are tests that check that the lint does in fact fire during the edition migration?

dingxiangfei2009 · 2024-11-06T12:54:36Z

There is a test called lint-if-let-rescope-gated.rs but to be honest it is still a test based on edition flag. I am still figuring out the best way to tell with certainty that a migration is run. So far I think there is no such thing yet.

A question to the t-compiler is, do we need a flag for this purpose? To be honest, only edition migration lints would be the potential users. Potentially lints for the match ergonomics and the async closures can benefit from the flag as well, because they are technically not executed outside the migration scenario.

dingxiangfei2009 · 2024-11-20T11:46:32Z

r? @compiler-errors

It looks like we need a look from someone on t-compiler to see this makes sense. Shall we?

compiler-errors

I'm confused. Why do we need to manually call this here? Shouldn't lints automatically be filtering using lints_that_dont_need_to_run? Isn't that the whole point of #125116?

I think we just need to remove the lint_level_at_node call, right?

compiler-errors · 2024-11-21T04:17:06Z

-        }
-        if let (Level::Allow, _) = cx.tcx.lint_level_at_node(IF_LET_RESCOPE, expr.hir_id) {
+        if expr.span.edition().at_least_rust_2024()
+            || cx.tcx.lints_that_dont_need_to_run(()).contains(&LintId::of(IF_LET_RESCOPE))


Do we even need this call to cx.tcx.lints_that_dont_need_to_run(())? Aren't lints already automatically being filtered by the late lint infrastructure?

Apparently it is not, at least when I instrumented the fn check_expr the log suggests that this lint was still called, which explains why the instruction count has increased significantly. I am looking for time to sit down and investigate why #125116 did not stop this lint from running.

alex-semenyuk · 2024-12-28T09:49:09Z

@dingxiangfei2009
From wg-triage. Any updates on this PR

compiler-errors · 2025-01-23T19:58:33Z

I looked into this. Yeah, we don't actually filter built-in lints. I think this is fine.

@bors r+ rollup=never

bors · 2025-01-23T19:58:36Z

📌 Commit 193fe5a has been approved by compiler-errors

It is now in the queue for this repository.

matthiaskrgr · 2025-01-23T22:05:21Z

@bors p=5 (scheduling)

bors · 2025-01-23T23:16:10Z

⌛ Testing commit 193fe5a with merge 22a220a...

bors · 2025-01-24T01:58:26Z

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing 22a220a to master...

rust-timer · 2025-01-24T04:31:39Z

Finished benchmarking commit (22a220a): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.8%	[-1.9%, -0.2%]	100
Improvements ✅ (secondary)	-1.5%	[-4.2%, -0.1%]	69
All ❌✅ (primary)	-0.8%	[-1.9%, -0.2%]	100

Max RSS (memory usage)

Results (primary -0.7%, secondary 3.9%)

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.9%	[0.9%, 0.9%]	1
Regressions ❌ (secondary)	3.9%	[3.9%, 3.9%]	1
Improvements ✅ (primary)	-2.2%	[-2.2%, -2.2%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.7%	[-2.2%, 0.9%]	2

Cycles

Results (primary -1.1%, secondary -2.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)	-1.1%	[-1.3%, -0.7%]	4
Improvements ✅ (secondary)	-2.7%	[-3.3%, -2.2%]	2
All ❌✅ (primary)	-1.1%	[-1.3%, -0.7%]	4

Binary size

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

Bootstrap: 776.719s -> 774.487s (-0.29%)
Artifact size: 325.95 MiB -> 326.07 MiB (0.04%)

nnethercote · 2025-01-27T21:59:14Z

Are there any other lints like this one that could also be skipped? :)

Kobzol · 2025-01-27T22:03:08Z

This PR mostly just fixed a situation where a very specific Edition 2024 lint was executed everytime, even though it was not needed unless in migration mode (AFAIK).

#125116 tried in general to not run lints that don't have to run (e.g. when they are allowed), but situations like these probably can't be detected by that.

compiler-errors · 2025-01-27T22:08:46Z

Note that #125116 does not filter out builtin lints which are lumped into the BuiltinCombinedLateLintPass or whatever it's called.

#134862 attempts to do some filtering in that pass, but it is probably not worth it -- the perf wins I think are just the wins from not running this pass. I'm re-running perf right now tho.

skip if-let-rescope lint unless requested by migration

193fe5a

dingxiangfei2009 marked this pull request as ready for review November 5, 2024 23:27

rustbot assigned petrochenkov Nov 5, 2024

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. labels Nov 5, 2024

dingxiangfei2009 changed the title ~~Skip if-let-rescope lint unless requested by migration~~ Skip if-let-rescope lint unless requested by migration Nov 5, 2024

rustbot assigned Kobzol and unassigned petrochenkov Nov 5, 2024

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Nov 6, 2024

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Nov 6, 2024

rustbot assigned compiler-errors and unassigned Kobzol Nov 20, 2024

compiler-errors suggested changes Nov 21, 2024

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 21, 2024

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 Jan 23, 2025

bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 24, 2025

bors merged commit 22a220a into rust-lang:master Jan 24, 2025

rustbot added this to the 1.86.0 milestone Jan 24, 2025

Uh oh!

Conversation

dingxiangfei2009 commented Nov 5, 2024

Uh oh!

rustbot commented Nov 5, 2024

Uh oh!

dingxiangfei2009 commented Nov 5, 2024

Uh oh!

lqd commented Nov 6, 2024

Uh oh!

This comment has been minimized.

bors commented Nov 6, 2024

Uh oh!

bors commented Nov 6, 2024

Uh oh!

This comment has been minimized.

rust-timer commented Nov 6, 2024

Overall result: ✅ improvements - no action needed

Uh oh!

dingxiangfei2009 commented Nov 6, 2024

Uh oh!

Kobzol commented Nov 6, 2024

Uh oh!

dingxiangfei2009 commented Nov 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dingxiangfei2009 commented Nov 20, 2024

Uh oh!

compiler-errors left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alex-semenyuk commented Dec 28, 2024

Uh oh!

compiler-errors commented Jan 23, 2025

Uh oh!

bors commented Jan 23, 2025

Uh oh!

matthiaskrgr commented Jan 23, 2025

Uh oh!

bors commented Jan 23, 2025

Uh oh!

bors commented Jan 24, 2025

Uh oh!

rust-timer commented Jan 24, 2025

Overall result: ✅ improvements - no action needed

Uh oh!

nnethercote commented Jan 27, 2025

Uh oh!

Kobzol commented Jan 27, 2025

Uh oh!

compiler-errors commented Jan 27, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

dingxiangfei2009 commented Nov 6, 2024 •

edited

Loading