Create an `AllocId` for `ConstValue::Slice`. #116707

cjgillot · 2023-10-13T20:39:21Z

This PR modifies ConstValue::Slice to use an AllocId instead of directly manipulating the allocation. This was originally proposed by #115764 but was a perf regression.

Almost 2 years later, enough code has changed to make this a perf improvement: #116707 (comment)

cjgillot · 2023-10-13T20:39:40Z

@bors try @rust-timer queue

bors · 2023-10-13T20:40:49Z

⌛ Trying commit 334753f with merge 9ef21e1...

Create an `AllocId` for `ConstValue::Slice`. r? `@ghost`

bors · 2023-10-13T21:53:15Z

☀️ Try build successful - checks-actions
Build commit: 9ef21e1 (9ef21e1d83c6c9220b072fd1a4f225949514cc28)

RalfJung · 2023-10-13T22:02:01Z

How does this differ from the almost identical perf experiment I did a few weeks ago? Here are the perf results.

rust-timer · 2023-10-14T00:18:48Z

Finished benchmarking commit (9ef21e1): comparison URL.

Overall result: ❌✅ regressions and improvements - 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)	1.7%	[1.7%, 1.7%]	1
Regressions ❌ (secondary)	2.5%	[0.4%, 7.7%]	7
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.2%	[-1.3%, -1.0%]	3
All ❌✅ (primary)	1.7%	[1.7%, 1.7%]	1

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)	1.5%	[0.9%, 2.2%]	3
Regressions ❌ (secondary)	3.8%	[0.8%, 14.5%]	17
Improvements ✅ (primary)	-1.6%	[-1.6%, -1.6%]	1
Improvements ✅ (secondary)	-5.3%	[-10.2%, -0.9%]	16
All ❌✅ (primary)	0.7%	[-1.6%, 2.2%]	4

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)	-	-	0
Regressions ❌ (secondary)	7.0%	[2.1%, 21.5%]	14
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

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)	0.1%	[0.0%, 0.3%]	30
Regressions ❌ (secondary)	0.0%	[0.0%, 0.1%]	9
Improvements ✅ (primary)	-0.0%	[-0.1%, -0.0%]	3
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.1%	[-0.1%, 0.3%]	33

Bootstrap: 628.692s -> 625.904s (-0.44%)
Artifact size: 271.29 MiB -> 271.29 MiB (0.00%)

cjgillot · 2023-10-14T10:26:37Z

No real difference. I just couldn't find your version.
Now there is one.

@bors try @rust-timer queue

bors · 2023-10-14T10:27:48Z

⌛ Trying commit e195fe8 with merge eafbd55...

Create an `AllocId` for `ConstValue::Slice`. r? `@ghost`

bors · 2023-10-14T11:40:41Z

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

rust-timer · 2023-10-14T21:03:51Z

Finished benchmarking commit (eafbd55): comparison URL.

Overall result: ❌✅ regressions and improvements - 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)	0.6%	[0.1%, 1.8%]	4
Regressions ❌ (secondary)	1.7%	[0.2%, 7.8%]	12
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.1%	[-1.2%, -1.0%]	3
All ❌✅ (primary)	0.6%	[0.1%, 1.8%]	4

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)	1.6%	[1.2%, 2.1%]	3
Regressions ❌ (secondary)	2.9%	[1.1%, 8.7%]	15
Improvements ✅ (primary)	-1.9%	[-3.3%, -0.5%]	2
Improvements ✅ (secondary)	-6.0%	[-9.9%, -1.0%]	19
All ❌✅ (primary)	0.2%	[-3.3%, 2.1%]	5

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)	-	-	0
Regressions ❌ (secondary)	8.0%	[2.6%, 13.7%]	4
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.5%	[-2.5%, -2.5%]	1
All ❌✅ (primary)	-	-	0

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)	0.1%	[0.0%, 0.3%]	30
Regressions ❌ (secondary)	0.0%	[0.0%, 0.1%]	9
Improvements ✅ (primary)	-0.1%	[-0.1%, -0.0%]	3
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.1%	[-0.1%, 0.3%]	33

Bootstrap: 627.527s -> 627.366s (-0.03%)
Artifact size: 271.26 MiB -> 271.31 MiB (0.02%)

cjgillot · 2025-07-13T13:17:13Z

@bors try @rust-timer queue

rust-timer · 2025-07-13T16:42:12Z

Finished benchmarking commit (f8fc695): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @rustbot label: +perf-regression-triaged. If not, please fix the regressions and do another perf run. If its results are neutral or positive, the label will be automatically removed.

@bors rollup=never
@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.9%	[0.8%, 1.0%]	4
Regressions ❌ (secondary)	0.4%	[0.0%, 0.8%]	29
Improvements ✅ (primary)	-0.3%	[-0.7%, -0.1%]	3
Improvements ✅ (secondary)	-3.7%	[-10.3%, -0.0%]	24
All ❌✅ (primary)	0.4%	[-0.7%, 1.0%]	7

Max RSS (memory usage)

Results (primary 0.3%, secondary -4.6%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	6.4%	[6.4%, 6.4%]	1
Regressions ❌ (secondary)	3.7%	[1.8%, 9.3%]	6
Improvements ✅ (primary)	-2.7%	[-3.8%, -1.7%]	2
Improvements ✅ (secondary)	-7.0%	[-10.5%, -1.2%]	21
All ❌✅ (primary)	0.3%	[-3.8%, 6.4%]	3

Cycles

Results (primary -2.4%, secondary -9.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)	3.6%	[2.0%, 5.4%]	5
Improvements ✅ (primary)	-2.4%	[-2.4%, -2.4%]	1
Improvements ✅ (secondary)	-13.2%	[-17.9%, -2.6%]	16
All ❌✅ (primary)	-2.4%	[-2.4%, -2.4%]	1

Binary size

Results (primary -0.1%, secondary 0.0%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	0.0%	[0.0%, 0.0%]	4
Regressions ❌ (secondary)	0.1%	[0.0%, 0.2%]	15
Improvements ✅ (primary)	-0.1%	[-0.1%, -0.0%]	47
Improvements ✅ (secondary)	-0.0%	[-0.1%, -0.0%]	16
All ❌✅ (primary)	-0.1%	[-0.1%, 0.0%]	51

Bootstrap: 490.115s -> 464.348s (-5.26%)
Artifact size: 374.71 MiB -> 374.62 MiB (-0.02%)

rustbot · 2025-07-13T21:38:26Z

r? @compiler-errors

rustbot has assigned @compiler-errors.
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 · 2025-07-13T21:38:28Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri, @RalfJung, @oli-obk, @lcnr

This PR changes Stable MIR

cc @oli-obk, @celinval, @ouz-a

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred in compiler/rustc_codegen_ssa

cc @WaffleLapkin

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

oli-obk · 2025-07-14T07:56:42Z

What's the change that fixed the performance issues?

cjgillot · 2025-07-14T08:37:37Z

No idea. And I'm not sure I want to bisect 2y of changes to find out.

oli-obk · 2025-07-14T08:55:57Z

No I meant 😆 what you changed in your PR

oh wait that perf run was 2 years old, ignore me

oli-obk · 2025-07-14T08:58:50Z

r=me with miri tests blessed, too

RalfJung · 2025-07-14T09:17:23Z

compiler/rustc_middle/src/mir/consts.rs

@@ -170,26 +171,29 @@ impl<'tcx> ConstValue<'tcx> {
                // Non-empty slice, must have memory. We know this is a relative pointer.
                let (inner_prov, offset) =
                    ptr.into_pointer_or_addr().ok()?.prov_and_relative_offset();
-                let data = tcx.global_alloc(inner_prov.alloc_id()).unwrap_memory();
-                (data, offset.bytes(), offset.bytes() + len)
+                (inner_prov.alloc_id(), offset.bytes(), offset.bytes() + len)


Suggested change

(inner_prov.alloc_id(), offset.bytes(), offset.bytes() + len)

(inner_prov.alloc_id(), offset.bytes(), len)

I think? You rename the variable to len but didn't adjust the logic here, it seems.

RalfJung · 2025-07-14T09:18:57Z

tests/mir-opt/building/user_type_annotations.let_else_bindless.built.after.mir

+    0x00 │ 69 6e 74 65 72 6e 61 6c 20 65 72 72 6f 72 3a 20 │ internal error: 
+    0x10 │ 65 6e 74 65 72 65 64 20 75 6e 72 65 61 63 68 61 │ entered unreacha
+    0x20 │ 62 6c 65 20 63 6f 64 65                         │ ble code
+}


Why do we print so many more allocation contents now?

For slices, we just printed the contents. This PR adds their alloc-id to the set of alloc-ids to dump. I find this more consistent, even if a bit verbose.

RalfJung · 2025-07-14T09:20:30Z

tests/ui/mir/gvn-nonsensical-sized-str.rs

This is just fixed by accident, right? There's likely still tons of issues due to nonsensical bounds, we just don't have a reproducer any more...

I have a cleaner fix in #143271 for this ICE and another one. But I agree, this is an accidental fix so meh.

cjgillot · 2025-07-14T09:36:47Z

I'll need some help with the miri test. The failing test verifies that we create a new alloc-id for each allocated object in a function call. It used a string, but with this PR a string now has a single alloc-id ever. Are there other constructs that would recover the proper behavior?

RalfJung · 2025-07-14T09:41:29Z

Cc @saethlin -- is the const alloc dedup machinery you added still needed with this PR?

RalfJung · 2025-07-14T09:44:45Z

Calling const_val_to_op on the same constant many times will now always produce the same result, I think.

We can still get different results across multiple crates, but that won't happen inside Miri.

saethlin · 2025-07-14T12:00:41Z

Cc @saethlin -- is the const alloc dedup machinery you added still needed with this PR?

I don't have time to review this in depth right now, but the upshot is that programs of this form which repeatedly evaluate a constant should not have constantly-growing memory use when run in Miri:

fn main() {
    loop {
        helper();
    }
}

fn helper() {
    "ouch";
}

This is pasted straight from the PR #118336.

To first order, if this program does not have ever-growing memory use in Miri with this PR I say it seems good. If we re-add the memory growth bug we'll just fix it again. It's not the end of the world, we got along just fine for years with the memory growth behavior.

RalfJung · 2025-07-14T13:12:17Z

yeah I think this PR properly fixes that memory growth, and we can now remove the stuff that #118336 added to work around the previous growth.

@cjgillot so for this PR

please remove the const_cache field and the eval_mir_constant machine hook, they should not be needed any more 🎉
please adjust tests/pass/const-addrs.rs to simply assert that it sees the same pointer every time (with a comment saying that this is not guaranteed behavior but reflects current rustc)

bors · 2025-07-14T19:57:05Z

☔ The latest upstream changes (presumably #143934) made this pull request unmergeable. Please resolve the merge conflicts.

rustbot · 2025-07-15T10:28:17Z

The Miri subtree was changed

cc @rust-lang/miri

This PR changes rustc_public

cc @oli-obk, @celinval, @ouz-a

RalfJung · 2025-07-15T12:19:57Z

src/tools/miri/tests/pass/const-addrs.rs

+// The interpreter used to create a new AllocId every time it evaluates any const.
+// This caused unbounded memory use in Miri.
+// This test verifies that we only create a bounded amount of addresses for any given const.
+// In practice, the interpreter always returns the same address, but we *do not guarantee* it.


Suggested change

// In practice, the interpreter always returns the same address, but we *do not guarantee* it.

// In practice, the interpreter always returns the same address, but we *do not guarantee* that.

RalfJung · 2025-07-15T12:20:21Z

src/tools/miri/tests/pass/const-addrs.rs

+// The interpreter used to create a new AllocId every time it evaluates any const.
+// This caused unbounded memory use in Miri.
+// This test verifies that we only create a bounded amount of addresses for any given const.
+// In practice, the interpreter always returns the same address, but we *do not guarantee* it.
 //@compile-flags: -Zinline-mir=no

 const EVALS: usize = 256;


We can make this test quite a bit faster now that it's not random any more.

Suggested change

const EVALS: usize = 256;

const EVALS: usize = 64;

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 Oct 13, 2023

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 Oct 13, 2023

bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 13, 2023

Auto merge of rust-lang#116707 - cjgillot:slice-id, r=<try>

9ef21e1

Create an `AllocId` for `ConstValue::Slice`. r? `@ghost`

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 Oct 14, 2023

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 Oct 14, 2023

bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 14, 2023

Auto merge of rust-lang#116707 - cjgillot:slice-id, r=<try>

eafbd55

Create an `AllocId` for `ConstValue::Slice`. r? `@ghost`

This comment has been minimized.

Sign in to view

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

Dylan-DPC added S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 16, 2024

cjgillot force-pushed the slice-id branch from e195fe8 to 766af91 Compare April 7, 2024 15:01

This comment has been minimized.

Sign in to view

cjgillot force-pushed the slice-id branch from 766af91 to 47bdbd3 Compare July 13, 2025 13:06

rustbot added the T-clippy Relevant to the Clippy team. label Jul 13, 2025

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 13, 2025

cjgillot marked this pull request as ready for review July 13, 2025 21:38

rustbot assigned compiler-errors Jul 13, 2025

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 13, 2025

This comment has been minimized.

Sign in to view

oli-obk assigned oli-obk and unassigned compiler-errors Jul 14, 2025

RalfJung reviewed Jul 14, 2025

View reviewed changes

cjgillot added 5 commits July 15, 2025 10:28

Give an AllocId to ConstValue::Slice.

f6250d2

Remove useless lifetime parameter.

f231d4f

Move crash test.

0f481cc

Fix length computation.

cbbad2d

Remove const deduplication from the interpreter.

620f9ea

cjgillot force-pushed the slice-id branch from d9fba2d to 620f9ea Compare July 15, 2025 10:28

RalfJung reviewed Jul 15, 2025

View reviewed changes

	(inner_prov.alloc_id(), offset.bytes(), offset.bytes() + len)
	(inner_prov.alloc_id(), offset.bytes(), len)

	// In practice, the interpreter always returns the same address, but we do not guarantee it.
	// In practice, the interpreter always returns the same address, but we do not guarantee that.

Create an AllocId for ConstValue::Slice. #116707

Are you sure you want to change the base?

Create an AllocId for ConstValue::Slice. #116707

Conversation

cjgillot commented Oct 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cjgillot commented Oct 13, 2023

Uh oh!

This comment has been minimized.

bors commented Oct 13, 2023

Uh oh!

bors commented Oct 13, 2023

Uh oh!

This comment has been minimized.

RalfJung commented Oct 13, 2023

Uh oh!

rust-timer commented Oct 14, 2023

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Uh oh!

cjgillot commented Oct 14, 2023

Uh oh!

This comment has been minimized.

bors commented Oct 14, 2023

Uh oh!

This comment has been minimized.

bors commented Oct 14, 2023

Uh oh!

This comment has been minimized.

rust-timer commented Oct 14, 2023

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Uh oh!

This comment has been minimized.

cjgillot commented Jul 13, 2025

Uh oh!

rust-timer commented Jul 13, 2025

Overall result: ❌✅ regressions and improvements - please read the text below

Uh oh!

rustbot commented Jul 13, 2025

Uh oh!

rustbot commented Jul 13, 2025

Uh oh!

This comment has been minimized.

oli-obk commented Jul 14, 2025

Uh oh!

cjgillot commented Jul 14, 2025

Uh oh!

oli-obk commented Jul 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oli-obk commented Jul 14, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjgillot commented Jul 14, 2025

Uh oh!

RalfJung commented Jul 14, 2025

Uh oh!

RalfJung commented Jul 14, 2025

Uh oh!

saethlin commented Jul 14, 2025

Uh oh!

RalfJung commented Jul 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bors commented Jul 14, 2025

Uh oh!

rustbot commented Jul 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Create an `AllocId` for `ConstValue::Slice`. #116707

Create an `AllocId` for `ConstValue::Slice`. #116707

cjgillot commented Oct 13, 2023 •

edited

Loading

oli-obk commented Jul 14, 2025 •

edited

Loading

RalfJung commented Jul 14, 2025 •

edited

Loading