transmutability: uninit transition matches unit byte only

[tmiasko]

The previous implementation was inconsistent about transitions that
apply for an init byte. For example, when answering a query, an init
byte could use corresponding init transition. Init byte could also use
uninit transition, but only when the corresponding init transition was
absent. This behaviour was incompatible with DFA union construction.

Define an uninit transition to match an uninit byte only and update
implementation accordingly. To describe that Tree::uninit is valid
for any value, build an automaton that accepts any byte value.

Additionally, represent byte ranges uniformly as a pair of integers to
avoid special case for uninit byte.

Fixes #140337.
Fixes #140168 (comment).

r? @jswrenn @joshlf

[joshlf]

Thanks so much for doing this! I am curious to see how this affects performance, but I really like how much simpler a lot of the code is.

[joshlf]

If you end up going the route of requiring that uninit implies that all byte values are set, then maybe rename this to is_uninit?

[joshlf]

Can you investigate the performance implication of this change? I worry that, for large types, storing runs as pairs of u16s rather than as pairs of u8s may degrade performance by worsening cache locality.

If that's problematic, another option would be to store u8s and have some nonsense sentinal value be used to represent uninit (e.g. (start: 1, end: 0)).

[tmiasko]

I didn't look at performance at all. I think we should start by adding a perf
rust.lang.org benchmark to track progress.

That being said, I think there are other aspects of the implementation that
make transmutability query exponential in the input size, so that would be
something to tackle first.

[joshlf]

That being said, I think there are other aspects of the implementation that
make transmutability query exponential in the input size, so that would be
something to tackle first.

Can you give examples? I'm not aware of any aspect of the current algorithm that is exponential in its input.

[tmiasko]

I am referring to behaviour of conditional answers. They tend to quickly grow
in size. It certainly doesn't help that answers don't use structural sharing.
The existing implementation is quickly running out of memory in cases with
dozen of states. The complexity characterization might be an exaggeration :-).

#![feature(transmutability)]
use std::mem::{Assume, TransmuteFrom};
pub fn is_maybe_transmutable<Src, Dst>()
where
    Dst: TransmuteFrom<Src, { Assume::SAFETY }>
{}

pub union U {
    pub a: &'static u8,
    pub b: &'static u16,
    pub c: &'static u32,
    pub d: &'static u64,
}

fn main() {
    is_maybe_transmutable::<[U; 10], [U; 10]>();
}

[joshlf]

Ooooh okay yeah that is not good. One thing we're eventually hoping to do is to optimize the array representation so that arrays are represented in the DFA as a DFA tagged with a length rather than N copies of the DFA stitched together back-to-back. This is tricky because there are a lot of edge cases (e.g., what happens when the source and destination type have arrays which partially overlap?). That said, the problem you've identified here would be the same if you just had a struct with 10 U fields, so there are other issues to address. Completely running out of memory certainly makes me suspect that there's something exponential(ish) going on here.

I think we should start by adding a perf rust.lang.org benchmark to track progress.

Do you know how to do that?

[tmiasko]

https://github.com/rust-lang/rustc-perf/tree/master/collector/compile-benchmarks#how-to-updateaddremove-benchmarks

[joshlf]

Talked to @jswrenn about this. We're going to hold off for the time being since this would make the performance triage more painful for what isn't a prime-time feature yet. Once we're closer to stabilizing we can revisit this.

[tmiasko]

I opened #140610 to track the performance issue mentioned earlier.

[joshlf]

We could also add the invariant to Byte that it never stores an empty range, and then we could avoid this check.

[joshlf]

Following up on this thread:

Previously, when assuming validity, query nevertheless ensured that there exists a value that is valid for both src and dst:

#![feature(transmutability)]
use std::mem::{Assume, TransmuteFrom};

pub fn is_transmutable<Src, Dst>()
where
    Dst: TransmuteFrom<Src, { Assume::VALIDITY }>,
{
}

pub union MaybeUninitByte {
    pub uninit: (),
    pub byte: u8,
}

fn main() {
    is_transmutable::<MaybeUninitByte, [u8; 2]>(); // previously rejected, now accepted
}

---

I'd expect this to hit the Answer::No(Reason::DstIsTooBig) branch here:

rust/compiler/rustc_transmute/src/maybe_transmutable/mod.rs

Lines 180 to 187 in 4326a44

	} else if src_state == self.src.accept {
	// extension: `size_of(Src) <= size_of(Dst)`
	if let Some(dst_state_prime) = self.dst.get_uninit_edge_dst(dst_state) {
	self.answer_memo(cache, src_state, dst_state_prime)
	} else {
	Answer::No(Reason::DstIsTooBig)
	}
	} else {

...but that's clearly not happening. cc @joshlf

This PR fixes this bug, but I don't understand why. Can you articulate what the bug was, and how this PR fixes it? I generally feel comfortable with this PR, but I would feel more comfortable if I understood what the bug was and why this is a correct fix for it.

[tmiasko]

This PR fixes this bug, but I don't understand why. Can you articulate what the bug was, and how this PR fixes it?

For an uninit transition in the source, the previous implementation returned Answer::Yes:

rust/compiler/rustc_transmute/src/maybe_transmutable/mod.rs

Line 247 in 4326a44

return Either::Left(iter::once(Answer::Yes).chain(answer_iter));

With assumed validity that immediately terminates the computation with answer Answer::Yes.

[joshlf]

This PR fixes this bug, but I don't understand why. Can you articulate what the bug was, and how this PR fixes it?

For an uninit transition in the source, the previous implementation returned Answer::Yes:

rust/compiler/rustc_transmute/src/maybe_transmutable/mod.rs

Line 247 in 4326a44

return Either::Left(iter::once(Answer::Yes).chain(answer_iter));

With assumed validity that immediately terminates the computation with answer Answer::Yes.

Gotcha that makes sense.

[jswrenn]

@bors r+

[bors]

📌 Commit 88a8679 has been approved by jswrenn

It is now in the queue for this repository.