Avoid a reverse map that is only used in diagnostics paths #139584

oli-obk · 2025-04-09T13:56:05Z

iterating a map until a value matches and returning the key is bad obviously, but it happens very rarely and only on diagnostics paths. It would also be a lot cheaper with #138995. Which is actually why I'm trying this out, that PR adds a new entry in create_def, which makes create_def show up in cachegrind. So I'm trying out if removing adding an entry in create_def is a perf improvement

oli-obk · 2025-04-09T13:56:14Z

@bors try @rust-timer queue

Avoid a reverse map that is only used in diagnostics paths r? `@ghost` iterating a map until a value matches and returning the key is bad obviously, but it happens very rarely and only on diagnostics paths. It would also be a lot cheaper with rust-lang#138995. Which is actually why I'm trying this out, that PR adds a new entry in `create_def`, which makes `create_def` show up in cachegrind. So I'm trying out if removing adding an entry in `create_def` is a perf improvement

bors · 2025-04-09T13:57:28Z

⌛ Trying commit e4bd751 with merge a6c5ab5...

compiler/rustc_data_structures/src/unord.rs

bors · 2025-04-09T16:04:31Z

☀️ Try build successful - checks-actions
Build commit: a6c5ab5 (a6c5ab580ffb0558bf0624c3e8572e5139810658)

rust-timer · 2025-04-09T18:43:55Z

Finished benchmarking commit (a6c5ab5): comparison URL.

Overall result: ❌ regressions - please read the text below

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-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.4%	[0.1%, 0.6%]	14
Regressions ❌ (secondary)	1.4%	[0.5%, 3.3%]	10
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.4%	[0.1%, 0.6%]	14

Max RSS (memory usage)

Results (primary 2.3%, secondary -0.6%)

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)	6.3%	[6.3%, 6.3%]	1
Regressions ❌ (secondary)	4.8%	[4.8%, 4.8%]	1
Improvements ✅ (primary)	-1.6%	[-1.6%, -1.6%]	1
Improvements ✅ (secondary)	-6.0%	[-6.0%, -6.0%]	1
All ❌✅ (primary)	2.3%	[-1.6%, 6.3%]	2

Cycles

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

Binary size

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

Bootstrap: 779.721s -> 780.693s (0.12%)
Artifact size: 366.13 MiB -> 366.10 MiB (-0.01%)

oli-obk · 2025-04-10T06:46:34Z

@bors try @rust-timer queue

Avoid a reverse map that is only used in diagnostics paths r? `@ghost` iterating a map until a value matches and returning the key is bad obviously, but it happens very rarely and only on diagnostics paths. It would also be a lot cheaper with rust-lang#138995. Which is actually why I'm trying this out, that PR adds a new entry in `create_def`, which makes `create_def` show up in cachegrind. So I'm trying out if removing adding an entry in `create_def` is a perf improvement

bors · 2025-04-10T06:47:46Z

⌛ Trying commit ae8c33c with merge 40f4c1b...

bors · 2025-04-10T08:54:43Z

☀️ Try build successful - checks-actions
Build commit: 40f4c1b (40f4c1b559f57e9c066368234f084422829ff140)

rust-timer · 2025-04-10T10:10:15Z

Finished benchmarking commit (40f4c1b): 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
Improvements ✅ (secondary)	-0.5%	[-0.9%, -0.1%]	2
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary -2.5%, secondary -1.0%)

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)	5.3%	[5.3%, 5.3%]	1
Improvements ✅ (primary)	-2.5%	[-3.0%, -2.0%]	2
Improvements ✅ (secondary)	-2.5%	[-3.1%, -1.8%]	4
All ❌✅ (primary)	-2.5%	[-3.0%, -2.0%]	2

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: 780.636s -> 779.061s (-0.20%)
Artifact size: 366.13 MiB -> 366.12 MiB (-0.00%)

rustbot · 2025-04-10T10:29:31Z

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

compiler/rustc_resolve/src/lib.rs

petrochenkov · 2025-04-10T16:14:54Z

r=me with #139584 (comment) addressed.
@rustbot author

rustbot · 2025-04-10T16:14:58Z

Reminder, once the PR becomes ready for a review, use @rustbot ready.

compiler/rustc_resolve/src/late.rs

oli-obk · 2025-04-11T10:15:51Z

@rustbot ready

petrochenkov · 2025-04-11T12:33:48Z

@bors r+

bors · 2025-04-11T12:33:50Z

📌 Commit 98d51fb has been approved by petrochenkov

It is now in the queue for this repository.

oli-obk · 2025-04-11T13:07:25Z

@bors rollup=maybe

Rollup of 7 pull requests Successful merges: - rust-lang#137835 (Use `BinOp::Cmp` for `iNN::signum`) - rust-lang#139584 (Avoid a reverse map that is only used in diagnostics paths) - rust-lang#139638 (Cleanup the `InstSimplify` MIR transformation) - rust-lang#139653 (Handle a negated literal in `eat_token_lit`.) - rust-lang#139662 (Tweak `DefPathData`) - rust-lang#139664 (Reuse address-space computation from global alloc) - rust-lang#139687 (Add spastorino to users_on_vacation) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#139584 - oli-obk:horrible-experiment-1, r=petrochenkov Avoid a reverse map that is only used in diagnostics paths r? `@petrochenkov` iterating a map until a value matches and returning the key is bad obviously, but it happens very rarely and only on diagnostics paths. It would also be a lot cheaper with rust-lang#138995. Which is actually why I'm trying this out, that PR adds a new entry in `create_def`, which makes `create_def` show up in cachegrind. So I'm trying out if removing adding an entry in `create_def` is a perf improvement

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 Apr 9, 2025

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 Apr 9, 2025

petrochenkov reviewed Apr 9, 2025

View reviewed changes

compiler/rustc_data_structures/src/unord.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Apr 9, 2025

oli-obk force-pushed the horrible-experiment-1 branch from e4bd751 to ae8c33c Compare April 10, 2025 06:46

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 Apr 10, 2025

This comment has been minimized.

Sign in to view

rustbot removed S-waiting-on-perf Status: Waiting on a perf run to be completed. perf-regression Performance regression. labels Apr 10, 2025

rustbot assigned petrochenkov Apr 10, 2025

oli-obk marked this pull request as ready for review April 10, 2025 10:29

oli-obk force-pushed the horrible-experiment-1 branch from ae8c33c to fa73472 Compare April 10, 2025 10:31

petrochenkov reviewed Apr 10, 2025

View reviewed changes

compiler/rustc_resolve/src/lib.rs Show resolved Hide resolved

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 Apr 10, 2025

oli-obk force-pushed the horrible-experiment-1 branch from fa73472 to 2662495 Compare April 11, 2025 08:21

petrochenkov reviewed Apr 11, 2025

View reviewed changes

compiler/rustc_resolve/src/late.rs Outdated Show resolved Hide resolved

oli-obk added 5 commits April 11, 2025 09:33

Avoid a reverse map that is only used in diagnostics paths

24efefa

Avoid another node_id_to_def_id call

f5c60f6

Avoid storing the LocalDefId twice

33a6820

Avoid a node_id_to_def_id call by just storing DefIds instead of NodeIds

d35e830

Only compute the DefId when a diagnostic is definitely emitted

98d51fb

oli-obk force-pushed the horrible-experiment-1 branch from 2662495 to 98d51fb Compare April 11, 2025 10:15

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

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 Apr 11, 2025

jhpratt mentioned this pull request Apr 11, 2025

Rollup of 7 pull requests #139689

Merged

bors merged commit bc05aae into rust-lang:master Apr 11, 2025
6 checks passed

rustbot added this to the 1.88.0 milestone Apr 11, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid a reverse map that is only used in diagnostics paths #139584

Avoid a reverse map that is only used in diagnostics paths #139584

oli-obk commented Apr 9, 2025 •

edited

Loading

oli-obk commented Apr 9, 2025

This comment has been minimized.

bors commented Apr 9, 2025

bors commented Apr 9, 2025

This comment has been minimized.

rust-timer commented Apr 9, 2025

oli-obk commented Apr 10, 2025

This comment has been minimized.

bors commented Apr 10, 2025

bors commented Apr 10, 2025

This comment has been minimized.

rust-timer commented Apr 10, 2025

rustbot commented Apr 10, 2025

petrochenkov commented Apr 10, 2025

rustbot commented Apr 10, 2025

oli-obk commented Apr 11, 2025

petrochenkov commented Apr 11, 2025

bors commented Apr 11, 2025

oli-obk commented Apr 11, 2025

Avoid a reverse map that is only used in diagnostics paths #139584

Avoid a reverse map that is only used in diagnostics paths #139584

Conversation

oli-obk commented Apr 9, 2025 • edited Loading

oli-obk commented Apr 9, 2025

This comment has been minimized.

bors commented Apr 9, 2025

bors commented Apr 9, 2025

This comment has been minimized.

rust-timer commented Apr 9, 2025

Overall result: ❌ regressions - please read the text below

oli-obk commented Apr 10, 2025

This comment has been minimized.

bors commented Apr 10, 2025

bors commented Apr 10, 2025

This comment has been minimized.

rust-timer commented Apr 10, 2025

Overall result: ✅ improvements - no action needed

rustbot commented Apr 10, 2025

petrochenkov commented Apr 10, 2025

rustbot commented Apr 10, 2025

oli-obk commented Apr 11, 2025

petrochenkov commented Apr 11, 2025

bors commented Apr 11, 2025

oli-obk commented Apr 11, 2025

oli-obk commented Apr 9, 2025 •

edited

Loading