Skip to content

Don't hash DelayedLints#155248

Closed
JonathanBrouwer wants to merge 1 commit intorust-lang:mainfrom
JonathanBrouwer:no_hash_delayed_lints
Closed

Don't hash DelayedLints#155248
JonathanBrouwer wants to merge 1 commit intorust-lang:mainfrom
JonathanBrouwer:no_hash_delayed_lints

Conversation

@JonathanBrouwer
Copy link
Copy Markdown
Contributor

@JonathanBrouwer JonathanBrouwer commented Apr 13, 2026 •
edited
Loading

This PR unblocks #154432, and was also a minor perf win locally

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 13, 2026
@JonathanBrouwer
Copy link
Copy Markdown
Contributor Author

JonathanBrouwer commented Apr 13, 2026

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Sign in to view
@rust-bors

This comment has been minimized.

Sign in to view
rust-bors bot pushed a commit that referenced this pull request Apr 13, 2026
@bors
Auto merge of #155248 - JonathanBrouwer:no_hash_delayed_lints, r=<try>
172ee0e 
Don't hash `DelayedLints`
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 13, 2026
@JonathanBrouwer JonathanBrouwer mentioned this pull request Apr 13, 2026
Open
JonathanBrouwer
JonathanBrouwer commented Apr 13, 2026
View reviewed changes
compiler/rustc_middle/src/queries.rs
/// Avoid calling this query directly.
query opt_ast_lowering_delayed_lints(key: hir::OwnerId) -> Option<&'tcx hir::lints::DelayedLints> {
desc { "getting AST lowering delayed lints in `{}`", tcx.def_path_str(key) }
no_hash
Copy link
Copy Markdown
Contributor Author

@JonathanBrouwer JonathanBrouwer Apr 13, 2026 •
edited by rustbot
Loading

Choose a reason for hiding this comment

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

View changes since the review

By marking the query as no_hash it will always be recomputed, but it is very cheap to rerun, in fact according to my local benchmarks rerunning is cheaper than hashing :3

@rust-bors
Copy link
Copy Markdown
Contributor

rust-bors bot commented Apr 13, 2026

☀️ Try build successful (CI)
Build commit: 172ee0e (172ee0e5c78ffeb8f2fc257f0c6f0fa7cf079fef, parent: 14196dbfa3eb7c30195251eac092b1b86c8a2d84)

@rust-timer

This comment has been minimized.

Sign in to view
@JonathanBrouwer JonathanBrouwer marked this pull request as ready for review April 13, 2026 19:43
@JonathanBrouwer
Copy link
Copy Markdown
Contributor Author

JonathanBrouwer commented Apr 13, 2026 •
edited
Loading

r? @GuillaumeGomez
cc @jdonszelmann

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 13, 2026
@rustbot rustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 13, 2026
@rustbot

This comment has been minimized.

Sign in to view
@rust-timer
Copy link
Copy Markdown
Collaborator

rust-timer commented Apr 13, 2026

Finished benchmarking commit (172ee0e): comparison URL.

Overall result: ✅ improvements - no action needed

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.

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

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 -0.2% [-0.2%, -0.2%] 2
Improvements ✅
(secondary)		 -0.3% [-0.4%, -0.0%] 13
All ❌✅ (primary) -0.2% [-0.2%, -0.2%] 2

Max RSS (memory usage)

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

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 1.5% [0.9%, 2.0%] 2
Improvements ✅
(primary)		 -2.2% [-2.2%, -2.2%] 1
Improvements ✅
(secondary)		 -5.3% [-6.4%, -4.1%] 2
All ❌✅ (primary) -2.2% [-2.2%, -2.2%] 1

Cycles

Results (primary -2.5%, secondary 28.6%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 28.6% [28.6%, 28.6%] 1
Improvements ✅
(primary)		 -2.5% [-2.9%, -2.1%] 2
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) -2.5% [-2.9%, -2.1%] 2

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 491.114s -> 494.706s (0.73%)
Artifact size: 394.23 MiB -> 394.22 MiB (-0.00%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 13, 2026
JonathanBrouwer
JonathanBrouwer commented Apr 13, 2026
View reviewed changes
compiler/rustc_hir/src/hir.rs

/// Lints delayed during ast lowering to be emitted
/// after hir has completely built
#[stable_hasher(ignore)]
Copy link
Copy Markdown
Contributor Author

@JonathanBrouwer JonathanBrouwer Apr 13, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

View changes since the review

Actually but that I'm looking at this diff again I'd like to double check that this line is sound, so please wait with merging

Copy link
Copy Markdown
Contributor Author

@JonathanBrouwer JonathanBrouwer Apr 14, 2026

Choose a reason for hiding this comment

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

Looking at it again, I'm actually pretty sure this is not sound :c
The hash of the lints can change without the hash of the HIR itself changing, crazily enough. And if delayed_lints here changes we need to rerun HIR analysis as that is what actually emits the lints, rather than re-using the cached result

@JonathanBrouwer JonathanBrouwer added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 13, 2026
@GuillaumeGomez
Copy link
Copy Markdown
Member

GuillaumeGomez commented Apr 13, 2026

Well, sounds good to me and that unblocks #154432 so yeay. r=me once you're sure everything is ok on your side too.

@JonathanBrouwer JonathanBrouwer marked this pull request as draft April 14, 2026 06:47
@rustbot rustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@GuillaumeGomez GuillaumeGomez GuillaumeGomez approved these changes

Assignees

@GuillaumeGomez GuillaumeGomez

Labels

S-blocked Status: Blocked on something else such as an RFC or other implementation work. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@JonathanBrouwer @rust-timer @rustbot @GuillaumeGomez @mati865