From 71e83347bb1004d3aa9eaa138e2bdefa585101d1 Mon Sep 17 00:00:00 2001 From: Nadrieril Date: Wed, 29 Nov 2023 21:43:06 +0100 Subject: [PATCH 1/2] Improve performance on wide matches --- .../rustc_pattern_analysis/src/usefulness.rs | 144 ++++++++++++++---- ...8437-exponential-time-on-diagonal-match.rs | 72 +++++++++ 2 files changed, 187 insertions(+), 29 deletions(-) create mode 100644 tests/ui/pattern/usefulness/issue-118437-exponential-time-on-diagonal-match.rs diff --git a/compiler/rustc_pattern_analysis/src/usefulness.rs b/compiler/rustc_pattern_analysis/src/usefulness.rs index a7cd944c0f1bd..d291b27dfc0e8 100644 --- a/compiler/rustc_pattern_analysis/src/usefulness.rs +++ b/compiler/rustc_pattern_analysis/src/usefulness.rs @@ -300,6 +300,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 @@ -669,11 +737,15 @@ impl fmt::Display for ValidityConstraint { struct PatStack<'a, 'p, Cx: TypeCx> { // Rows of len 1 are very common, which is why `SmallVec[_; 2]` works well. pats: SmallVec<[&'a DeconstructedPat<'p, Cx>; 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<'a, 'p, Cx: TypeCx> PatStack<'a, 'p, Cx> { fn from_pattern(pat: &'a DeconstructedPat<'p, Cx>) -> Self { - PatStack { pats: smallvec![pat] } + PatStack { pats: smallvec![pat], relevant: true } } fn is_empty(&self) -> bool { @@ -708,12 +780,17 @@ impl<'a, 'p, Cx: TypeCx> PatStack<'a, 'p, Cx> { &self, pcx: &PlaceCtxt<'a, 'p, Cx>, ctor: &Constructor, + ctor_is_relevant: bool, ) -> PatStack<'a, 'p, Cx> { // 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 } } } @@ -779,10 +856,11 @@ impl<'a, 'p, Cx: TypeCx> MatrixRow<'a, 'p, Cx> { &self, pcx: &PlaceCtxt<'a, 'p, Cx>, ctor: &Constructor, + ctor_is_relevant: bool, parent_row: usize, ) -> MatrixRow<'a, 'p, Cx> { 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, @@ -897,8 +975,9 @@ impl<'a, 'p, Cx: TypeCx> Matrix<'a, 'p, Cx> { &self, pcx: &PlaceCtxt<'a, 'p, Cx>, ctor: &Constructor, + ctor_is_relevant: bool, ) -> Matrix<'a, 'p, Cx> { - 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(ctor); let new_place_validity = std::iter::repeat(new_validity) .take(ctor.arity(pcx)) @@ -908,7 +987,7 @@ impl<'a, 'p, Cx: TypeCx> Matrix<'a, 'p, Cx> { 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); } } @@ -1108,7 +1187,10 @@ impl WitnessMatrix { 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); @@ -1167,6 +1249,15 @@ fn compute_exhaustiveness_and_usefulness<'a, 'p, Cx: TypeCx>( ) -> WitnessMatrix { 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(mcx) 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. @@ -1179,8 +1270,14 @@ fn compute_exhaustiveness_and_usefulness<'a, 'p, Cx: TypeCx>( 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:?}"); @@ -1223,32 +1320,21 @@ fn compute_exhaustiveness_and_usefulness<'a, 'p, Cx: TypeCx>( 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(mcx, &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() { diff --git a/tests/ui/pattern/usefulness/issue-118437-exponential-time-on-diagonal-match.rs b/tests/ui/pattern/usefulness/issue-118437-exponential-time-on-diagonal-match.rs new file mode 100644 index 0000000000000..39ad2d4abf3cc --- /dev/null +++ b/tests/ui/pattern/usefulness/issue-118437-exponential-time-on-diagonal-match.rs @@ -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() {} From efb04e6572db5db258154299267a780bd192fb5c Mon Sep 17 00:00:00 2001 From: Nadrieril Date: Sat, 23 Dec 2023 22:15:15 +0100 Subject: [PATCH 2/2] Rework the explanation of relevancy --- .../rustc_pattern_analysis/src/usefulness.rs | 186 +++++++++++++----- 1 file changed, 138 insertions(+), 48 deletions(-) diff --git a/compiler/rustc_pattern_analysis/src/usefulness.rs b/compiler/rustc_pattern_analysis/src/usefulness.rs index d291b27dfc0e8..b51b1a1f72255 100644 --- a/compiler/rustc_pattern_analysis/src/usefulness.rs +++ b/compiler/rustc_pattern_analysis/src/usefulness.rs @@ -300,71 +300,163 @@ //! //! //! -//! # `Missing` and relevant constructors +//! # `Missing` and relevancy +//! +//! ## Relevant values //! //! Take the following example: //! //! ```compile_fail,E0004 +//! # let foo = (true, true); +//! match foo { +//! (true, _) => 1, +//! (_, true) => 2, +//! }; +//! ``` +//! +//! Consider the value `(true, true)`: +//! - Row 2 does not distinguish `(true, true)` and `(false, true)`; +//! - `false` does not show up in the first column of the match, so without knowing anything else we +//! can deduce that `(false, true)` matches the same or fewer rows than `(true, true)`. +//! +//! Using those two facts together, we deduce that `(true, true)` will not give us more usefulness +//! information about row 2 than `(false, true)` would. We say that "`(true, true)` is made +//! irrelevant for row 2 by `(false, true)`". We will use this idea to prune the search tree. +//! +//! +//! ## Computing relevancy +//! +//! We now generalize from the above example to approximate relevancy in a simple way. Note that we +//! will only compute an approximation: we can sometimes determine when a case is irrelevant, but +//! computing this precisely is at least as hard as computing usefulness. +//! +//! Our computation of relevancy relies on the `Missing` constructor. As explained in +//! [`crate::constructor`], `Missing` represents the constructors not present in a given column. For +//! example in the following: +//! +//! ```compile_fail,E0004 //! enum Direction { North, South, East, West } //! # let wind = (Direction::North, 0u8); //! match wind { -//! (Direction::North, _) => {} // arm 1 -//! (_, 50..) => {} // arm 2 -//! } +//! (Direction::North, _) => 1, +//! (_, 50..) => 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: +//! Here `South`, `East` and `West` are missing in the first column, and `0..50` is missing in the +//! second. Both of these sets are represented by `Constructor::Missing` in their corresponding +//! column. //! -//! ```ignore(partial code) -//! (50..) => {} // arm 2 -//! ``` +//! We then compute relevancy as follows: during the course of the algorithm, for a row `r`: +//! - if `r` has a wildcard in the first column; +//! - and some constructors are missing in that column; +//! - then any `c != Missing` is considered irrelevant for row `r`. //! -//! 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)`). +//! By this we mean that continuing the algorithm by specializing with `c` is guaranteed not to +//! contribute more information about the usefulness of row `r` than what we would get by +//! specializing with `Missing`. The argument is the same as in the previous subsection. //! -//! When we then specialize with `North`, we have two arms left: +//! Once we've specialized by a constructor `c` that is irrelevant for row `r`, we're guaranteed to +//! only explore values irrelevant for `r`. If we then ever reach a point where we're only exploring +//! values that are irrelevant to all of the rows (including the virtual wildcard row used for +//! exhaustiveness), we skip that case entirely. //! -//! ```ignore(partial code) -//! (_) => {} // arm 1 -//! (50..) => {} // arm 2 +//! +//! ## Example +//! +//! Let's go through a variation on the first example: +//! +//! ```compile_fail,E0004 +//! # let foo = (true, true, true); +//! match foo { +//! (true, _, true) => 1, +//! (_, true, _) => 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. +//! ```text +//! ┐ Patterns: +//! │ 1. `[(true, _, true)]` +//! │ 2. `[(_, true, _)]` +//! │ 3. `[_]` // virtual extra wildcard row +//! │ +//! │ Specialize with `(,,)`: +//! ├─┐ Patterns: +//! │ │ 1. `[true, _, true]` +//! │ │ 2. `[_, true, _]` +//! │ │ 3. `[_, _, _]` +//! │ │ +//! │ │ There are missing constructors in the first column (namely `false`), hence +//! │ │ `true` is irrelevant for rows 2 and 3. +//! │ │ +//! │ │ Specialize with `true`: +//! │ ├─┐ Patterns: +//! │ │ │ 1. `[_, true]` +//! │ │ │ 2. `[true, _]` // now exploring irrelevant cases +//! │ │ │ 3. `[_, _]` // now exploring irrelevant cases +//! │ │ │ +//! │ │ │ There are missing constructors in the first column (namely `false`), hence +//! │ │ │ `true` is irrelevant for rows 1 and 3. +//! │ │ │ +//! │ │ │ Specialize with `true`: +//! │ │ ├─┐ Patterns: +//! │ │ │ │ 1. `[true]` // now exploring irrelevant cases +//! │ │ │ │ 2. `[_]` // now exploring irrelevant cases +//! │ │ │ │ 3. `[_]` // now exploring irrelevant cases +//! │ │ │ │ +//! │ │ │ │ The current case is irrelevant for all rows: we backtrack immediately. +//! │ │ ├─┘ +//! │ │ │ +//! │ │ │ Specialize with `false`: +//! │ │ ├─┐ Patterns: +//! │ │ │ │ 1. `[true]` +//! │ │ │ │ 3. `[_]` // now exploring irrelevant cases +//! │ │ │ │ +//! │ │ │ │ Specialize with `true`: +//! │ │ │ ├─┐ Patterns: +//! │ │ │ │ │ 1. `[]` +//! │ │ │ │ │ 3. `[]` // now exploring irrelevant cases +//! │ │ │ │ │ +//! │ │ │ │ │ Row 1 is therefore useful. +//! │ │ │ ├─┘ +//! +//! ``` +//! +//! Relevancy allowed us to skip the case `(true, true, _)` entirely. In some cases this pruning can +//! give drastic speedups. The case this was built for is the following (#118437): //! -//! 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. +//! ```ignore(illustrative) +//! match foo { +//! (true, _, _, _, ..) => 1, +//! (_, true, _, _, ..) => 2, +//! (_, _, true, _, ..) => 3, +//! (_, _, _, true, ..) => 4, +//! ... +//! } +//! ``` //! -//! 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. +//! Without considering relevancy, we would explore all 2^n combinations of the `true` and `Missing` +//! constructors. Relevancy tells us that e.g. `(true, true, false, false, false, ...)` is +//! irrelevant for all the rows. This allows us to skip all cases with more than one `true` +//! constructor, changing the runtime from exponential to linear. //! -//! 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: +//! ## Relevancy and exhaustiveness //! -//! ```compile_fail,E0004 -//! # let foo = (true, true, true); +//! For exhaustiveness, we do something slightly different w.r.t relevancy: we do not report +//! witnesses of non-exhaustiveness that are irrelevant for the virtual wildcard row. For example, +//! in: +//! +//! ```ignore(illustrative) //! match foo { -//! (true, _, true) => {} -//! (_, 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. +//! we only report `(false, _)` as missing. This was a deliberate choice made early in the +//! development of rust, for diagnostic and performance purposes. As showed in the previous section, +//! ignoring irrelevant cases preserves usefulness, so this choice still correctly computes whether +//! a match is exhaustive. //! //! //! @@ -738,8 +830,8 @@ struct PatStack<'a, 'p, Cx: TypeCx> { // Rows of len 1 are very common, which is why `SmallVec[_; 2]` works well. pats: SmallVec<[&'a DeconstructedPat<'p, Cx>; 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. + /// by a different, more general, case. When the case is irrelevant for all rows this allows us + /// to skip a case entirely. This is purely an optimization. See at the top for details. relevant: bool, } @@ -1251,10 +1343,8 @@ fn compute_exhaustiveness_and_usefulness<'a, 'p, Cx: TypeCx>( 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. + // is purely an optimization: skipping this check doesn't affect correctness. See the top of + // the file for details. return WitnessMatrix::empty(); } @@ -1275,7 +1365,7 @@ fn compute_exhaustiveness_and_usefulness<'a, 'p, Cx: TypeCx>( return if matrix.wildcard_row.relevant { WitnessMatrix::unit_witness() } else { - // We can omit the witness without affecting correctness, so we do. + // We choose to not report anything here; see at the top for details. WitnessMatrix::empty() }; };