adjust ConstValue::Slice to work for arbitrary slice types #115870

RalfJung · 2023-09-15T14:25:04Z

valtrees have already been assuming that this works; this PR makes it a reality. Also further restrict ConstValue::Slice to what it is actually used for; this even shrinks ConstValue from 32 to 24 bytes which is a nice win. :)

The alternative to this approach is to make ConstValue::Slice work really only for &str/&[u8] literals, and never return it in op_to_const. That would make op_to_const very clean. We could then even remove the meta field; the length would always be data.inner().len(). We could almost just use a Symbol instead of a ConstAllocation, but we have to support byte strings and there doesn't seem to be an interned representation of them (or rather, ConstAllocation is their interned representation). In this world, valtrees of slice reference types would then become noticeably more expensive to turn into a ConstValue -- but does that matter? Specifically for &str/&[u8] we could still use the optimized representation if we wanted.

If byte strings were already interned somewhere I'd gravitate towards the alternative, but the way things stand, we need a ConstAllocation case anyway to support byte strings, and then we might as well support arbitrary slices. (Or we say that byte strings don't get an optimized representation at all. Such a performance cliff between str and byte strings is probably unexpected, though due to the lack of interning for byte strings I think there might already be a performance cliff there.)

rustbot · 2023-09-15T14:25:12Z

r? @davidtwco

(rustbot has picked a reviewer for you, use r? to override)

rustbot · 2023-09-15T14:25:16Z

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

This PR changes Stable MIR

cc @oli-obk, @celinval, @spastorino

The Miri subtree was changed

cc @rust-lang/miri

RalfJung · 2023-09-15T14:25:20Z

r? @oli-obk

RalfJung · 2023-09-15T14:25:58Z

compiler/rustc_codegen_cranelift/src/constant.rs

-                .iconst(fx.pointer_type, i64::try_from(end.checked_sub(start).unwrap()).unwrap());
+            let ptr = pointer_for_allocation(fx, alloc_id).get_addr(fx);
+            // FIXME: the `try_from` here can actually fail, e.g. for very long ZST slices.
+            let len = fx.bcx.ins().iconst(fx.pointer_type, i64::try_from(meta).unwrap());


@bjorn3 how does one turn an arbitrary "target usize" into a clif value? Going via i64 could overflow...

Fixed this in bjorn3/rustc_codegen_cranelift@cb55ce1

RalfJung · 2023-09-15T14:28:21Z

I wonder if the size reduction gives us a measurable speedup.
@bors try @rust-timer queue

bors · 2023-09-15T14:28:31Z

⌛ Trying commit e989ec5 with merge 06e5097...

adjust constValue::Slice to work for arbitrary slice types valtrees have already been assuming that this works; this PR makes it a reality. Also further restrict `ConstValue::Slice` to what it is actually used for; this even shrinks `ConstValue` from 32 to 24 bytes which is a nice win. :) The alternative to this approach is to make `ConstValue::Slice` work really only for `&str`/`&[u8]` literals, and never return it in `op_to_const`. That would make `op_to_const` very clean. We could then even remove the `meta` field; the length would always be `data.inner().len()`. We could *almost* just use a `Symbol` instead of a `ConstAllocation`, but we have to support byte strings and there doesn't seem to be an interned representation of them (or rather, `ConstAllocation` *is* their interned representation). In this world, valtrees of slice reference types would then become noticeably more expensive to turn into a `ConstValue` -- but does that matter? Specifically for `&str`/`&[u8]` we could still use the optimized representation if we wanted. If byte strings were already interned somewhere I'd gravitate towards the alternative, but the way things stand, we need a `ConstAllocation` case anyway to support byte strings, and then we might as well support arbitrary slices. (Or we say that byte strings don't get an optimized representation at all. Such a performance cliff between `str` and byte strings is probably unexpected, though due to the lack of interning for byte strings I think there might already be a performance cliff there.)

bors · 2023-09-15T15:37:52Z

☀️ Try build successful - checks-actions
Build commit: 06e5097 (06e50979de2aa96e49ce7185493e9dd6314ba866)

rust-timer · 2023-09-15T16:59:13Z

Finished benchmarking commit (06e5097): comparison URL.

Overall result: ✅ improvements - no 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.

@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
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.0%	[-1.0%, -1.0%]	2
All ❌✅ (primary)	-	-	0

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)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.0%	[-2.0%, -2.0%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-2.0%	[-2.0%, -2.0%]	1

Cycles

This benchmark run did not return any relevant results for this metric.

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
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.0%	[-0.0%, -0.0%]	17
Improvements ✅ (secondary)	-0.0%	[-0.0%, -0.0%]	6
All ❌✅ (primary)	-0.0%	[-0.0%, -0.0%]	17

Bootstrap: 633.484s -> 630.971s (-0.40%)
Artifact size: 318.18 MiB -> 318.13 MiB (-0.02%)

oli-obk · 2023-09-18T09:54:12Z

compiler/rustc_middle/src/mir/interpret/value.rs

+        data: ConstAllocation<'tcx>,
+        /// The metadata field of the reference.
+        /// This is a "target usize", so we use `u64` as in the interpreter.
+        meta: u64,


do we even need this anymore? It can be computed from the ConstAllocation's size and the element type from the constant's type

This variant is only interesting for MIR building in check builds. For codegen builds we'll end-up creating the AllocId anyway.
Meawhile, we still have an asymmetry between the variants of ConstValue and the variants we have in interpreter and codegen (Uninit, Scalar, ScalarPair, Indirect).

What about:

making this a variant in mir::ConstantKind instead? a ConstantKind::ByteSlice(&'tcx [u8])?

introducing a ScalarPair variant in ConstValue? Example in WIP Introduce ConstValue::ScalarPair #115915 for the latter.

The "initial" representation by MIR building would be a ConstantKind::ByteSlice(&[u8]), and would get normalized to a ScalarPair for/before codegen.

It can be computed from the ConstAllocation's size and the element type from the constant's type

Not for ZST slices...

The "initial" representation by MIR building would be a ConstantKind::ByteSlice(&[u8]), and would get normalized to a ScalarPair for/before codegen.

I would expect that to have the same perf impact as normalizing to a ScalarPair during MIR building (or even worse if it's done post-mono). I tried that (by putting an AllocId into ConstValue::Slice), it didn't end well. So I think this is not going to be fast enough.

Meawhile, we still have an asymmetry between the variants of ConstValue and the variants we have in interpreter and codegen (Uninit, Scalar, ScalarPair, Indirect).

Yeah that is totally deliberate. We used to have ConstValue::ScalarPair and moved away from it. I think moving back towards ScalarPair would be a mistake. We have no reason to permit an optimized representation of &dyn Trait or (i8, bool) or whatever, as fully evaluated constants.

You are pre-supposing that we want to have symmetry between ConstValue and OpTy, but that's not the case; those types serve very different roles with different trade-offs.

I think long-term we want to remove ConstValue entirely. See #115877.

compiler/rustc_const_eval/src/const_eval/eval_queries.rs

rustbot · 2023-09-18T11:37:11Z

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

This PR changes Stable MIR

cc @oli-obk, @celinval, @spastorino

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

bors · 2023-09-19T16:17:05Z

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

oli-obk · 2023-09-20T09:53:22Z

r=me modulo the cranelift question

RalfJung · 2023-09-20T11:50:33Z

The cranelift question is pre-existing though, the old code already did i64::try_from(end_minus_start).unwrap(). So this PR doesn't make things any worse, and I think we can leave the FIXME for @bjorn3 to resolve one day.

oli-obk · 2023-09-20T17:52:04Z

@bors r+

bors · 2023-09-20T17:52:06Z

📌 Commit ea22adb has been approved by oli-obk

It is now in the queue for this repository.

bors · 2023-09-20T18:06:52Z

⌛ Testing commit ea22adb with merge 9da3e81...

bors · 2023-09-20T19:55:29Z

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 9da3e81 to master...

rust-timer · 2023-09-20T21:52:55Z

Finished benchmarking commit (9da3e81): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -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
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.4%	[-0.5%, -0.2%]	4
All ❌✅ (primary)	-	-	0

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)	2.6%	[2.6%, 2.6%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-5.1%	[-5.1%, -5.1%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-1.2%	[-5.1%, 2.6%]	2

Cycles

This benchmark run did not return any relevant results for this metric.

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
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.0%	[-0.0%, -0.0%]	18
Improvements ✅ (secondary)	-0.0%	[-0.0%, -0.0%]	6
All ❌✅ (primary)	-0.0%	[-0.0%, -0.0%]	18

Bootstrap: 632.239s -> 633.382s (0.18%)
Artifact size: 317.84 MiB -> 317.82 MiB (-0.01%)

rustbot assigned davidtwco Sep 15, 2023

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 Sep 15, 2023

rustbot assigned oli-obk and unassigned davidtwco Sep 15, 2023

RalfJung commented Sep 15, 2023

View reviewed changes

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 Sep 15, 2023

RalfJung changed the title ~~adjust constValue::Slice to work for arbitrary slice types~~ adjust ConstValue::Slice to work for arbitrary slice types Sep 15, 2023

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 Sep 15, 2023

oli-obk reviewed Sep 18, 2023

View reviewed changes

compiler/rustc_const_eval/src/const_eval/eval_queries.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

RalfJung force-pushed the const-value-slice branch from 6e21222 to 995233f Compare September 18, 2023 11:50

adjust constValue::Slice to work for arbitrary slice types

ea22adb

RalfJung force-pushed the const-value-slice branch from 995233f to ea22adb Compare September 19, 2023 18:18

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 Sep 20, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 20, 2023

bors merged commit 9da3e81 into rust-lang:master Sep 20, 2023

rustbot added this to the 1.74.0 milestone Sep 20, 2023

RalfJung deleted the const-value-slice branch September 21, 2023 10:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adjust ConstValue::Slice to work for arbitrary slice types #115870

adjust ConstValue::Slice to work for arbitrary slice types #115870

RalfJung commented Sep 15, 2023

rustbot commented Sep 15, 2023

rustbot commented Sep 15, 2023

RalfJung commented Sep 15, 2023

RalfJung Sep 15, 2023

bjorn3 Oct 2, 2023

RalfJung commented Sep 15, 2023

This comment has been minimized.

bors commented Sep 15, 2023

bors commented Sep 15, 2023

This comment has been minimized.

rust-timer commented Sep 15, 2023

oli-obk Sep 18, 2023

cjgillot Sep 18, 2023

RalfJung Sep 18, 2023

RalfJung Sep 18, 2023 •

edited

Loading

RalfJung Sep 18, 2023 •

edited

Loading

rustbot commented Sep 18, 2023

This comment has been minimized.

bors commented Sep 19, 2023

oli-obk commented Sep 20, 2023

RalfJung commented Sep 20, 2023

oli-obk commented Sep 20, 2023

bors commented Sep 20, 2023

bors commented Sep 20, 2023

bors commented Sep 20, 2023

rust-timer commented Sep 20, 2023

adjust ConstValue::Slice to work for arbitrary slice types #115870

adjust ConstValue::Slice to work for arbitrary slice types #115870

Conversation

RalfJung commented Sep 15, 2023

rustbot commented Sep 15, 2023

rustbot commented Sep 15, 2023

RalfJung commented Sep 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung commented Sep 15, 2023

This comment has been minimized.

bors commented Sep 15, 2023

bors commented Sep 15, 2023

This comment has been minimized.

rust-timer commented Sep 15, 2023

Overall result: ✅ improvements - no action needed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung Sep 18, 2023 • edited Loading

Choose a reason for hiding this comment

RalfJung Sep 18, 2023 • edited Loading

Choose a reason for hiding this comment

rustbot commented Sep 18, 2023

This comment has been minimized.

bors commented Sep 19, 2023

oli-obk commented Sep 20, 2023

RalfJung commented Sep 20, 2023

oli-obk commented Sep 20, 2023

bors commented Sep 20, 2023

bors commented Sep 20, 2023

bors commented Sep 20, 2023

rust-timer commented Sep 20, 2023

Overall result: ✅ improvements - no action needed

RalfJung Sep 18, 2023 •

edited

Loading

RalfJung Sep 18, 2023 •

edited

Loading