Micro-optimize `InferCtxt::replace_bound_vars_with_placeholders` by avoiding unnecessary allocation #96021

martingms · 2022-04-13T19:08:07Z

Showed up hot in a callgrind profile of compiling bitmaps-3.1.0

replace_bound_vars allocates a BTreeMap that was only used for an .is_empty() check. This PR replaces the map with a simple boolean.

Local benchmarks:

Primary benchmarks

Benchmark	Profile	Scenario	% Change	Significance Factor?
diesel-1.4.8	check	full	-0.38%	1.91x
diesel-1.4.8	debug	full	-0.37%	1.87x
diesel-1.4.8	opt	full	-0.37%	1.85x
diesel-1.4.8	check	incr-full	-0.34%	1.72x
diesel-1.4.8	debug	incr-full	-0.33%	1.67x
diesel-1.4.8	opt	incr-full	-0.32%	1.61x

Secondary benchmarks

Benchmark	Profile	Scenario	% Change	Significance Factor?
coercions	debug	full	1.96%	9.81x
bitmaps-3.1.0	check	full	-1.54%	7.72x
bitmaps-3.1.0	debug	full	-1.48%	7.38x
bitmaps-3.1.0	opt	full	-1.39%	6.94x
bitmaps-3.1.0	check	incr-full	-1.26%	6.30x
bitmaps-3.1.0	debug	incr-full	-1.19%	5.97x
bitmaps-3.1.0	opt	incr-full	-1.15%	5.73x
tt-muncher	opt	full	-0.50%	2.50x
wf-projection-stress-65510	opt	full	-0.49%	2.47x
wf-projection-stress-65510	check	full	-0.44%	2.22x
wf-projection-stress-65510	debug	full	-0.42%	2.12x
wf-projection-stress-65510	check	incr-full	-0.40%	2.02x
deep-vector	debug	incr-patched: add vec item	-0.40%	1.98x
deep-vector	opt	incr-patched: println	0.39%	1.93x
wf-projection-stress-65510	debug	incr-full	-0.38%	1.92x
wf-projection-stress-65510	opt	incr-full	-0.37%	1.84x
projection-caching	check	full	-0.37%	1.83x
deep-vector	debug	full	-0.32%	1.61x

r? @nnethercote

compiler-errors · 2022-04-13T19:14:17Z

@bors try @rust-timer queue

rust-timer · 2022-04-13T19:14:18Z

Awaiting bors try build completion.

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

bors · 2022-04-13T19:14:25Z

⌛ Trying commit f764286 with merge 91117d057a604d5dd404a883f9714ef5d5588d8d...

jackh726 · 2022-04-13T19:41:41Z

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


        // If there were higher-ranked regions to replace, then actually create
        // the next universe (this avoids needlessly creating universes).
-        if !map.is_empty() {


Wait...is this wrong?

I think this should be taking into account types and consts too...

Do we see bound types in practice? Like other than in canonical queries, which I assume we liberate elsewhere? (maybe not)

We don't :) At least not much today. Which is likely why this isn't doesn't fail everywhere. But in the future, we'll probably see more.

Is the wrongness pre-existing? This change looks ok to me from a mechanical refactoring point of view, but I don't know much about this code in general.

Yeah, looks preexisting.

So do you have any concern with this PR being merged?

Nope! But someone (probably me) should file an issue so this doesn't get forgotten.

bors · 2022-04-13T20:41:47Z

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

rust-timer · 2022-04-13T20:41:49Z

Queued 91117d057a604d5dd404a883f9714ef5d5588d8d with parent 0d13f6a, future comparison URL.

rust-timer · 2022-04-14T05:39:25Z

Finished benchmarking commit (91117d057a604d5dd404a883f9714ef5d5588d8d): comparison url.

Summary:

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

	Regressions 😿 (primary)	Regressions 😿 (secondary)	Improvements 🎉 (primary)	Improvements 🎉 (secondary)	All 😿 🎉 (primary)
count¹	0	7	1	0	1
mean²	N/A	0.8%	-0.2%	N/A	-0.2%
max	N/A	1.2%	-0.2%	N/A	-0.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.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

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

nnethercote · 2022-04-14T06:58:39Z

Another weird performance result... I don't know what's happening. This change really seems like it should be a straight win.

nnethercote · 2022-04-20T21:03:53Z

We had some weird CI perf results in other PRs last week. Let's try this one again.

@bors try @rust-timer queue

rust-timer · 2022-04-20T21:03:54Z

Awaiting bors try build completion.

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

bors · 2022-04-20T21:04:06Z

⌛ Trying commit f764286 with merge 2ada63cfb077a0ca6434451a2d4f6bd5e3367450...

bors · 2022-04-20T22:54:34Z

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

rust-timer · 2022-04-20T22:54:36Z

Queued 2ada63cfb077a0ca6434451a2d4f6bd5e3367450 with parent 3e69bda, future comparison URL.

rust-timer · 2022-04-20T23:55:35Z

Finished benchmarking commit (2ada63cfb077a0ca6434451a2d4f6bd5e3367450): comparison url.

Summary:

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

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

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

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

nnethercote · 2022-05-26T22:54:52Z

#97345 landed recently. It entirely removes a great chunk of the compiler's work for bitmaps and similar benchmarks, including the calls to InferCtxt::replace_bound_vars_with_placeholders. So this PR isn't needed any more. Thanks again for the attempt, @martingms!

Avoid allocating region map when replacing bound vars

f764286

rust-highfive assigned nnethercote Apr 13, 2022

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

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

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

jackh726 reviewed Apr 13, 2022

View reviewed changes

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

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

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

JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 22, 2022

nnethercote closed this May 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Micro-optimize `InferCtxt::replace_bound_vars_with_placeholders` by avoiding unnecessary allocation #96021

Micro-optimize `InferCtxt::replace_bound_vars_with_placeholders` by avoiding unnecessary allocation #96021

martingms commented Apr 13, 2022

compiler-errors commented Apr 13, 2022

rust-timer commented Apr 13, 2022

bors commented Apr 13, 2022

jackh726 Apr 13, 2022

compiler-errors Apr 13, 2022

jackh726 Apr 13, 2022

nnethercote Apr 14, 2022

jackh726 Apr 14, 2022

nnethercote Apr 14, 2022

jackh726 Apr 14, 2022

bors commented Apr 13, 2022

rust-timer commented Apr 13, 2022

rust-timer commented Apr 14, 2022

nnethercote commented Apr 14, 2022

nnethercote commented Apr 20, 2022

rust-timer commented Apr 20, 2022

bors commented Apr 20, 2022

bors commented Apr 20, 2022

rust-timer commented Apr 20, 2022

rust-timer commented Apr 20, 2022

nnethercote commented May 26, 2022

Micro-optimize InferCtxt::replace_bound_vars_with_placeholders by avoiding unnecessary allocation #96021

Micro-optimize InferCtxt::replace_bound_vars_with_placeholders by avoiding unnecessary allocation #96021

Conversation

martingms commented Apr 13, 2022

Primary benchmarks

Secondary benchmarks

compiler-errors commented Apr 13, 2022

rust-timer commented Apr 13, 2022

bors commented Apr 13, 2022

jackh726 Apr 13, 2022

Choose a reason for hiding this comment

compiler-errors Apr 13, 2022

Choose a reason for hiding this comment

jackh726 Apr 13, 2022

Choose a reason for hiding this comment

nnethercote Apr 14, 2022

Choose a reason for hiding this comment

jackh726 Apr 14, 2022

Choose a reason for hiding this comment

nnethercote Apr 14, 2022

Choose a reason for hiding this comment

jackh726 Apr 14, 2022

Choose a reason for hiding this comment

bors commented Apr 13, 2022

rust-timer commented Apr 13, 2022

rust-timer commented Apr 14, 2022

Footnotes

nnethercote commented Apr 14, 2022

nnethercote commented Apr 20, 2022

rust-timer commented Apr 20, 2022

bors commented Apr 20, 2022

bors commented Apr 20, 2022

rust-timer commented Apr 20, 2022

rust-timer commented Apr 20, 2022

Footnotes

nnethercote commented May 26, 2022

Micro-optimize `InferCtxt::replace_bound_vars_with_placeholders` by avoiding unnecessary allocation #96021

Micro-optimize `InferCtxt::replace_bound_vars_with_placeholders` by avoiding unnecessary allocation #96021