Fix visible_parent_map fallback map merging perf regression

[zalanlevai]

View all comments

This PR attempts to fix the visible_parent_map perf regression introduced in #160464, which was found in a post-merge perf run.

Because the original PR fixes unnecessary iterations in the breadth-first search (BFS), meaning that it only reduces the amount of work during the BFS, the likely cause of the perf regression is the changed merging of the fallback map into the final visible parent map after the BFS.

The goal of this PR is to determine whether this is the case through a perf try run. The change itself works around the Unord* APIs, but might be worth it to work around the perf regression.

• I did not use an LLM to create a change in this PR.
• I used an LLM to create a change in this PR, and I have explained below how it was used.

[JonathanBrouwer]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: 9cbccc7 (9cbccc762cb6f14132feb088e9dd05ce77b54760)
Base parent: 153ecc4 (153ecc4f74035b709bb3e1eb9546f1d934865042)

[rust-timer]

Finished benchmarking commit (9cbccc7): comparison URL.

Overall result: ❌✅ regressions and improvements - please read:

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.

Next, please: If you can, justify the regressions found in this try perf run in writing along with @rustbot label: +perf-regression-triaged. If not, fix the regressions and do another perf run. Neutral or positive results will clear the label automatically.

@bors rollup=never rustc-perf
@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.4%	[0.2%, 0.5%]	7
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.4%	[-0.5%, -0.4%]	6
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary -9.2%, secondary -0.4%)

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)	0.6%	[0.5%, 0.8%]	5
Improvements ✅ (primary)	-9.2%	[-9.2%, -9.2%]	1
Improvements ✅ (secondary)	-0.8%	[-2.4%, -0.4%]	10
All ❌✅ (primary)	-9.2%	[-9.2%, -9.2%]	1

Cycles

Results (secondary 0.2%)

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.2%	[0.4%, 5.0%]	11
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.7%	[-1.6%, -0.4%]	11
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 458.036s -> 460.145s (0.46%)
Artifact size: 398.61 MiB -> 398.66 MiB (0.01%)

[zalanlevai]

@JonathanBrouwer Thanks! I had a look through the perf run results and compared each benchmark that regressed in #160464 with the corresponding benchmark in this perf run. This change resolves all 7 perf regressions introduced in #160464, with the percentage deltas in this perf run cancelling out the ones in the perf run of the regressed PR.

This change's perf run also shows 7 other perf regressions of similar magnitude: 5 are the deep-vector benchmark and the other two are the tuple-stress benchmark. However, looking at their code, these two benchmarks do not contain code that would be affected by this change, at least from what I can tell. Looking at the profiles for these regressed benchmarks, the visible_parent_map query is not significant enough to show up in any of the before or after profiles. Do you think these new regressions could be a fluke or unrelated to this change?

[zalanlevai]

I believe this is the correct label given the findings in my previous comment.

@rustbot label +perf-regression-triaged

[rustbot]

r? @davidtwco

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

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler
• compiler expanded to 75 candidates
• Random selection from 19 candidates

[panstromek]

This change resolves all 7 perf regressions introduced in #160464

I think Jonathan won't have time today, so I'll just confirm, this looks legit, I think your assessment is correct.

Do you think these new regressions could be a fluke or unrelated to this change?

The deep-vector and tuple-stress changes are a bit puzzling, but it could be some artifact noise, we see that relatively often, especially for those tiny stress tests. We can try to re-measure with up to date parent or just wait on post-merge results, that'll usually show something different so we know it's noise.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[zalanlevai]

Thank you @panstromek for taking a look at this! Just to be safe, I rebased the PR onto latest main to see whether these new regressions are real. Could you please re-run a try build perf run on this so we can see whether the results are any different?

[panstromek]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: a7be450 (a7be4507d612028cbc821ef97eefd7d31c20ed10)
Base parent: 7088e4b (7088e4b63a9516ebfbfe2ab2d999cf01a528ac14)

[rust-timer]

Finished benchmarking commit (a7be450): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up.

@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
Improvements ✅ (secondary)	-0.4%	[-0.5%, -0.4%]	6
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (secondary -0.3%)

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)	2.6%	[1.0%, 6.0%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.1%	[-3.4%, -0.4%]	12
All ❌✅ (primary)	-	-	0

Cycles

Results (secondary 1.8%)

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)	3.8%	[0.4%, 16.4%]	13
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.8%	[-1.2%, -0.5%]	10
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 455.179s -> 454.498s (-0.15%)
Artifact size: 398.37 MiB -> 399.13 MiB (0.19%)

[zalanlevai]

Yay! This perf run neatly shows that the original regressions are still gone (including wg-grammar, which is just right below the significance threshold to be shown in the overview) and that no other benchmarks were affected by this change.

[JonathanBrouwer]

@bors r+
Thanks for taking a look at this!

View changes since this review

[rust-bors]

📌 Commit d577cbc has been approved by JonathanBrouwer

It is now in the queue for this repository.

[JonathanBrouwer]

@bors rollup=iffy
Perf effect is pretty small so I'm throwing this in a small rolluo