Experiment: Make intrinsic::size_of(slice) saturate

[CAD97]

Experiment to see how much of a perf impact this is.

Currently, size_of_val will wrap for a slice that is larger than usize::MAX. Constructing a pointer to such is stably safe via ptr::slice_from_raw_parts[_mut].

I would like it to be possible to have a safe/checked version of size_of_val_raw, which returns roughly Option<#[rustc_valid_scalar_range_start(0)] isize>. The simple way to do so is to have intrinsic::size_of saturate, and then the safe wrapper can check for > isize::MAX as the error condition.

This PR is to check how expensive it would be to just always saturate the computation of a slice's size.

This PR does not address any of the other kinds of unsized types. Aggregate types will still potentially overflow, and it is still (AIUI) undecided whether the vtable part of pointer-to-dyn-trait is required to point to a valid vtable.

[rust-highfive]

r? @fee1-dead

(rust-highfive has picked a reviewer for you, use r? to override)

[CAD97]

Well I wrote that wrong and I don't know how to write it properly, especially given it compiles on my machine. I'll need to ask for help here 😅

[fee1-dead]

r? @nikomatsakis for reassignment/guidance

[CAD97]

At a micro/assembly level, this changes getting the size of a slice from imul to mul, mov, cmovo.. The question is how impactful this actually is...

[nikomatsakis]

Hmm, I'm not sure what the problem is tbh -- seems like an LLVM problem.

r? @bjorn3 -- I'm guessing you might have insight, or have a good idea who would!

[bjorn3]

@CAD97

Constructing a pointer to such is stably safe via ptr::slice_from_raw_parts[_mut].

Maybe add an assert there instead. Alternatively we can say that given that the pointer part of the fat pointer returned by ptr::slice_from_raw_parts doesn't have to be valid, the slice length doesn't have to make sense either. I don't think this can cause unsoundness. For references we don't have this problem anyway as you can't have a value larger than isize::MAX to take a reference of in the first place.

@nikomatsakis

Hmm, I'm not sure what the problem is tbh -- seems like an LLVM problem.

This is not an LLVM problem. size_of_val on a slice multiplies the element size with the slice length. This multiplication can overflow in which case the resulting stated length is incorrect.

[CAD97]

There's no unsoundness problem currently; we say that construction of the pointer is safe, and using it is not. This is reasonable, and fits the normal raw pointer semantics.

The "problem" is a theoretical one: that size_of_val_raw is unsafe and can overflow. It would be nice to have a version of size_of_val_raw which can be guaranteed to saturate; currently, there's no way to actually check that calling size_of_val_raw will not overflow.

(Any soundness issue is resulting from using size_of_val_raw in a way which is currently documented as disallowed.)

This is, then, an attempt at an experiment to see the cost of making size_of_val_raw saturate. Perhaps the safe interface should be a new lang item, try_size_of_val or similar, but it would be nice if the normal way would just be the sound way; the purpose of this experiment is to check the cost of such.

To be clear: I don't really expect this to merge, there's no need for it to merge, and there's no immediate benefit to this merging.

Also, the original motivation for this PR (doing allocation work over pointee metadata) has disappeared (it's using Layout instead). This is then just more of a "what if?" than anything really meaningful in the short term.

Again, to reiterate: the goal is to enable std to eventually provide something like

#[rustc_valid_scalar_range_start(0)]
struct size(isize);
/*safe*/ fn try_size_of_val_raw<T>(*const T) -> Option<size>;

This PR's point is to examine the cost of making this just size_of_val_raw, rather than a new function.

(But, depending on how extern type is treated, it might need to be a separate function anyway. size_of_val_raw should always match size_of_val's behavior, but try_size_of_val_raw should maybe probably return None for extern types with unknown size.)

[bjorn3]

I don't think I can make a good decision on this.

r? rust-lang/compiler

[michaelwoerister]

@CAD97, can you fix the PR so that we can do a perf run?

[CAD97]

Ok, I think I fixed the build errors, so this should be perf run capable now.

[CAD97]

Well nevermind, take two 😅

[michaelwoerister]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

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

[bors]

⌛ Trying commit ed3502f with merge 2410a27108a1d1203807aa16f689a9df484b43f1...

[bors]

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

[rust-timer]

Queued 2410a27108a1d1203807aa16f689a9df484b43f1 with parent 7417110, future comparison URL.

[rust-timer]

Finished benchmarking commit (2410a27108a1d1203807aa16f689a9df484b43f1): comparison url.

Summary:

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

	Regressions 😿 (primary)	Regressions 😿 (secondary)	Improvements 🎉 (primary)	Improvements 🎉 (secondary)	All 😿 🎉 (primary)
count1	1	0	0	0	1
mean2	0.6%	N/A	N/A	N/A	0.6%
max	0.6%	N/A	N/A	N/A	0.6%

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

Footnotes

1. number of relevant changes ↩

2. the arithmetic mean of the percent change ↩

[CAD97]

This answers the question at hand, and the original motivation for this experiment has faded, so I'm going to go ahead and just close this.

I still think that a safe way to go from pointee metadata to Option<Layout> is desirable, but I'm not going to push for it in the short term.