cleanup bound variable handling #97648

lcnr · 2022-06-02T11:42:09Z

each commit should be pretty self-contained and hopefully straightforward to review.

I've added 677ec23a8dbf8ff5f1c03ccebd46f8b85e5ec1fc so that we can stop returning the region map from replace_bound_vars_with_fresh_vars in the following commit.

r? @nikomatsakis or @jackh726

lcnr · 2022-06-02T11:42:29Z

@bors try @rust-timer queue

rust-timer · 2022-06-02T11:42:30Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2022-06-02T11:42:37Z

⌛ Trying commit 1f034249b78c28e3af4386ec181270785d3f2e27 with merge 764220f5017763a43ddc644fcece3f0c0054228c...

jackh726 · 2022-06-02T12:02:25Z

r=me with clean perf

bors · 2022-06-02T13:09:08Z

☀️ Try build successful - checks-actions
Build commit: 764220f5017763a43ddc644fcece3f0c0054228c (764220f5017763a43ddc644fcece3f0c0054228c)

rust-timer · 2022-06-02T13:09:10Z

Queued 764220f5017763a43ddc644fcece3f0c0054228c with parent 5e6bb83, future comparison URL.

rust-timer · 2022-06-02T14:34:57Z

Finished benchmarking commit (764220f5017763a43ddc644fcece3f0c0054228c): comparison url.

Instruction count

Primary benchmarks: mixed results
Secondary benchmarks: mixed results

	mean¹	max	count²
Regressions 😿 (primary)	1.3%	1.4%	2
Regressions 😿 (secondary)	3.7%	4.2%	6
Improvements 🎉 (primary)	-0.2%	-0.3%	7
Improvements 🎉 (secondary)	-0.3%	-0.6%	15
All 😿🎉 (primary)	0.1%	1.4%	9

Max RSS (memory usage)

Results

Primary benchmarks: 🎉 relevant improvement found
Secondary benchmarks: 🎉 relevant improvements found

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	-2.3%	-2.3%	1
Improvements 🎉 (secondary)	-3.1%	-4.5%	4
All 😿🎉 (primary)	-2.3%	-2.3%	1

Cycles

Results

Primary benchmarks: mixed results
Secondary benchmarks: mixed results

	mean¹	max	count²
Regressions 😿 (primary)	2.8%	2.8%	1
Regressions 😿 (secondary)	2.7%	3.1%	2
Improvements 🎉 (primary)	-2.3%	-2.3%	1
Improvements 🎉 (secondary)	-1.9%	-2.0%	2
All 😿🎉 (primary)	0.2%	2.8%	2

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

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-review -S-waiting-on-perf +perf-regression

the arithmetic mean of the percent change ↩ ↩² ↩³
number of relevant changes ↩ ↩² ↩³

jackh726 · 2022-06-02T19:43:54Z

compiler/rustc_infer/src/infer/higher_ranked/mod.rs

@@ -76,21 +83,25 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
        // (i.e., if there are no placeholders).
        let next_universe = self.universe().next_universe();

+        let replaced_bound_var = Cell::new(false);


I wouldn't be surprised if this is the cause of the perf regression.

lcnr · 2022-06-08T16:48:07Z

@bors try @rust-timer queue

rust-timer · 2022-06-08T16:48:08Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2022-06-08T16:48:15Z

⌛ Trying commit 6b1affc8eee5f2cdda28f147f3d5afa5376afa84 with merge 334a9e57d7951e2e681a8459304b8add7519138e...

bors · 2022-06-08T18:38:40Z

☀️ Try build successful - checks-actions
Build commit: 334a9e57d7951e2e681a8459304b8add7519138e (334a9e57d7951e2e681a8459304b8add7519138e)

rust-timer · 2022-06-08T18:38:42Z

Queued 334a9e57d7951e2e681a8459304b8add7519138e with parent 09d52bc, future comparison URL.

rust-timer · 2022-06-08T20:14:54Z

Finished benchmarking commit (334a9e57d7951e2e681a8459304b8add7519138e): comparison url.

Instruction count

Primary benchmarks: mixed results
Secondary benchmarks: mixed results

	mean¹	max	count²
Regressions 😿 (primary)	0.3%	0.7%	10
Regressions 😿 (secondary)	0.8%	2.1%	25
Improvements 🎉 (primary)	-0.4%	-0.5%	9
Improvements 🎉 (secondary)	-0.4%	-0.5%	7
All 😿🎉 (primary)	-0.0%	0.7%	19

Max RSS (memory usage)

Results

Primary benchmarks: no relevant changes found
Secondary benchmarks: mixed results

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	3.2%	5.0%	4
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	-2.7%	-2.8%	2
All 😿🎉 (primary)	N/A	N/A	0

Cycles

Results

Primary benchmarks: mixed results
Secondary benchmarks: no relevant changes found

	mean¹	max	count²
Regressions 😿 (primary)	2.7%	2.7%	1
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	-2.8%	-2.8%	1
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	-0.1%	-2.8%	2

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

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-review -S-waiting-on-perf +perf-regression

the arithmetic mean of the percent change ↩ ↩² ↩³
number of relevant changes ↩ ↩² ↩³

compiler/rustc_infer/src/infer/higher_ranked/mod.rs

bors · 2022-06-10T10:20:33Z

☀️ Try build successful - checks-actions
Build commit: 86aca0894812c777883f063244a3e238a38aadc0 (86aca0894812c777883f063244a3e238a38aadc0)

rust-timer · 2022-06-10T10:20:35Z

Queued 86aca0894812c777883f063244a3e238a38aadc0 with parent 52ee2a2, future comparison URL.

rust-timer · 2022-06-10T11:45:45Z

Finished benchmarking commit (86aca0894812c777883f063244a3e238a38aadc0): comparison url.

Instruction count

Primary benchmarks: mixed results
Secondary benchmarks: 😿 relevant regressions found

	mean¹	max	count²
Regressions 😿 (primary)	1.1%	1.4%	3
Regressions 😿 (secondary)	1.3%	3.3%	29
Improvements 🎉 (primary)	-0.4%	-0.5%	10
Improvements 🎉 (secondary)	-0.4%	-0.5%	4
All 😿🎉 (primary)	-0.0%	1.4%	13

Max RSS (memory usage)

Results

Primary benchmarks: 😿 relevant regression found
Secondary benchmarks: 🎉 relevant improvement found

	mean¹	max	count²
Regressions 😿 (primary)	2.5%	2.5%	1
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	-3.1%	-3.1%	1
All 😿🎉 (primary)	2.5%	2.5%	1

Cycles

Results

Primary benchmarks: no relevant changes found
Secondary benchmarks: 😿 relevant regressions found

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	2.6%	2.7%	2
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	N/A	N/A	0

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

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-review -S-waiting-on-perf +perf-regression

the arithmetic mean of the percent change ↩ ↩² ↩³
number of relevant changes ↩ ↩² ↩³

lcnr · 2022-06-10T12:29:01Z

compiler/rustc_infer/src/infer/mod.rs

+            return inner;
+        }
+
+        let mut region_map = FxHashMap::default();


it looks like the perf regression is caused by this code being pretty hot and us always creating 3 separate storages here and then having to drop them.

Went from BTreeMap -> SsoHashMap -> FxHashMap to figure out if these help. The more complex Drop impl (and greater size) of SsoHashMap made things worse. So hopefully the comparatively simple HashMap ends up being an improvement.

If not I think we should commit to tracking bound vars in binders and modify this to eagerly create a Vec of inference vars using value.bound_vars() and then using that. Doing so should allow us to get some perf improvements in other areas as well.

lcnr · 2022-06-10T12:29:35Z

@bors try @rust-timer queue

rust-timer · 2022-06-10T12:29:36Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2022-06-10T12:29:42Z

⌛ Trying commit efdf948 with merge b1d9f0069e496cbc091da1b5e708140a8b6c7da6...

bors · 2022-06-10T14:22:44Z

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

rust-timer · 2022-06-10T14:22:47Z

Queued b1d9f0069e496cbc091da1b5e708140a8b6c7da6 with parent f19ccc2, future comparison URL.

rust-timer · 2022-06-10T15:39:44Z

Finished benchmarking commit (b1d9f0069e496cbc091da1b5e708140a8b6c7da6): comparison url.

Instruction count

Primary benchmarks: 🎉 relevant improvements found
Secondary benchmarks: mixed results

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	0.3%	0.3%	5
Improvements 🎉 (primary)	-0.6%	-1.4%	20
Improvements 🎉 (secondary)	-0.5%	-0.6%	15
All 😿🎉 (primary)	-0.6%	-1.4%	20

Max RSS (memory usage)

Results

Primary benchmarks: 🎉 relevant improvement found
Secondary benchmarks: 😿 relevant regressions found

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	3.1%	4.2%	2
Improvements 🎉 (primary)	-2.3%	-2.3%	1
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	-2.3%	-2.3%	1

Cycles

Results

Primary benchmarks: no relevant changes found
Secondary benchmarks: 😿 relevant regression found

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	2.1%	2.1%	1
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	N/A	N/A	0

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

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-review -S-waiting-on-perf -perf-regression

the arithmetic mean of the percent change ↩ ↩² ↩³
number of relevant changes ↩ ↩² ↩³

lcnr · 2022-06-10T16:55:05Z

perf is pretty stupid 🤷 whatever

@bors r=jackh726

bors · 2022-06-10T16:55:07Z

📌 Commit efdf948 has been approved by jackh726

bors · 2022-06-11T01:39:46Z

⌛ Testing commit efdf948 with merge 75307c2...

bors · 2022-06-11T04:00:17Z

☀️ Test successful - checks-actions
Approved by: jackh726
Pushing 75307c2 to master...

rust-timer · 2022-06-11T05:19:09Z

Finished benchmarking commit (75307c2): comparison url.

Instruction count

Primary benchmarks: 🎉 relevant improvements found
Secondary benchmarks: mixed results

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	0.3%	0.3%	4
Improvements 🎉 (primary)	-0.7%	-1.7%	22
Improvements 🎉 (secondary)	-0.5%	-0.6%	16
All 😿🎉 (primary)	-0.7%	-1.7%	22

Max RSS (memory usage)

Results

Primary benchmarks: mixed results
Secondary benchmarks: no relevant changes found

	mean¹	max	count²
Regressions 😿 (primary)	2.0%	2.0%	1
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	-1.8%	-1.8%	1
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	0.1%	2.0%	2

Cycles

Results

Primary benchmarks: no relevant changes found
Secondary benchmarks: 🎉 relevant improvements found

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	-2.9%	-4.0%	3
All 😿🎉 (primary)	N/A	N/A	0

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

the arithmetic mean of the percent change ↩ ↩² ↩³
number of relevant changes ↩ ↩² ↩³

pnkfelix · 2022-06-14T22:07:28Z

mixed results, but the improvements far outweigh the regressions here.
marking as triaged

@rustbot label: +perf-regression-triaged.

rust-highfive assigned nikomatsakis Jun 2, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jun 2, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 2, 2022

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jun 2, 2022

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Jun 2, 2022

jackh726 reviewed Jun 2, 2022

View reviewed changes

lcnr force-pushed the bound-var-replacer branch 2 times, most recently from 8cb0a81 to 6b1affc Compare June 8, 2022 16:47

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jun 8, 2022

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jun 8, 2022

lcnr commented Jun 9, 2022

View reviewed changes

compiler/rustc_infer/src/infer/higher_ranked/mod.rs Outdated Show resolved Hide resolved

lcnr force-pushed the bound-var-replacer branch from 4ccebd0 to 4ff98fa Compare June 10, 2022 08:05

lcnr added 2 commits June 10, 2022 10:09

BoundVarReplacer remove Option

8a0cb6a

only expect lb lt for fn calls in mir typeck

434c7da

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jun 10, 2022

use FxHashMap instead of BTreeMap

efdf948

lcnr force-pushed the bound-var-replacer branch from 9bb1058 to efdf948 Compare June 10, 2022 12:24

lcnr commented Jun 10, 2022

View reviewed changes

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jun 10, 2022

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

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 Jun 10, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 11, 2022

bors merged commit 75307c2 into rust-lang:master Jun 11, 2022

rustbot added this to the 1.63.0 milestone Jun 11, 2022

lcnr deleted the bound-var-replacer branch June 11, 2022 15:00

rustbot added the perf-regression-triaged The performance regression has been triaged. label Jun 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cleanup bound variable handling #97648

cleanup bound variable handling #97648

lcnr commented Jun 2, 2022

lcnr commented Jun 2, 2022

rust-timer commented Jun 2, 2022

bors commented Jun 2, 2022

jackh726 commented Jun 2, 2022

bors commented Jun 2, 2022

rust-timer commented Jun 2, 2022

rust-timer commented Jun 2, 2022

jackh726 Jun 2, 2022

lcnr commented Jun 8, 2022

rust-timer commented Jun 8, 2022

bors commented Jun 8, 2022

bors commented Jun 8, 2022

rust-timer commented Jun 8, 2022

rust-timer commented Jun 8, 2022

bors commented Jun 10, 2022

rust-timer commented Jun 10, 2022

rust-timer commented Jun 10, 2022

lcnr Jun 10, 2022 •

edited

Loading

lcnr commented Jun 10, 2022

rust-timer commented Jun 10, 2022

bors commented Jun 10, 2022

bors commented Jun 10, 2022

rust-timer commented Jun 10, 2022

rust-timer commented Jun 10, 2022

lcnr commented Jun 10, 2022

bors commented Jun 10, 2022

bors commented Jun 11, 2022

bors commented Jun 11, 2022

rust-timer commented Jun 11, 2022

pnkfelix commented Jun 14, 2022

cleanup bound variable handling #97648

cleanup bound variable handling #97648

Conversation

lcnr commented Jun 2, 2022

lcnr commented Jun 2, 2022

rust-timer commented Jun 2, 2022

bors commented Jun 2, 2022

jackh726 commented Jun 2, 2022

bors commented Jun 2, 2022

rust-timer commented Jun 2, 2022

rust-timer commented Jun 2, 2022

Footnotes

Choose a reason for hiding this comment

lcnr commented Jun 8, 2022

rust-timer commented Jun 8, 2022

bors commented Jun 8, 2022

bors commented Jun 8, 2022

rust-timer commented Jun 8, 2022

rust-timer commented Jun 8, 2022

Footnotes

bors commented Jun 10, 2022

rust-timer commented Jun 10, 2022

rust-timer commented Jun 10, 2022

Footnotes

lcnr Jun 10, 2022 • edited Loading

Choose a reason for hiding this comment

lcnr commented Jun 10, 2022

rust-timer commented Jun 10, 2022

bors commented Jun 10, 2022

bors commented Jun 10, 2022

rust-timer commented Jun 10, 2022

rust-timer commented Jun 10, 2022

Footnotes

lcnr commented Jun 10, 2022

bors commented Jun 10, 2022

bors commented Jun 11, 2022

bors commented Jun 11, 2022

rust-timer commented Jun 11, 2022

Footnotes

pnkfelix commented Jun 14, 2022

lcnr Jun 10, 2022 •

edited

Loading