Skip to content

Commit

Permalink
Auto merge of rust-lang#118796 - Nadrieril:fix-exponential-id-match-2…
Browse files Browse the repository at this point in the history 
…, r=<try>

Exhaustiveness: Improve performance on wide matches

rust-lang#118437 revealed an exponential case in exhaustiveness checking. While [exponential cases are unavoidable](https://compilercrim.es/rust-np/), this one only showed up after my rust-lang#117611 rewrite of the algorithm. I remember anticipating a case like this and dismissing it as unrealistic, but here we are :').

The tricky match is as follows:
```rust
match command {
    BaseCommand { field01: true, .. } => {}
    BaseCommand { field02: true, .. } => {}
    BaseCommand { field03: true, .. } => {}
    BaseCommand { field04: true, .. } => {}
    BaseCommand { field05: true, .. } => {}
    BaseCommand { field06: true, .. } => {}
    BaseCommand { field07: true, .. } => {}
    BaseCommand { field08: true, .. } => {}
    BaseCommand { field09: true, .. } => {}
    BaseCommand { field10: true, .. } => {}
    // ...20 more of the same

    _ => {}
}
```

To fix this, this PR formalizes a concept of "relevancy" (naming is hard) that was already used to decide what patterns to report. Now we track it for every row, which in wide matches like the above can drastically cut on the number of cases we explore. After this fix, the above match is checked with linear-many cases instead of exponentially-many.

Fixes rust-lang#118437

r? `@cjgillot`
  • Loading branch information
@bors
bors committed Dec 10, 2023
2 parents c1a3919 + 13a8e36 commit 4abe247
Show file tree
Hide file tree
Showing 2 changed files with 187 additions and 29 deletions.
144 changes: 115 additions & 29 deletions compiler/rustc_mir_build/src/thir/pattern/usefulness.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,74 @@
//!
//!
//!
//! # `Missing` and relevant constructors
//!
//! Take the following example:
//!
//! ```compile_fail,E0004
//! enum Direction { North, South, East, West }
//! # let wind = (Direction::North, 0u8);
//! match wind {
//! (Direction::North, _) => {} // arm 1
//! (_, 50..) => {} // arm 2
//! }
//! ```
//!
//! Remember that we represent the "everything else" cases with [`Constructor::Missing`]. When we
//! specialize with `Missing` in the first column, we have one arm left:
//!
//! ```ignore(partial code)
//! (50..) => {} // arm 2
//! ```
//!
//! We then conclude that arm 2 is useful, and that the match is non-exhaustive with witness
//! `(Missing, 0..50)` (which we would display to the user as `(_, 0..50)`).
//!
//! When we then specialize with `North`, we have two arms left:
//!
//! ```ignore(partial code)
//! (_) => {} // arm 1
//! (50..) => {} // arm 2
//! ```
//!
//! Because `Missing` only matches wildcard rows, specializing with `Missing` is guaranteed to
//! result in a subset of the rows obtained from specializing with anything else. This means that
//! any row with a wildcard found useful when specializing with anything else would also be found
//! useful in the `Missing` case. In our example, after specializing with `North` here we will not
//! gain new information regarding the usefulness of arm 2 or of the fake wildcard row used for
//! exhaustiveness. This allows us to skip cases.
//!
//! When specializing, if there is a `Missing` case we call the other constructors "irrelevant".
//! When there is no `Missing` case there are no irrelevant constructors.
//!
//! What happens then is: when we specialize a wildcard with an irrelevant constructor, we know we
//! won't get new info for this row; we consider that row "irrelevant". Whenever all the rows are
//! found irrelevant, we can safely skip the case entirely.
//!
//! In the example above, we will entirely skip the `(North, 50..)` case. This skipping was
//! developped as a solution to #118437. It doesn't look like much but it can save us from
//! exponential blowup.
//!
//! There's a subtlety regarding exhaustiveness: while this shortcutting doesn't affect correctness,
//! it can affect which witnesses are reported. For example, in the following:
//!
//! ```compile_fail,E0004
//! # let foo = (true, true, true);
//! match foo {
//! (true, _, true) => {}
//! (_, true, _) => {}
//! }
//! ```
//!
//! In this example we will skip the `(true, true, _)` case entirely. Thus `(true, true, false)`
//! will not be reported as missing. In fact we go further than this: we deliberately do not report
//! any cases that are irrelevant for the fake wildcard row. For example, in `match ... { (true,
//! true) => {} }` we will not report `(true, false)` as missing. This was a deliberate choice made
//! early in the development of rust; it so happens that it is beneficial for performance reasons
//! too.
//!
//!
//!
//! # Or-patterns
//!
//! What we have described so far works well if there are no or-patterns. To handle them, if the
Expand Down Expand Up @@ -710,11 +778,15 @@ impl fmt::Display for ValidityConstraint {
struct PatStack<'p, 'tcx> {
// Rows of len 1 are very common, which is why `SmallVec[_; 2]` works well.
pats: SmallVec<[&'p DeconstructedPat<'p, 'tcx>; 2]>,
/// Sometimes we know that as far as this row is concerned, the current case is already handled
/// by a different, more general, case. When all rows are irrelevant this allows us to skip many
/// branches. This is purely an optimization. See at the top for details.
relevant: bool,
}

impl<'p, 'tcx> PatStack<'p, 'tcx> {
fn from_pattern(pat: &'p DeconstructedPat<'p, 'tcx>) -> Self {
PatStack { pats: smallvec![pat] }
PatStack { pats: smallvec![pat], relevant: true }
}

fn is_empty(&self) -> bool {
Expand Down Expand Up @@ -749,12 +821,17 @@ impl<'p, 'tcx> PatStack<'p, 'tcx> {
&self,
pcx: &PatCtxt<'_, 'p, 'tcx>,
ctor: &Constructor<'tcx>,
ctor_is_relevant: bool,
) -> PatStack<'p, 'tcx> {
// We pop the head pattern and push the new fields extracted from the arguments of
// `self.head()`.
let mut new_pats = self.head().specialize(pcx, ctor);
new_pats.extend_from_slice(&self.pats[1..]);
PatStack { pats: new_pats }
// `ctor` is relevant for this row if it is the actual constructor of this row, or if the
// row has a wildcard and `ctor` is relevant for wildcards.
let ctor_is_relevant =
!matches!(self.head().ctor(), Constructor::Wildcard) || ctor_is_relevant;
PatStack { pats: new_pats, relevant: self.relevant && ctor_is_relevant }
}
}

Expand Down Expand Up @@ -820,10 +897,11 @@ impl<'p, 'tcx> MatrixRow<'p, 'tcx> {
&self,
pcx: &PatCtxt<'_, 'p, 'tcx>,
ctor: &Constructor<'tcx>,
ctor_is_relevant: bool,
parent_row: usize,
) -> MatrixRow<'p, 'tcx> {
MatrixRow {
pats: self.pats.pop_head_constructor(pcx, ctor),
pats: self.pats.pop_head_constructor(pcx, ctor, ctor_is_relevant),
parent_row,
is_under_guard: self.is_under_guard,
useful: false,
Expand Down Expand Up @@ -952,8 +1030,9 @@ impl<'p, 'tcx> Matrix<'p, 'tcx> {
&self,
pcx: &PatCtxt<'_, 'p, 'tcx>,
ctor: &Constructor<'tcx>,
ctor_is_relevant: bool,
) -> Matrix<'p, 'tcx> {
let wildcard_row = self.wildcard_row.pop_head_constructor(pcx, ctor);
let wildcard_row = self.wildcard_row.pop_head_constructor(pcx, ctor, ctor_is_relevant);
let new_validity = self.place_validity[0].specialize(pcx, ctor);
let new_place_validity = std::iter::repeat(new_validity)
.take(ctor.arity(pcx))
Expand All @@ -963,7 +1042,7 @@ impl<'p, 'tcx> Matrix<'p, 'tcx> {
Matrix { rows: Vec::new(), wildcard_row, place_validity: new_place_validity };
for (i, row) in self.rows().enumerate() {
if ctor.is_covered_by(pcx, row.head().ctor()) {
let new_row = row.pop_head_constructor(pcx, ctor, i);
let new_row = row.pop_head_constructor(pcx, ctor, ctor_is_relevant, i);
matrix.expand_and_push(new_row);
}
}
Expand Down Expand Up @@ -1161,7 +1240,10 @@ impl<'tcx> WitnessMatrix<'tcx> {
if matches!(ctor, Constructor::Missing) {
// We got the special `Missing` constructor that stands for the constructors not present
// in the match.
if !report_individual_missing_ctors {
if missing_ctors.is_empty() {
// Nothing to report.
*self = Self::empty();
} else if !report_individual_missing_ctors {
// Report `_` as missing.
let pat = WitnessPat::wild_from_ctor(pcx, Constructor::Wildcard);
self.push_pattern(pat);
Expand Down Expand Up @@ -1220,6 +1302,15 @@ fn compute_exhaustiveness_and_usefulness<'p, 'tcx>(
) -> WitnessMatrix<'tcx> {
debug_assert!(matrix.rows().all(|r| r.len() == matrix.column_count()));

if !matrix.wildcard_row.relevant && matrix.rows().all(|r| !r.pats.relevant) {
// Here we know that nothing will contribute further to exhaustiveness or usefulness. This
// is purely an optimization: skipping this check doesn't affect correctness. This check
// does change runtime behavior from exponential to quadratic on some matches found in the
// wild, so it's pretty important. It also affects which missing patterns will be reported.
// See the top of the file for details.
return WitnessMatrix::empty();
}

let Some(ty) = matrix.head_ty() else {
// The base case: there are no columns in the matrix. We are morally pattern-matching on ().
// A row is useful iff it has no (unguarded) rows above it.
Expand All @@ -1232,8 +1323,14 @@ fn compute_exhaustiveness_and_usefulness<'p, 'tcx>(
return WitnessMatrix::empty();
}
}
// No (unguarded) rows, so the match is not exhaustive. We return a new witness.
return WitnessMatrix::unit_witness();
// No (unguarded) rows, so the match is not exhaustive. We return a new witness unless
// irrelevant.
return if matrix.wildcard_row.relevant {
WitnessMatrix::unit_witness()
} else {
// We can omit the witness without affecting correctness, so we do.
WitnessMatrix::empty()
};
};

debug!("ty: {ty:?}");
Expand Down Expand Up @@ -1274,32 +1371,21 @@ fn compute_exhaustiveness_and_usefulness<'p, 'tcx>(

let mut ret = WitnessMatrix::empty();
for ctor in split_ctors {
debug!("specialize({:?})", ctor);
// Dig into rows that match `ctor`.
let mut spec_matrix = matrix.specialize_constructor(pcx, &ctor);
debug!("specialize({:?})", ctor);
// `ctor` is *irrelevant* if there's another constructor in `split_ctors` that matches
// strictly fewer rows. In that case we can sometimes skip it. See the top of the file for
// details.
let ctor_is_relevant = matches!(ctor, Constructor::Missing) || missing_ctors.is_empty();
let mut spec_matrix = matrix.specialize_constructor(pcx, &ctor, ctor_is_relevant);
let mut witnesses = ensure_sufficient_stack(|| {
compute_exhaustiveness_and_usefulness(cx, &mut spec_matrix, false)
});

let counts_for_exhaustiveness = match ctor {
Constructor::Missing => !missing_ctors.is_empty(),
// If there are missing constructors we'll report those instead. Since `Missing` matches
// only the wildcard rows, it matches fewer rows than this constructor, and is therefore
// guaranteed to result in the same or more witnesses. So skipping this does not
// jeopardize correctness.
_ => missing_ctors.is_empty(),
};
if counts_for_exhaustiveness {
// Transform witnesses for `spec_matrix` into witnesses for `matrix`.
witnesses.apply_constructor(
pcx,
&missing_ctors,
&ctor,
report_individual_missing_ctors,
);
// Accumulate the found witnesses.
ret.extend(witnesses);
}
// Transform witnesses for `spec_matrix` into witnesses for `matrix`.
witnesses.apply_constructor(pcx, &missing_ctors, &ctor, report_individual_missing_ctors);
// Accumulate the found witnesses.
ret.extend(witnesses);

// A parent row is useful if any of its children is.
for child_row in spec_matrix.rows() {
Expand Down
72 changes: 72 additions & 0 deletions tests/ui/pattern/usefulness/issue-118437-exponential-time-on-diagonal-match.rs
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
// check-pass
struct BaseCommand {
field01: bool,
field02: bool,
field03: bool,
field04: bool,
field05: bool,
field06: bool,
field07: bool,
field08: bool,
field09: bool,
field10: bool,
field11: bool,
field12: bool,
field13: bool,
field14: bool,
field15: bool,
field16: bool,
field17: bool,
field18: bool,
field19: bool,
field20: bool,
field21: bool,
field22: bool,
field23: bool,
field24: bool,
field25: bool,
field26: bool,
field27: bool,
field28: bool,
field29: bool,
field30: bool,
}

fn request_key(command: BaseCommand) {
match command {
BaseCommand { field01: true, .. } => {}
BaseCommand { field02: true, .. } => {}
BaseCommand { field03: true, .. } => {}
BaseCommand { field04: true, .. } => {}
BaseCommand { field05: true, .. } => {}
BaseCommand { field06: true, .. } => {}
BaseCommand { field07: true, .. } => {}
BaseCommand { field08: true, .. } => {}
BaseCommand { field09: true, .. } => {}
BaseCommand { field10: true, .. } => {}
BaseCommand { field11: true, .. } => {}
BaseCommand { field12: true, .. } => {}
BaseCommand { field13: true, .. } => {}
BaseCommand { field14: true, .. } => {}
BaseCommand { field15: true, .. } => {}
BaseCommand { field16: true, .. } => {}
BaseCommand { field17: true, .. } => {}
BaseCommand { field18: true, .. } => {}
BaseCommand { field19: true, .. } => {}
BaseCommand { field20: true, .. } => {}
BaseCommand { field21: true, .. } => {}
BaseCommand { field22: true, .. } => {}
BaseCommand { field23: true, .. } => {}
BaseCommand { field24: true, .. } => {}
BaseCommand { field25: true, .. } => {}
BaseCommand { field26: true, .. } => {}
BaseCommand { field27: true, .. } => {}
BaseCommand { field28: true, .. } => {}
BaseCommand { field29: true, .. } => {}
BaseCommand { field30: true, .. } => {}

_ => {}
}
}

fn main() {}

0 comments on commit 4abe247

Please sign in to comment.