From cbc577fc719e00f60004c8b22db0256f4c9da4b2 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Thu, 7 May 2020 15:03:45 +1000 Subject: [PATCH 1/3] Reduce `TypedArena` creations in `check_match`. `check_match` creates a new `TypedArena` for every call to `create_and_enter`. DHAT tells me that each `TypedArena` typically is barely used, with typically a single allocation per arena. This commit moves the `TypedArena` creation outwards a bit, into `check_match`, and then passes it into `create_and_enter`. This reduces the number of arenas created by about 4-5x, for a very small perf win. (Moving the arena creation further outwards is hard because `check_match` is a query.) --- src/librustc_mir_build/hair/pattern/_match.rs | 7 +++---- .../hair/pattern/check_match.rs | 20 ++++++++++++++----- 2 files changed, 18 insertions(+), 9 deletions(-) diff --git a/src/librustc_mir_build/hair/pattern/_match.rs b/src/librustc_mir_build/hair/pattern/_match.rs index de3ae2e961f42..4e8ec6152e358 100644 --- a/src/librustc_mir_build/hair/pattern/_match.rs +++ b/src/librustc_mir_build/hair/pattern/_match.rs @@ -588,12 +588,11 @@ impl<'a, 'tcx> MatchCheckCtxt<'a, 'tcx> { crate fn create_and_enter( tcx: TyCtxt<'tcx>, param_env: ty::ParamEnv<'tcx>, + pattern_arena: &'a TypedArena>, module: DefId, - f: impl FnOnce(MatchCheckCtxt<'_, 'tcx>) -> R, + f: impl FnOnce(MatchCheckCtxt<'a, 'tcx>) -> R, ) -> R { - let pattern_arena = TypedArena::default(); - - f(MatchCheckCtxt { tcx, param_env, module, pattern_arena: &pattern_arena }) + f(MatchCheckCtxt { tcx, param_env, module, pattern_arena }) } fn is_uninhabited(&self, ty: Ty<'tcx>) -> bool { diff --git a/src/librustc_mir_build/hair/pattern/check_match.rs b/src/librustc_mir_build/hair/pattern/check_match.rs index c90634e511bb1..eda694ee2cba5 100644 --- a/src/librustc_mir_build/hair/pattern/check_match.rs +++ b/src/librustc_mir_build/hair/pattern/check_match.rs @@ -1,9 +1,9 @@ use super::_match::Usefulness::*; use super::_match::WitnessPreference::*; use super::_match::{expand_pattern, is_useful, MatchCheckCtxt, Matrix, PatStack}; - use super::{PatCtxt, PatKind, PatternError}; +use arena::TypedArena; use rustc_ast::ast::Mutability; use rustc_errors::{error_code, struct_span_err, Applicability, DiagnosticBuilder}; use rustc_hir as hir; @@ -17,7 +17,6 @@ use rustc_session::lint::builtin::{IRREFUTABLE_LET_PATTERNS, UNREACHABLE_PATTERN use rustc_session::parse::feature_err; use rustc_session::Session; use rustc_span::{sym, Span}; - use std::slice; crate fn check_match(tcx: TyCtxt<'_>, def_id: DefId) { @@ -26,8 +25,12 @@ crate fn check_match(tcx: TyCtxt<'_>, def_id: DefId) { Some(id) => tcx.hir().body_owned_by(tcx.hir().as_local_hir_id(id)), }; - let mut visitor = - MatchVisitor { tcx, tables: tcx.body_tables(body_id), param_env: tcx.param_env(def_id) }; + let mut visitor = MatchVisitor { + tcx, + tables: tcx.body_tables(body_id), + param_env: tcx.param_env(def_id), + pattern_arena: TypedArena::default(), + }; visitor.visit_body(tcx.hir().body(body_id)); } @@ -39,6 +42,7 @@ struct MatchVisitor<'a, 'tcx> { tcx: TyCtxt<'tcx>, tables: &'a ty::TypeckTables<'tcx>, param_env: ty::ParamEnv<'tcx>, + pattern_arena: TypedArena>, } impl<'tcx> Visitor<'tcx> for MatchVisitor<'_, 'tcx> { @@ -145,7 +149,13 @@ impl<'tcx> MatchVisitor<'_, 'tcx> { fn check_in_cx(&self, hir_id: HirId, f: impl FnOnce(MatchCheckCtxt<'_, 'tcx>)) { let module = self.tcx.parent_module(hir_id); - MatchCheckCtxt::create_and_enter(self.tcx, self.param_env, module.to_def_id(), |cx| f(cx)); + MatchCheckCtxt::create_and_enter( + self.tcx, + self.param_env, + &self.pattern_arena, + module.to_def_id(), + f, + ); } fn check_match( From 95f600d6b9aa892000c8d525a5f5cee9f89b6d1d Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Fri, 8 May 2020 07:42:49 +1000 Subject: [PATCH 2/3] Remove `MatchCheckCtxt::create_and_enter`. It has a single call site. --- src/librustc_mir_build/hair/pattern/_match.rs | 12 +----------- src/librustc_mir_build/hair/pattern/check_match.rs | 14 +++++++------- 2 files changed, 8 insertions(+), 18 deletions(-) diff --git a/src/librustc_mir_build/hair/pattern/_match.rs b/src/librustc_mir_build/hair/pattern/_match.rs index 4e8ec6152e358..cdafb63f1ebc1 100644 --- a/src/librustc_mir_build/hair/pattern/_match.rs +++ b/src/librustc_mir_build/hair/pattern/_match.rs @@ -580,21 +580,11 @@ crate struct MatchCheckCtxt<'a, 'tcx> { /// outside it's module and should not be matchable with an empty match /// statement. crate module: DefId, - param_env: ty::ParamEnv<'tcx>, + crate param_env: ty::ParamEnv<'tcx>, crate pattern_arena: &'a TypedArena>, } impl<'a, 'tcx> MatchCheckCtxt<'a, 'tcx> { - crate fn create_and_enter( - tcx: TyCtxt<'tcx>, - param_env: ty::ParamEnv<'tcx>, - pattern_arena: &'a TypedArena>, - module: DefId, - f: impl FnOnce(MatchCheckCtxt<'a, 'tcx>) -> R, - ) -> R { - f(MatchCheckCtxt { tcx, param_env, module, pattern_arena }) - } - fn is_uninhabited(&self, ty: Ty<'tcx>) -> bool { if self.tcx.features().exhaustive_patterns { self.tcx.is_ty_uninhabited_from(self.module, ty, self.param_env) diff --git a/src/librustc_mir_build/hair/pattern/check_match.rs b/src/librustc_mir_build/hair/pattern/check_match.rs index eda694ee2cba5..c3e853c0b2d12 100644 --- a/src/librustc_mir_build/hair/pattern/check_match.rs +++ b/src/librustc_mir_build/hair/pattern/check_match.rs @@ -149,13 +149,13 @@ impl<'tcx> MatchVisitor<'_, 'tcx> { fn check_in_cx(&self, hir_id: HirId, f: impl FnOnce(MatchCheckCtxt<'_, 'tcx>)) { let module = self.tcx.parent_module(hir_id); - MatchCheckCtxt::create_and_enter( - self.tcx, - self.param_env, - &self.pattern_arena, - module.to_def_id(), - f, - ); + let cx = MatchCheckCtxt { + tcx: self.tcx, + param_env: self.param_env, + module: module.to_def_id(), + pattern_arena: &self.pattern_arena, + }; + f(cx); } fn check_match( From d26d187ff83b0b10687a3c380114cda1590d9e26 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Fri, 8 May 2020 07:51:10 +1000 Subject: [PATCH 3/3] Replace `MatchVisitor::check_in_cx` with `MatchVisitor::new_cx`. The closure isn't necessary. --- .../hair/pattern/check_match.rs | 155 +++++++++--------- 1 file changed, 75 insertions(+), 80 deletions(-) diff --git a/src/librustc_mir_build/hair/pattern/check_match.rs b/src/librustc_mir_build/hair/pattern/check_match.rs index c3e853c0b2d12..0f22288437ca1 100644 --- a/src/librustc_mir_build/hair/pattern/check_match.rs +++ b/src/librustc_mir_build/hair/pattern/check_match.rs @@ -147,15 +147,13 @@ impl<'tcx> MatchVisitor<'_, 'tcx> { (pattern, pattern_ty) } - fn check_in_cx(&self, hir_id: HirId, f: impl FnOnce(MatchCheckCtxt<'_, 'tcx>)) { - let module = self.tcx.parent_module(hir_id); - let cx = MatchCheckCtxt { + fn new_cx(&self, hir_id: HirId) -> MatchCheckCtxt<'_, 'tcx> { + MatchCheckCtxt { tcx: self.tcx, param_env: self.param_env, - module: module.to_def_id(), + module: self.tcx.parent_module(hir_id).to_def_id(), pattern_arena: &self.pattern_arena, - }; - f(cx); + } } fn check_match( @@ -169,91 +167,88 @@ impl<'tcx> MatchVisitor<'_, 'tcx> { self.check_patterns(arm.guard.is_some(), &arm.pat); } - self.check_in_cx(scrut.hir_id, |ref mut cx| { - let mut have_errors = false; + let mut cx = self.new_cx(scrut.hir_id); - let inlined_arms: Vec<_> = arms - .iter() - .map(|hir::Arm { pat, guard, .. }| { - (self.lower_pattern(cx, pat, &mut have_errors).0, pat.hir_id, guard.is_some()) - }) - .collect(); + let mut have_errors = false; - // Bail out early if inlining failed. - if have_errors { - return; - } + let inlined_arms: Vec<_> = arms + .iter() + .map(|hir::Arm { pat, guard, .. }| { + (self.lower_pattern(&mut cx, pat, &mut have_errors).0, pat.hir_id, guard.is_some()) + }) + .collect(); - // Fourth, check for unreachable arms. - let matrix = check_arms(cx, &inlined_arms, source); + // Bail out early if inlining failed. + if have_errors { + return; + } + + // Fourth, check for unreachable arms. + let matrix = check_arms(&mut cx, &inlined_arms, source); - // Fifth, check if the match is exhaustive. - let scrut_ty = self.tables.node_type(scrut.hir_id); - // Note: An empty match isn't the same as an empty matrix for diagnostics purposes, - // since an empty matrix can occur when there are arms, if those arms all have guards. - let is_empty_match = inlined_arms.is_empty(); - check_exhaustive(cx, scrut_ty, scrut.span, &matrix, scrut.hir_id, is_empty_match); - }) + // Fifth, check if the match is exhaustive. + let scrut_ty = self.tables.node_type(scrut.hir_id); + // Note: An empty match isn't the same as an empty matrix for diagnostics purposes, + // since an empty matrix can occur when there are arms, if those arms all have guards. + let is_empty_match = inlined_arms.is_empty(); + check_exhaustive(&mut cx, scrut_ty, scrut.span, &matrix, scrut.hir_id, is_empty_match); } fn check_irrefutable(&self, pat: &'tcx Pat<'tcx>, origin: &str, sp: Option) { - self.check_in_cx(pat.hir_id, |ref mut cx| { - let (pattern, pattern_ty) = self.lower_pattern(cx, pat, &mut false); - let pats: Matrix<'_, '_> = vec![PatStack::from_pattern(pattern)].into_iter().collect(); - - let witnesses = match check_not_useful(cx, pattern_ty, &pats, pat.hir_id) { - Ok(_) => return, - Err(err) => err, - }; - - let joined_patterns = joined_uncovered_patterns(&witnesses); - let mut err = struct_span_err!( - self.tcx.sess, - pat.span, - E0005, - "refutable pattern in {}: {} not covered", - origin, - joined_patterns - ); - let suggest_if_let = match &pat.kind { - hir::PatKind::Path(hir::QPath::Resolved(None, path)) - if path.segments.len() == 1 && path.segments[0].args.is_none() => - { - const_not_var(&mut err, cx.tcx, pat, path); - false - } - _ => { - err.span_label( - pat.span, - pattern_not_covered_label(&witnesses, &joined_patterns), - ); - true - } - }; + let mut cx = self.new_cx(pat.hir_id); - if let (Some(span), true) = (sp, suggest_if_let) { - err.note( - "`let` bindings require an \"irrefutable pattern\", like a `struct` or \ - an `enum` with only one variant", - ); - if let Ok(snippet) = self.tcx.sess.source_map().span_to_snippet(span) { - err.span_suggestion( - span, - "you might want to use `if let` to ignore the variant that isn't matched", - format!("if {} {{ /* */ }}", &snippet[..snippet.len() - 1]), - Applicability::HasPlaceholders, - ); - } - err.note( - "for more information, visit \ - https://doc.rust-lang.org/book/ch18-02-refutability.html", + let (pattern, pattern_ty) = self.lower_pattern(&mut cx, pat, &mut false); + let pats: Matrix<'_, '_> = vec![PatStack::from_pattern(pattern)].into_iter().collect(); + + let witnesses = match check_not_useful(&mut cx, pattern_ty, &pats, pat.hir_id) { + Ok(_) => return, + Err(err) => err, + }; + + let joined_patterns = joined_uncovered_patterns(&witnesses); + let mut err = struct_span_err!( + self.tcx.sess, + pat.span, + E0005, + "refutable pattern in {}: {} not covered", + origin, + joined_patterns + ); + let suggest_if_let = match &pat.kind { + hir::PatKind::Path(hir::QPath::Resolved(None, path)) + if path.segments.len() == 1 && path.segments[0].args.is_none() => + { + const_not_var(&mut err, cx.tcx, pat, path); + false + } + _ => { + err.span_label(pat.span, pattern_not_covered_label(&witnesses, &joined_patterns)); + true + } + }; + + if let (Some(span), true) = (sp, suggest_if_let) { + err.note( + "`let` bindings require an \"irrefutable pattern\", like a `struct` or \ + an `enum` with only one variant", + ); + if let Ok(snippet) = self.tcx.sess.source_map().span_to_snippet(span) { + err.span_suggestion( + span, + "you might want to use `if let` to ignore the variant that isn't matched", + format!("if {} {{ /* */ }}", &snippet[..snippet.len() - 1]), + Applicability::HasPlaceholders, ); } + err.note( + "for more information, visit \ + https://doc.rust-lang.org/book/ch18-02-refutability.html", + ); + } - adt_defined_here(cx, &mut err, pattern_ty, &witnesses); - err.note(&format!("the matched value is of type `{}`", pattern_ty)); - err.emit(); - }); + adt_defined_here(&mut cx, &mut err, pattern_ty, &witnesses); + err.note(&format!("the matched value is of type `{}`", pattern_ty)); + err.emit(); } }