Clean up potential_query_instability with FxIndexMap and UnordMap #120931

chenyukang · 2024-02-11T12:54:40Z

From #120485 (comment)

Kobzol · 2024-02-11T17:42:58Z

bors · 2024-02-11T17:45:03Z

⌛ Trying commit 487167c with merge f51ebab...

…=<try> Clean up potential_query_instability with FxIndexMap and UnordMap From rust-lang#120485 (comment) r? `@michaelwoerister`

bors · 2024-02-11T19:13:25Z

☀️ Try build successful - checks-actions
Build commit: f51ebab (f51ebabcb2747475594aa49e1b24597ff65306b0)

rust-timer · 2024-02-11T20:26:19Z

Finished benchmarking commit (f51ebab): 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.3%	[-0.3%, -0.3%]	1
All ❌✅ (primary)	-	-	0

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
Regressions ❌ (secondary)	3.2%	[2.2%, 5.4%]	4
Improvements ✅ (primary)	-0.9%	[-0.9%, -0.9%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.9%	[-0.9%, -0.9%]	1

Cycles

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

Binary size

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

Bootstrap: 663.621s -> 663.612s (-0.00%)
Artifact size: 308.42 MiB -> 308.31 MiB (-0.03%)

erikdesjardins

Some comment nits

compiler/rustc_borrowck/src/universal_regions.rs

compiler/rustc_codegen_ssa/src/back/link.rs

compiler/rustc_codegen_ssa/src/base.rs

compiler/rustc_middle/src/mir/mono.rs

rustbot · 2024-02-12T02:04:33Z

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

chenyukang · 2024-02-12T02:38:41Z

@bors try @rust-timer queue

bors · 2024-02-12T02:39:52Z

⌛ Trying commit 2d054d0 with merge 268fce2...

…=<try> Clean up potential_query_instability with FxIndexMap and UnordMap From rust-lang#120485 (comment) r? `@michaelwoerister`

michaelwoerister · 2024-02-12T16:14:49Z

Thanks for the PR, @chenyukang! The review is in my work queue.

bors · 2024-02-12T17:55:09Z

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

chenyukang · 2024-02-14T10:37:19Z

Looks good to me, thanks @chenyukang!

I'm happy to approve with the commit history cleaned up a bit.

rebased now.

michaelwoerister · 2024-02-14T11:17:58Z

Thank you!

@bors r+

bors · 2024-02-14T11:18:01Z

📌 Commit 3f27e4b has been approved by michaelwoerister

It is now in the queue for this repository.

…=michaelwoerister Clean up potential_query_instability with FxIndexMap and UnordMap From rust-lang#120485 (comment) r? `@michaelwoerister`

bors · 2024-02-14T23:12:06Z

⌛ Testing commit 3f27e4b with merge d2495e3...

rust-log-analyzer · 2024-02-15T00:02:14Z

The job aarch64-gnu failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

[RUSTC-TIMING] tempfile test:false 0.566
    Checking tracing-log v0.1.4
[RUSTC-TIMING] rand test:false 1.946
    Checking twox-hash v1.6.3
##[error]The runner has received a shutdown signal. This can happen when the runner service is stopped, or a manually started runner is canceled.
    Checking powerfmt v0.2.0
[RUSTC-TIMING] aho_corasick test:false 3.305
[RUSTC-TIMING] unicode_normalization test:false 1.321
    Checking unicode-security v0.1.1
    Checking unicode-security v0.1.1

##[group]Clock drift check
  local time: Thu Feb 15 00:01:49 UTC 2024
Session terminated, killing shell...[RUSTC-TIMING] unicode_ident test:false 0.172
##[endgroup]
[RUSTC-TIMING] tracing_log test:false 0.580
    Checking time-core v0.1.2
[RUSTC-TIMING] time_core test:false 0.148

bors · 2024-02-15T00:02:31Z

💔 Test failed - checks-actions

michaelwoerister · 2024-02-15T10:06:00Z

@bors retry

bors · 2024-02-15T12:36:41Z

⌛ Testing commit 3f27e4b with merge fa9f77f...

bors · 2024-02-15T14:42:16Z

☀️ Test successful - checks-actions
Approved by: michaelwoerister
Pushing fa9f77f to master...

rust-timer · 2024-02-15T15:56:18Z

Finished benchmarking commit (fa9f77f): 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
Regressions ❌ (secondary)	0.2%	[0.2%, 0.2%]	4
Improvements ✅ (primary)	-0.2%	[-0.2%, -0.2%]	2
Improvements ✅ (secondary)	-6.6%	[-6.6%, -6.6%]	1
All ❌✅ (primary)	-0.2%	[-0.2%, -0.2%]	2

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

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)	1.5%	[1.5%, 1.5%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-4.8%	[-4.8%, -4.8%]	1
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 635.436s -> 637.562s (0.33%)
Artifact size: 306.15 MiB -> 306.31 MiB (0.05%)

First, introduce a typedef `DiagnosticArgMap`. Second, make the `args` field public, and remove the `args` getter and `replace_args setter. These were necessary previously because the getter had a `#[allow(rustc::potential_query_instability)]` attribute, but that was removed in rust-lang#120931 when the args were changed from `FxHashMap` to `FxIndexMap`. (All the other `Diagnostic` fields are public.)

pnkfelix · 2024-02-20T17:19:21Z

regressions are solely to the secondary many-assoc-items benchmark (check full, check/debug/opt incr-full), and was anticipated.
marking as triaged

@rustbot label: +perf-regression-triaged

First, introduce a typedef `DiagnosticArgMap`. Second, make the `args` field public, and remove the `args` getter and `replace_args` setter. These were necessary previously because the getter had a `#[allow(rustc::potential_query_instability)]` attribute, but that was removed in rust-lang#120931 when the args were changed from `FxHashMap` to `FxIndexMap`. (All the other `Diagnostic` fields are public.)

rustbot assigned michaelwoerister Feb 11, 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 Feb 11, 2024