Create a separate query for required and mentioned items instead of tracking them in the MIR body

[oli-obk]

implements #122862 (comment)

May permit further improvements without sacrificing perf... iff this PR isn't horrible for perf 🙃

[rustbot]

r? @fee1-dead

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

[rustbot]

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

[oli-obk]

@RalfJung I'm assuming the previous behaviour was a bug?

[RalfJung]

The intent was for it to pass with and without optimizations. Making this fail seems like it would be pretty bad for perf as we will have to consider all implicit destructors "mentioned" even when we know for sure they won't be called! I think it is quite important to gather "mentioned" items after drop elaboration, otherwise there's no way we are going to get decent performance here.

[rustbot]

The Miri subtree was changed

cc @rust-lang/miri

[oli-obk]

@bors try @rust-timer queue

[bors]

⌛ Trying commit c4e09b5 with merge 63e1afc...

[fee1-dead]

A bit busy this weekend, sorry

r? compiler

[bors]

☀️ Try build successful - checks-actions
Build commit: 63e1afc (63e1afcc70836ae4eb500e4e654d76ece4856564)

[saethlin]

r? saethlin

[rust-timer]

Finished benchmarking commit (63e1afc): comparison URL.

Overall result: ❌ regressions - 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.

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 a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	2.6%	[0.2%, 13.8%]	200
Regressions ❌ (secondary)	4.1%	[0.2%, 130.7%]	106
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.3%	[-0.3%, -0.3%]	1
All ❌✅ (primary)	2.6%	[0.2%, 13.8%]	200

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)	4.1%	[0.9%, 16.2%]	64
Regressions ❌ (secondary)	8.7%	[7.6%, 10.9%]	3
Improvements ✅ (primary)	-4.3%	[-7.3%, -1.5%]	6
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	3.4%	[-7.3%, 16.2%]	70

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)	4.9%	[0.8%, 18.7%]	123
Regressions ❌ (secondary)	7.8%	[0.9%, 120.8%]	52
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	4.9%	[0.8%, 18.7%]	123

Binary size

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%	[0.0%, 4.2%]	114
Regressions ❌ (secondary)	1.6%	[0.0%, 4.7%]	50
Improvements ✅ (primary)	-0.3%	[-0.3%, -0.1%]	9
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.9%	[-0.3%, 4.2%]	123

Bootstrap: 668.072s -> 673.251s (0.78%)
Artifact size: 317.99 MiB -> 318.17 MiB (0.06%)

[oli-obk]

@bors try @rust-timer queue

[bors]

⌛ Trying commit 27f29d1 with merge fdf29c5...

[bors]

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

[rust-timer]

Finished benchmarking commit (fdf29c5): comparison URL.

Overall result: ❌ regressions - 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.

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 a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	2.3%	[0.2%, 13.9%]	210
Regressions ❌ (secondary)	4.1%	[0.2%, 130.7%]	98
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.2%	[-0.3%, -0.2%]	4
All ❌✅ (primary)	2.3%	[0.2%, 13.9%]	210

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)	4.1%	[0.8%, 16.4%]	56
Regressions ❌ (secondary)	6.7%	[3.9%, 10.7%]	5
Improvements ✅ (primary)	-4.1%	[-8.1%, -1.8%]	5
Improvements ✅ (secondary)	-3.9%	[-3.9%, -3.9%]	1
All ❌✅ (primary)	3.4%	[-8.1%, 16.4%]	61

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)	4.5%	[0.6%, 18.0%]	122
Regressions ❌ (secondary)	8.1%	[1.0%, 121.7%]	45
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	4.5%	[0.6%, 18.0%]	122

Binary size

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%	[0.0%, 4.2%]	114
Regressions ❌ (secondary)	1.6%	[0.0%, 4.7%]	50
Improvements ✅ (primary)	-0.3%	[-0.4%, -0.1%]	9
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.9%	[-0.4%, 4.2%]	123

Bootstrap: 667.559s -> 673.416s (0.88%)
Artifact size: 318.06 MiB -> 318.11 MiB (0.02%)

[RalfJung]

Not incorporating inlined consts can lead to ICEs in the interpreter: we may be inlining a function that contains a const that fails. The interpreter will evaluate all required_consts (which would not include the inlined const) and then assert that all const-eval during function evaluation succeeds.

[RalfJung]

Removing this optimization will also not be great for perf, as it means the "mentioned items" traversal has to visit all the functions that got inlined.

[RalfJung]

A large part of the perf problems probably comes from the fact that mentioned_items is a lot bigger with this PR than it is currently. Ideally we can keep the set itself entirely unchanged, and just change the way it is stored. I don't know what is the best way to do that. Maybe something like this:

A MIR body contains something similar to a Steal<Vec<...>>. During MIR building the items are present. At the end of computing optimized_mir, before putting the final MIR into the query result, mentioned items get stolen and the mentioned items query is fed with the result. This way they get serialized separately. (Probably we actually want a 3-state enum: items not-yet-present, items present, items stolen.)

In the local crate, when we ask for mentioned items, we run the optimized_mir query and then we can return the mentioned_items result.

For dependencies, when we ask for mentioned items, we deserialize them from disk -- and we don't have to load the entire MIR body for that.