-
Notifications
You must be signed in to change notification settings - Fork 13.3k
transmutability: uninit transition matches unit byte only #140380
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
} | ||
|
||
#[inline] | ||
fn contains_uninit(&self) -> bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
?
None => write!(f, "uninit"), | ||
Some((lo, hi)) => write!(f, "{lo}..={hi}"), | ||
} | ||
write!(f, "{}..{}", self.start, self.end) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO it'd still be helpful to have uninit be printed as a special string (e.g. "uninit") rather than as 256. Seeing 0..257
would be confusing to anyone who isn't intimately familiar with this implementation. Maybe it could instead be printed as 0..256|uninit
or something similar?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tmiasko Can you do this one too? Happy to merge after that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added an implementation. Although, it is unclear to me whether it is an improvement.
// FIXME(@joshlf): Make `State` a `NonZero` so that this is NPO'd. | ||
uninit: Option<S>, | ||
// Runs are non-empty, non-overlapping, and stored in ascending order. | ||
runs: SmallVec<[(Byte, S); 1]>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you investigate the performance implication of this change? I worry that, for large types, storing runs as pairs of u16
s rather than as pairs of u8
s may degrade performance by worsening cache locality.
If that's problematic, another option would be to store u8
s and have some nonsense sentinal value be used to represent uninit (e.g. (start: 1, end: 0)
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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]>();
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
None => Self { runs: SmallVec::new(), uninit: Some(dst) }, | ||
pub(crate) fn new(range: Byte, dst: S) -> Self { | ||
let mut this = Self { runs: SmallVec::new() }; | ||
if !range.is_empty() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could also add the invariant to Byte
that it never stores an empty range, and then we could avoid this check.
Following up on this thread:
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. |
f24d315
to
d3ca772
Compare
For an uninit transition in the source, the previous implementation returned
With assumed validity that immediately terminates the computation with answer |
Gotcha that makes sense. |
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.
@bors r+ |
transmutability: uninit transition matches unit byte only 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 rust-lang#140337. Fixes rust-lang#140168 (comment). r? `@jswrenn` `@joshlf`
Rollup of 14 pull requests Successful merges: - rust-lang#140380 (transmutability: uninit transition matches unit byte only) - rust-lang#140385 (Subtree update of `rust-analyzer`) - rust-lang#140395 (organize and extend forbidden target feature tests) - rust-lang#140430 (Improve test coverage of HIR pretty printing.) - rust-lang#140458 (Fix for async drop ice with partly dropped tuple) - rust-lang#140460 (Fix handling of LoongArch target features not supported by LLVM 19) - rust-lang#140465 (chore: edit and move tests) - rust-lang#140467 (Don't FCW assoc consts in patterns) - rust-lang#140468 (Minor tweaks to make some normalization (adjacent) code less confusing) - rust-lang#140470 (CI: rfl: move job forward to Linux v6.15-rc4) - rust-lang#140476 (chore: delete unused ui/auxiliary crates) - rust-lang#140481 (Require sanitizers be enabled for asan_odr_windows.rs) - rust-lang#140486 (rustfmt: Also allow bool literals as first item of let chain) - rust-lang#140494 (Parser: Document restrictions) r? `@ghost` `@rustbot` modify labels: rollup
transmutability: uninit transition matches unit byte only 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 rust-lang#140337. Fixes rust-lang#140168 (comment). r? ``@jswrenn`` ``@joshlf``
Rollup of 13 pull requests Successful merges: - rust-lang#140380 (transmutability: uninit transition matches unit byte only) - rust-lang#140385 (Subtree update of `rust-analyzer`) - rust-lang#140395 (organize and extend forbidden target feature tests) - rust-lang#140458 (Fix for async drop ice with partly dropped tuple) - rust-lang#140460 (Fix handling of LoongArch target features not supported by LLVM 19) - rust-lang#140465 (chore: edit and move tests) - rust-lang#140467 (Don't FCW assoc consts in patterns) - rust-lang#140468 (Minor tweaks to make some normalization (adjacent) code less confusing) - rust-lang#140470 (CI: rfl: move job forward to Linux v6.15-rc4) - rust-lang#140476 (chore: delete unused ui/auxiliary crates) - rust-lang#140481 (Require sanitizers be enabled for asan_odr_windows.rs) - rust-lang#140486 (rustfmt: Also allow bool literals as first item of let chain) - rust-lang#140494 (Parser: Document restrictions) r? `@ghost` `@rustbot` modify labels: rollup
…renn transmutability: ensure_sufficient_stack when answering query Based on rust-lang#140380. Fixes rust-lang#118860. The compile time was addressed earlier, this merely addresses stack overflow part of the issue. r? `@jswrenn` `@joshlf`
transmutability: uninit transition matches unit byte only 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 rust-lang#140337. Fixes rust-lang#140168 (comment). r? ```@jswrenn``` ```@joshlf```
Rollup merge of rust-lang#140504 - tmiasko:answer-ensure-stack, r=jswrenn transmutability: ensure_sufficient_stack when answering query Based on rust-lang#140380. Fixes rust-lang#118860. The compile time was addressed earlier, this merely addresses stack overflow part of the issue. r? `@jswrenn` `@joshlf`
…iaskrgr Rollup of 11 pull requests Successful merges: - rust-lang#140380 (transmutability: uninit transition matches unit byte only) - rust-lang#140385 (Subtree update of `rust-analyzer`) - rust-lang#140458 (Fix for async drop ice with partly dropped tuple) - rust-lang#140465 (chore: edit and move tests) - rust-lang#140467 (Don't FCW assoc consts in patterns) - rust-lang#140468 (Minor tweaks to make some normalization (adjacent) code less confusing) - rust-lang#140470 (CI: rfl: move job forward to Linux v6.15-rc4) - rust-lang#140476 (chore: delete unused ui/auxiliary crates) - rust-lang#140481 (Require sanitizers be enabled for asan_odr_windows.rs) - rust-lang#140486 (rustfmt: Also allow bool literals as first item of let chain) - rust-lang#140494 (Parser: Document restrictions) r? `@ghost` `@rustbot` modify labels: rollup
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 validfor 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