Skip to content

Commit

Permalink
Auto merge of #68966 - jonas-schievink:coherence-perf, r=<try>
Browse files Browse the repository at this point in the history
Improve performance of coherence checks

The biggest perf improvement in here is expected to come from the removal of the remaining #43355 warning code since the effectively runs the expensive parts of coherence *twice* (which can even be visualized by obtaining a flamegraph https://twitter.com/sheevink/status/1225963187511709696).
  • Loading branch information
bors committed Feb 8, 2020
2 parents 85ffd44 + 14a15d6 commit fe57f83
Show file tree
Hide file tree
Showing 10 changed files with 76 additions and 132 deletions.
3 changes: 2 additions & 1 deletion src/librustc/query/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -647,7 +647,8 @@ rustc_queries! {
query trait_impls_of(key: DefId) -> &'tcx ty::trait_def::TraitImpls {
desc { |tcx| "trait impls of `{}`", tcx.def_path_str(key) }
}
query specialization_graph_of(_: DefId) -> &'tcx specialization_graph::Graph {
query specialization_graph_of(key: DefId) -> &'tcx specialization_graph::Graph {
desc { |tcx| "building specialization graph of trait `{}`", tcx.def_path_str(key) }
cache_on_disk_if { true }
}
query is_object_safe(key: DefId) -> bool {
Expand Down
23 changes: 6 additions & 17 deletions src/librustc/traits/coherence.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@

use crate::infer::{CombinedSnapshot, InferOk};
use crate::traits::select::IntercrateAmbiguityCause;
use crate::traits::IntercrateMode;
use crate::traits::SkipLeakCheck;
use crate::traits::{self, Normalized, Obligation, ObligationCause, SelectionContext};
use crate::ty::fold::TypeFoldable;
Expand All @@ -27,7 +26,7 @@ enum InCrate {
#[derive(Debug, Copy, Clone)]
pub enum Conflict {
Upstream,
Downstream { used_to_be_broken: bool },
Downstream,
}

pub struct OverlapResult<'tcx> {
Expand All @@ -53,7 +52,6 @@ pub fn overlapping_impls<F1, F2, R>(
tcx: TyCtxt<'_>,
impl1_def_id: DefId,
impl2_def_id: DefId,
intercrate_mode: IntercrateMode,
skip_leak_check: SkipLeakCheck,
on_overlap: F1,
no_overlap: F2,
Expand All @@ -65,13 +63,12 @@ where
debug!(
"overlapping_impls(\
impl1_def_id={:?}, \
impl2_def_id={:?},
intercrate_mode={:?})",
impl1_def_id, impl2_def_id, intercrate_mode
impl2_def_id={:?})",
impl1_def_id, impl2_def_id,
);

let overlaps = tcx.infer_ctxt().enter(|infcx| {
let selcx = &mut SelectionContext::intercrate(&infcx, intercrate_mode);
let selcx = &mut SelectionContext::intercrate(&infcx);
overlap(selcx, skip_leak_check, impl1_def_id, impl2_def_id).is_some()
});

Expand All @@ -83,7 +80,7 @@ where
// this time tracking intercrate ambuiguity causes for better
// diagnostics. (These take time and can lead to false errors.)
tcx.infer_ctxt().enter(|infcx| {
let selcx = &mut SelectionContext::intercrate(&infcx, intercrate_mode);
let selcx = &mut SelectionContext::intercrate(&infcx);
selcx.enable_tracking_intercrate_ambiguity_causes();
on_overlap(overlap(selcx, skip_leak_check, impl1_def_id, impl2_def_id).unwrap())
})
Expand Down Expand Up @@ -202,15 +199,7 @@ pub fn trait_ref_is_knowable<'tcx>(
if orphan_check_trait_ref(tcx, trait_ref, InCrate::Remote).is_ok() {
// A downstream or cousin crate is allowed to implement some
// substitution of this trait-ref.

// A trait can be implementable for a trait ref by both the current
// crate and crates downstream of it. Older versions of rustc
// were not aware of this, causing incoherence (issue #43355).
let used_to_be_broken = orphan_check_trait_ref(tcx, trait_ref, InCrate::Local).is_ok();
if used_to_be_broken {
debug!("trait_ref_is_knowable({:?}) - USED TO BE BROKEN", trait_ref);
}
return Some(Conflict::Downstream { used_to_be_broken });
return Some(Conflict::Downstream);
}

if trait_ref_is_local_or_fundamental(tcx, trait_ref) {
Expand Down
7 changes: 0 additions & 7 deletions src/librustc/traits/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,13 +76,6 @@ pub use self::chalk_fulfill::{

pub use self::types::*;

/// Whether to enable bug compatibility with issue #43355.
#[derive(Copy, Clone, PartialEq, Eq, Debug)]
pub enum IntercrateMode {
Issue43355,
Fixed,
}

/// Whether to skip the leak check, as part of a future compatibility warning step.
#[derive(Copy, Clone, PartialEq, Eq, Debug)]
pub enum SkipLeakCheck {
Expand Down
44 changes: 15 additions & 29 deletions src/librustc/traits/select.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@ use super::DerivedObligationCause;
use super::Selection;
use super::SelectionResult;
use super::TraitNotObjectSafe;
use super::TraitQueryMode;
use super::{BuiltinDerivedObligation, ImplDerivedObligation, ObligationCauseCode};
use super::{IntercrateMode, TraitQueryMode};
use super::{ObjectCastObligation, Obligation};
use super::{ObligationCause, PredicateObligation, TraitObligation};
use super::{OutputTypeParameterMismatch, Overflow, SelectionError, Unimplemented};
Expand Down Expand Up @@ -78,7 +78,7 @@ pub struct SelectionContext<'cx, 'tcx> {
/// other words, we consider `$0: Bar` to be unimplemented if
/// there is no type that the user could *actually name* that
/// would satisfy it. This avoids crippling inference, basically.
intercrate: Option<IntercrateMode>,
intercrate: bool,

intercrate_ambiguity_causes: Option<Vec<IntercrateAmbiguityCause>>,

Expand Down Expand Up @@ -216,22 +216,18 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
SelectionContext {
infcx,
freshener: infcx.freshener(),
intercrate: None,
intercrate: false,
intercrate_ambiguity_causes: None,
allow_negative_impls: false,
query_mode: TraitQueryMode::Standard,
}
}

pub fn intercrate(
infcx: &'cx InferCtxt<'cx, 'tcx>,
mode: IntercrateMode,
) -> SelectionContext<'cx, 'tcx> {
debug!("intercrate({:?})", mode);
pub fn intercrate(infcx: &'cx InferCtxt<'cx, 'tcx>) -> SelectionContext<'cx, 'tcx> {
SelectionContext {
infcx,
freshener: infcx.freshener(),
intercrate: Some(mode),
intercrate: true,
intercrate_ambiguity_causes: None,
allow_negative_impls: false,
query_mode: TraitQueryMode::Standard,
Expand All @@ -246,7 +242,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
SelectionContext {
infcx,
freshener: infcx.freshener(),
intercrate: None,
intercrate: false,
intercrate_ambiguity_causes: None,
allow_negative_impls,
query_mode: TraitQueryMode::Standard,
Expand All @@ -261,7 +257,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
SelectionContext {
infcx,
freshener: infcx.freshener(),
intercrate: None,
intercrate: false,
intercrate_ambiguity_causes: None,
allow_negative_impls: false,
query_mode,
Expand All @@ -274,7 +270,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
/// false overflow results (#47139) and because it costs
/// computation time.
pub fn enable_tracking_intercrate_ambiguity_causes(&mut self) {
assert!(self.intercrate.is_some());
assert!(self.intercrate);
assert!(self.intercrate_ambiguity_causes.is_none());
self.intercrate_ambiguity_causes = Some(vec![]);
debug!("selcx: enable_tracking_intercrate_ambiguity_causes");
Expand All @@ -284,7 +280,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
/// was enabled and disables tracking at the same time. If
/// tracking is not enabled, just returns an empty vector.
pub fn take_intercrate_ambiguity_causes(&mut self) -> Vec<IntercrateAmbiguityCause> {
assert!(self.intercrate.is_some());
assert!(self.intercrate);
self.intercrate_ambiguity_causes.take().unwrap_or(vec![])
}

Expand Down Expand Up @@ -560,7 +556,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
) -> Result<EvaluationResult, OverflowError> {
debug!("evaluate_trait_predicate_recursively({:?})", obligation);

if self.intercrate.is_none()
if !self.intercrate
&& obligation.is_global()
&& obligation.param_env.caller_bounds.iter().all(|bound| bound.needs_subst())
{
Expand Down Expand Up @@ -725,7 +721,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
stack.fresh_trait_ref.skip_binder().input_types().any(|ty| ty.is_fresh());
// This check was an imperfect workaround for a bug in the old
// intercrate mode; it should be removed when that goes away.
if unbound_input_types && self.intercrate == Some(IntercrateMode::Issue43355) {
if unbound_input_types && self.intercrate {
debug!(
"evaluate_stack({:?}) --> unbound argument, intercrate --> ambiguous",
stack.fresh_trait_ref
Expand Down Expand Up @@ -1204,7 +1200,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
fn is_knowable<'o>(&mut self, stack: &TraitObligationStack<'o, 'tcx>) -> Option<Conflict> {
debug!("is_knowable(intercrate={:?})", self.intercrate);

if !self.intercrate.is_some() {
if !self.intercrate {
return None;
}

Expand All @@ -1216,17 +1212,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
// bound regions.
let trait_ref = predicate.skip_binder().trait_ref;

let result = coherence::trait_ref_is_knowable(self.tcx(), trait_ref);
if let (
Some(Conflict::Downstream { used_to_be_broken: true }),
Some(IntercrateMode::Issue43355),
) = (result, self.intercrate)
{
debug!("is_knowable: IGNORING conflict to be bug-compatible with #43355");
None
} else {
result
}
coherence::trait_ref_is_knowable(self.tcx(), trait_ref)
}

/// Returns `true` if the global caches can be used.
Expand All @@ -1247,7 +1233,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
// the master cache. Since coherence executes pretty quickly,
// it's not worth going to more trouble to increase the
// hit-rate, I don't think.
if self.intercrate.is_some() {
if self.intercrate {
return false;
}

Expand Down Expand Up @@ -3303,7 +3289,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
return Err(());
}

if self.intercrate.is_none()
if !self.intercrate
&& self.tcx().impl_polarity(impl_def_id) == ty::ImplPolarity::Reservation
{
debug!("match_impl: reservation impls only apply in intercrate mode");
Expand Down
7 changes: 1 addition & 6 deletions src/librustc/traits/specialize/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -332,14 +332,9 @@ pub(super) fn specialization_graph_provider(
let impl_span =
tcx.sess.source_map().def_span(tcx.span_of_impl(impl_def_id).unwrap());
let mut err = match used_to_be_allowed {
Some(FutureCompatOverlapErrorKind::Issue43355) | None => {
struct_span_err!(tcx.sess, impl_span, E0119, "{}", msg)
}
None => struct_span_err!(tcx.sess, impl_span, E0119, "{}", msg),
Some(kind) => {
let lint = match kind {
FutureCompatOverlapErrorKind::Issue43355 => {
unreachable!("converted to hard error above")
}
FutureCompatOverlapErrorKind::Issue33140 => {
ORDER_DEPENDENT_TRAIT_OBJECTS
}
Expand Down
21 changes: 1 addition & 20 deletions src/librustc/traits/specialize/specialization_graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ pub use rustc::traits::types::specialization_graph::*;

#[derive(Copy, Clone, Debug)]
pub enum FutureCompatOverlapErrorKind {
Issue43355,
Issue33140,
LeakCheck,
}
Expand Down Expand Up @@ -111,7 +110,6 @@ impl<'tcx> Children {
tcx,
possible_sibling,
impl_def_id,
traits::IntercrateMode::Issue43355,
traits::SkipLeakCheck::default(),
|overlap| {
if let Some(overlap_kind) =
Expand Down Expand Up @@ -155,30 +153,13 @@ impl<'tcx> Children {
replace_children.push(possible_sibling);
} else {
if let None = tcx.impls_are_allowed_to_overlap(impl_def_id, possible_sibling) {
// do future-compat checks for overlap. Have issue #33140
// errors overwrite issue #43355 errors when both are present.

traits::overlapping_impls(
tcx,
possible_sibling,
impl_def_id,
traits::IntercrateMode::Fixed,
traits::SkipLeakCheck::default(),
|overlap| {
last_lint = Some(FutureCompatOverlapError {
error: overlap_error(overlap),
kind: FutureCompatOverlapErrorKind::Issue43355,
});
},
|| (),
);
// Do future-compat checks for overlap.

if last_lint.is_none() {
traits::overlapping_impls(
tcx,
possible_sibling,
impl_def_id,
traits::IntercrateMode::Fixed,
traits::SkipLeakCheck::Yes,
|overlap| {
last_lint = Some(FutureCompatOverlapError {
Expand Down
12 changes: 5 additions & 7 deletions src/librustc_typeck/coherence/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,12 @@ use rustc_hir::def_id::DefId;
use rustc_hir::ItemKind;

pub fn check_trait(tcx: TyCtxt<'_>, trait_def_id: DefId) {
let lang_items = tcx.lang_items();
Checker { tcx, trait_def_id }
.check(tcx.lang_items().drop_trait(), visit_implementation_of_drop)
.check(tcx.lang_items().copy_trait(), visit_implementation_of_copy)
.check(tcx.lang_items().coerce_unsized_trait(), visit_implementation_of_coerce_unsized)
.check(
tcx.lang_items().dispatch_from_dyn_trait(),
visit_implementation_of_dispatch_from_dyn,
);
.check(lang_items.drop_trait(), visit_implementation_of_drop)
.check(lang_items.copy_trait(), visit_implementation_of_copy)
.check(lang_items.coerce_unsized_trait(), visit_implementation_of_coerce_unsized)
.check(lang_items.dispatch_from_dyn_trait(), visit_implementation_of_dispatch_from_dyn);
}

struct Checker<'tcx> {
Expand Down
3 changes: 1 addition & 2 deletions src/librustc_typeck/coherence/inherent_impls_overlap.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use crate::namespace::Namespace;
use rustc::traits::{self, IntercrateMode, SkipLeakCheck};
use rustc::traits::{self, SkipLeakCheck};
use rustc::ty::TyCtxt;
use rustc_errors::struct_span_err;
use rustc_hir as hir;
Expand Down Expand Up @@ -75,7 +75,6 @@ impl InherentOverlapChecker<'tcx> {
self.tcx,
impl1_def_id,
impl2_def_id,
IntercrateMode::Issue43355,
// We go ahead and just skip the leak check for
// inherent impls without warning.
SkipLeakCheck::Yes,
Expand Down
Loading

0 comments on commit fe57f83

Please sign in to comment.