rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
98.76k stars 12.76k forks source link

Rust doesn't use niche in reference (or pointer) to slice #132235

Open VorfeedCanal opened 3 weeks ago

VorfeedCanal commented 3 weeks ago

From reddit thread: why can't rust optimize the size of Option<Option<&str>>?

Indeed, in spite of the fact that &str doesn't all use more than half of possible bit sequences (top bit of 2nd word is always zero, plus first word can not be zero) only Option<&str> is optimized

Would it make sense to teach compiler about that niche or is it prevented by some deeper issue?

workingjubilee commented 3 weeks ago

Duplicate of https://github.com/rust-lang/rust/issues/119507

the8472 commented 3 weeks ago

I don't think that's correct. #119507 is about using niches across fields. This one is about having niches in slice lengths.

And I've attempted to improve that in #116542 but so far perf hasn't been looking good. Maybe because the IR gets more complicated.

workingjubilee commented 3 weeks ago

@the8472 Isn't it a duplicate of https://github.com/rust-lang/rust/issues/125363 then?

workingjubilee commented 3 weeks ago

In any case, I don't really think having a zillion tiny issues regarding the layout code, which is a big ball of mud, is very helpful. It would be better to collect the various possible optimizations and reason about them in a central manner, rather than trying to make tiny adjustments to spot-fix the code.

the8472 commented 3 weeks ago

125363 is similar to #119507 actually.

And having a "more niche optimizations" super issue would help. But some of the sub-issues are still useful because some specific cases can likely be fixed without difficult perf tradeoffs while others are less certain.

workingjubilee commented 3 weeks ago

Honestly, I'm not sure why your PR #116542 is seemingly both trying to add metadata and alter the layout of slices.

workingjubilee commented 3 weeks ago

The fundamental issue is that all of the issues we're discussing are trying to do the same thing: "There are two niches, one in each of two separate parts of this. Optimize the layout based on that."

In that regard, a struct and a slice should not be treated significantly differently.

the8472 commented 3 weeks ago

This issue isn't about jointly using two pre-existing niches. It's about adding a new set of niches to the length field in slice-refs, by exploiting the isize::MAX bytes upper limit, something we're currently not doing.

Honestly, I'm not sure why your PR https://github.com/rust-lang/rust/pull/116542 is seemingly both trying to add metadata and alter the layout of slices.

They're based on the same thing, derived from the validity ranges of the fields.

workingjubilee commented 3 weeks ago

They may be based on the same thing, but that doesn't mean the changes are functionally dependent in the implementation.

workingjubilee commented 3 weeks ago

...Unless they are, in which case that segment of the layout code is possibly in even more dire need of being scrapped and rewritten than I thought.

the8472 commented 3 weeks ago

I mean if we define the slice length field validity range - which currently isn't explicitly set and so defaults to the same validity as usize - then both metadata and niches fall out of that automatically. Treating them differently for the purpose of llvmir metadata and niche calculation would take extra effort.

workingjubilee commented 3 weeks ago

I suppose I'll reopen this then, but now I'm wondering if we can convince the layout code that &[T] is secretly a funny-looking struct.

the8472 commented 3 weeks ago

I think layout code should already be seeing an univariant ADT with two fields and scalarpair abi. My PR does result in layout changes.

Vrtgs commented 3 weeks ago

I suppose I'll reopen this then, but now I'm wondering if we can convince the layout code that &[T] is secretly a funny-looking struct.

The issue with this is that when T is a zst, it is now okay for a &[T] to have more than isize::MAX elements

Also isize::MAX is an even bigger range than the null optimization, so I think it should be favored instead of the null optimization if only one niche optimization can be applied