Handle `rustc_hir_analysis` cases of `potential_query_instability` lint #131140

ismailarilik · 2024-10-02T05:31:24Z

This PR removes #![allow(rustc::potential_query_instability)] line from compiler/rustc_hir_analysis/src/lib.rs and converts FxHash{Map,Set} types into FxIndex{Map,Set} to suppress lint errors.

A somewhat tracking issue: #84447

…ity` lint

rustbot · 2024-10-02T05:31:31Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @jieyouxu (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

rustbot · 2024-10-02T06:42:05Z

HIR ty lowering was modified

cc @fmease

jieyouxu · 2024-10-02T09:46:04Z

@bors try @rust-timer queue

bors · 2024-10-02T09:47:15Z

⌛ Trying commit 807e812 with merge 459d927...

…instability-lint-for-rustc-hir-analysis, r=<try> Handle `rustc_hir_analysis` cases of `potential_query_instability` lint This PR removes `#![allow(rustc::potential_query_instability)]` line from [`compiler/rustc_hir_analysis/src/lib.rs`](https://github.com/rust-lang/rust/blob/master/compiler/rustc_hir_analysis/src/lib.rs#L61) and converts `FxHash{Map,Set}` types into `FxIndex{Map,Set}` to suppress lint errors. A somewhat tracking issue: rust-lang#84447

bors · 2024-10-02T11:30:14Z

☀️ Try build successful - checks-actions
Build commit: 459d927 (459d927b7f6c2513923e975f6b861e1a9a478cd0)

compiler-errors

These look good, and I doubt they will cause perf issues. I think I may have introduced those GAT usages of hashmaps lol.

compiler-errors · 2024-10-02T14:56:13Z

r=me (in rust parlance, i will approve) after the perf run comes back.

rust-timer · 2024-10-02T15:01:01Z

Finished benchmarking commit (459d927): 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 a highly reliable metric that was 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)	-0.1%	[-0.1%, -0.1%]	3
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

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

Cycles

Results (secondary 2.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
Regressions ❌ (secondary)	8.2%	[8.2%, 8.2%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.4%	[-2.4%, -2.4%]	1
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 772.38s -> 770.477s (-0.25%)
Artifact size: 341.55 MiB -> 341.50 MiB (-0.01%)

compiler-errors · 2024-10-02T15:04:31Z

👍
@bors r+ rollup

bors · 2024-10-02T15:04:35Z

📌 Commit 807e812 has been approved by compiler-errors

It is now in the queue for this repository.

…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#130863 (Relax a debug assertion for dyn principal *equality* in codegen) - rust-lang#131016 (Apple: Do not specify an SDK version in `rlib` object files) - rust-lang#131140 (Handle `rustc_hir_analysis` cases of `potential_query_instability` lint) - rust-lang#131141 (mpmc doctest: make sure main thread waits for child threads) - rust-lang#131150 (only query `params_in_repr` if def kind is adt) - rust-lang#131151 (Replace zero-width whitespace with a visible `\` in the PR template) - rust-lang#131152 (Improve const traits diagnostics for new desugaring) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#131140 - ismailarilik:handle-potential-query-instability-lint-for-rustc-hir-analysis, r=compiler-errors Handle `rustc_hir_analysis` cases of `potential_query_instability` lint This PR removes `#![allow(rustc::potential_query_instability)]` line from [`compiler/rustc_hir_analysis/src/lib.rs`](https://github.com/rust-lang/rust/blob/master/compiler/rustc_hir_analysis/src/lib.rs#L61) and converts `FxHash{Map,Set}` types into `FxIndex{Map,Set}` to suppress lint errors. A somewhat tracking issue: rust-lang#84447

This is an attempt to gain the performance loss after the PR rust-lang#131140. Here the related objects are `IndexSet` so do not require a sort to stay stable.

ismailarilik · 2024-10-06T13:02:58Z

@compiler-errors I know somehow performance is improved in this PR but I think we can improve it further by removing unnecessary sorts. They were exist to make the related sets stable but it isn't necessary anymore. And also we can make them immutable so more secure. The PR for this: #131328

…rts-in-rustc-hir-analysis, r=compiler-errors Remove unnecessary sorts in `rustc_hir_analysis` A follow-up after rust-lang#131140. Here the related objects are `IndexSet` so do not require a sort to stay stable. And they don't need to be `mut` anymore. r? `@compiler-errors`

…rts-in-rustc-hir-analysis, r=compiler-errors Remove unnecessary sorts in `rustc_hir_analysis` A follow-up after rust-lang#131140. Here the related objects are `IndexSet` so do not require a sort to stay stable. And they don't need to be `mut` anymore. r? ``@compiler-errors``

…rts-in-rustc-hir-analysis, r=compiler-errors Remove unnecessary sorts in `rustc_hir_analysis` A follow-up after rust-lang#131140. Here the related objects are `IndexSet` so do not require a sort to stay stable. And they don't need to be `mut` anymore. r? ```@compiler-errors```

Rollup merge of rust-lang#131328 - ismailarilik:remove-unnecessary-sorts-in-rustc-hir-analysis, r=compiler-errors Remove unnecessary sorts in `rustc_hir_analysis` A follow-up after rust-lang#131140. Here the related objects are `IndexSet` so do not require a sort to stay stable. And they don't need to be `mut` anymore. r? ```@compiler-errors```

Handle rustc-hir-analysis cases of `rustc::potential_query_instabil…

807e812

…ity` lint

rustbot assigned jieyouxu Oct 2, 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 Oct 2, 2024

ismailarilik mentioned this pull request Oct 2, 2024

Audit and handle remaining cases of rustc::potential_query_instability lint (iterating HashMaps) #84447

Open

ismailarilik changed the title ~~Handle rustc-hir-analysis cases of potential_query_instability lint~~ Handle rustc_hir_analysis cases of potential_query_instability lint Oct 2, 2024

ismailarilik marked this pull request as ready for review October 2, 2024 06:42

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 Oct 2, 2024

This comment has been minimized.

Sign in to view

compiler-errors approved these changes Oct 2, 2024

View reviewed changes

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

jieyouxu assigned compiler-errors and unassigned jieyouxu Oct 2, 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-review Status: Awaiting review from the assignee but also interested parties. labels Oct 2, 2024

matthiaskrgr mentioned this pull request Oct 2, 2024

Rollup of 7 pull requests #131158

Merged

bors merged commit 7e0797c into rust-lang:master Oct 2, 2024
7 checks passed

rustbot added this to the 1.83.0 milestone Oct 2, 2024

ismailarilik deleted the handle-potential-query-instability-lint-for-rustc-hir-analysis branch October 3, 2024 05:48

ismailarilik mentioned this pull request Oct 6, 2024

Remove unnecessary sorts in rustc_hir_analysis #131328

Merged

Handle rustc_hir_analysis cases of potential_query_instability lint #131140

Handle rustc_hir_analysis cases of potential_query_instability lint #131140

Uh oh!

Conversation

ismailarilik commented Oct 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Oct 2, 2024

Uh oh!

rustbot commented Oct 2, 2024

Uh oh!

jieyouxu commented Oct 2, 2024

Uh oh!

This comment has been minimized.

bors commented Oct 2, 2024

Uh oh!

bors commented Oct 2, 2024

Uh oh!

This comment has been minimized.

compiler-errors left a comment

Choose a reason for hiding this comment

Uh oh!

compiler-errors commented Oct 2, 2024

Uh oh!

rust-timer commented Oct 2, 2024

Overall result: ✅ improvements - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

Uh oh!

compiler-errors commented Oct 2, 2024

Uh oh!

bors commented Oct 2, 2024

Uh oh!

Uh oh!

ismailarilik commented Oct 6, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Handle `rustc_hir_analysis` cases of `potential_query_instability` lint #131140

Handle `rustc_hir_analysis` cases of `potential_query_instability` lint #131140

ismailarilik commented Oct 2, 2024 •

edited

Loading