Skip to content

Commit

Permalink
Auto merge of #52342 - nnethercote:CanonicalVar, r=nikomatsakis
Browse files Browse the repository at this point in the history
Avoid most allocations in `Canonicalizer`.

Extra allocations are a significant cost of NLL, and the most common
ones come from within `Canonicalizer`. In particular, `canonical_var()`
contains this code:

    indices
	.entry(kind)
	.or_insert_with(|| {
	    let cvar1 = variables.push(info);
	    let cvar2 = var_values.push(kind);
	    assert_eq!(cvar1, cvar2);
	    cvar1
	})
	.clone()

`variables` and `var_values` are `Vec`s. `indices` is a `HashMap` used
to track what elements have been inserted into `var_values`. If `kind`
hasn't been seen before, `indices`, `variables` and `var_values` all get
a new element. (The number of elements in each container is always the
same.) This results in lots of allocations.

In practice, most of the time these containers only end up holding a few
elements. This PR changes them to avoid heap allocations in the common
case, by changing the `Vec`s to `SmallVec`s and only using `indices`
once enough elements are present. (When the number of elements is small,
a direct linear search of `var_values` is as good or better than a
hashmap lookup.)

The changes to `variables` are straightforward and contained within
`Canonicalizer`. The changes to `indices` are more complex but also
contained within `Canonicalizer`. The changes to `var_values` are more
intrusive because they require defining a new type
`SmallCanonicalVarValues` -- which is to `CanonicalVarValues` as
`SmallVec` is to `Vec -- and passing stack-allocated values of that type
in from outside.

All this speeds up a number of NLL "check" builds, the best by 2%.

r? @nikomatsakis
  • Loading branch information
bors committed Jul 18, 2018
2 parents 4f3c7a4 + 7cc5277 commit f686885
Show file tree
Hide file tree
Showing 12 changed files with 121 additions and 63 deletions.
100 changes: 68 additions & 32 deletions src/librustc/infer/canonical/canonicalizer.rs
Expand Up @@ -16,8 +16,8 @@
//! [c]: https://rust-lang-nursery.github.io/rustc-guide/traits/canonicalization.html

use infer::canonical::{
Canonical, CanonicalTyVarKind, CanonicalVarInfo, CanonicalVarKind, CanonicalVarValues,
Canonicalized,
Canonical, CanonicalTyVarKind, CanonicalVarInfo, CanonicalVarKind, Canonicalized,
SmallCanonicalVarValues,
};
use infer::InferCtxt;
use std::sync::atomic::Ordering;
Expand All @@ -26,7 +26,8 @@ use ty::subst::Kind;
use ty::{self, CanonicalVar, Lift, Slice, Ty, TyCtxt, TypeFlags};

use rustc_data_structures::fx::FxHashMap;
use rustc_data_structures::indexed_vec::IndexVec;
use rustc_data_structures::indexed_vec::Idx;
use rustc_data_structures::small_vec::SmallVec;

impl<'cx, 'gcx, 'tcx> InferCtxt<'cx, 'gcx, 'tcx> {
/// Canonicalizes a query value `V`. When we canonicalize a query,
Expand All @@ -47,7 +48,8 @@ impl<'cx, 'gcx, 'tcx> InferCtxt<'cx, 'gcx, 'tcx> {
pub fn canonicalize_query<V>(
&self,
value: &V,
) -> (Canonicalized<'gcx, V>, CanonicalVarValues<'tcx>)
var_values: &mut SmallCanonicalVarValues<'tcx>
) -> Canonicalized<'gcx, V>
where
V: TypeFoldable<'tcx> + Lift<'gcx>,
{
Expand All @@ -65,6 +67,7 @@ impl<'cx, 'gcx, 'tcx> InferCtxt<'cx, 'gcx, 'tcx> {
static_region: true,
other_free_regions: true,
},
var_values,
)
}

Expand Down Expand Up @@ -96,10 +99,11 @@ impl<'cx, 'gcx, 'tcx> InferCtxt<'cx, 'gcx, 'tcx> {
pub fn canonicalize_response<V>(
&self,
value: &V,
) -> (Canonicalized<'gcx, V>, CanonicalVarValues<'tcx>)
) -> Canonicalized<'gcx, V>
where
V: TypeFoldable<'tcx> + Lift<'gcx>,
{
let mut var_values = SmallVec::new();
Canonicalizer::canonicalize(
value,
Some(self),
Expand All @@ -108,6 +112,7 @@ impl<'cx, 'gcx, 'tcx> InferCtxt<'cx, 'gcx, 'tcx> {
static_region: false,
other_free_regions: false,
},
&mut var_values
)
}

Expand All @@ -123,7 +128,8 @@ impl<'cx, 'gcx, 'tcx> InferCtxt<'cx, 'gcx, 'tcx> {
pub fn canonicalize_hr_query_hack<V>(
&self,
value: &V,
) -> (Canonicalized<'gcx, V>, CanonicalVarValues<'tcx>)
var_values: &mut SmallCanonicalVarValues<'tcx>
) -> Canonicalized<'gcx, V>
where
V: TypeFoldable<'tcx> + Lift<'gcx>,
{
Expand All @@ -141,6 +147,7 @@ impl<'cx, 'gcx, 'tcx> InferCtxt<'cx, 'gcx, 'tcx> {
static_region: false,
other_free_regions: true,
},
var_values
)
}
}
Expand All @@ -163,9 +170,11 @@ impl CanonicalizeRegionMode {
struct Canonicalizer<'cx, 'gcx: 'tcx, 'tcx: 'cx> {
infcx: Option<&'cx InferCtxt<'cx, 'gcx, 'tcx>>,
tcx: TyCtxt<'cx, 'gcx, 'tcx>,
variables: IndexVec<CanonicalVar, CanonicalVarInfo>,
variables: SmallVec<[CanonicalVarInfo; 8]>,
var_values: &'cx mut SmallCanonicalVarValues<'tcx>,
// Note that indices is only used once `var_values` is big enough to be
// heap-allocated.
indices: FxHashMap<Kind<'tcx>, CanonicalVar>,
var_values: IndexVec<CanonicalVar, Kind<'tcx>>,
canonicalize_region_mode: CanonicalizeRegionMode,
needs_canonical_flags: TypeFlags,
}
Expand Down Expand Up @@ -295,7 +304,8 @@ impl<'cx, 'gcx, 'tcx> Canonicalizer<'cx, 'gcx, 'tcx> {
infcx: Option<&'cx InferCtxt<'cx, 'gcx, 'tcx>>,
tcx: TyCtxt<'cx, 'gcx, 'tcx>,
canonicalize_region_mode: CanonicalizeRegionMode,
) -> (Canonicalized<'gcx, V>, CanonicalVarValues<'tcx>)
var_values: &'cx mut SmallCanonicalVarValues<'tcx>
) -> Canonicalized<'gcx, V>
where
V: TypeFoldable<'tcx> + Lift<'gcx>,
{
Expand All @@ -320,20 +330,17 @@ impl<'cx, 'gcx, 'tcx> Canonicalizer<'cx, 'gcx, 'tcx> {
variables: Slice::empty(),
value: out_value,
};
let values = CanonicalVarValues {
var_values: IndexVec::default(),
};
return (canon_value, values);
return canon_value;
}

let mut canonicalizer = Canonicalizer {
infcx,
tcx,
canonicalize_region_mode,
needs_canonical_flags,
variables: IndexVec::default(),
variables: SmallVec::new(),
var_values,
indices: FxHashMap::default(),
var_values: IndexVec::default(),
};
let out_value = value.fold_with(&mut canonicalizer);

Expand All @@ -348,16 +355,12 @@ impl<'cx, 'gcx, 'tcx> Canonicalizer<'cx, 'gcx, 'tcx> {
)
});

let canonical_variables = tcx.intern_canonical_var_infos(&canonicalizer.variables.raw);
let canonical_variables = tcx.intern_canonical_var_infos(&canonicalizer.variables);

let canonical_value = Canonical {
Canonical {
variables: canonical_variables,
value: out_value,
};
let canonical_var_values = CanonicalVarValues {
var_values: canonicalizer.var_values,
};
(canonical_value, canonical_var_values)
}
}

/// Creates a canonical variable replacing `kind` from the input,
Expand All @@ -366,21 +369,54 @@ impl<'cx, 'gcx, 'tcx> Canonicalizer<'cx, 'gcx, 'tcx> {
/// potentially a free region).
fn canonical_var(&mut self, info: CanonicalVarInfo, kind: Kind<'tcx>) -> CanonicalVar {
let Canonicalizer {
indices,
variables,
var_values,
indices,
..
} = self;

indices
.entry(kind)
.or_insert_with(|| {
let cvar1 = variables.push(info);
let cvar2 = var_values.push(kind);
assert_eq!(cvar1, cvar2);
cvar1
})
.clone()
// This code is hot. `variables` and `var_values` are usually small
// (fewer than 8 elements ~95% of the time). They are SmallVec's to
// avoid allocations in those cases. We also don't use `indices` to
// determine if a kind has been seen before until the limit of 8 has
// been exceeded, to also avoid allocations for `indices`.
if var_values.is_array() {
// `var_values` is stack-allocated. `indices` isn't used yet. Do a
// direct linear search of `var_values`.
if let Some(idx) = var_values.iter().position(|&k| k == kind) {
// `kind` is already present in `var_values`.
CanonicalVar::new(idx)
} else {
// `kind` isn't present in `var_values`. Append it. Likewise
// for `info` and `variables`.
variables.push(info);
var_values.push(kind);
assert_eq!(variables.len(), var_values.len());

// If `var_values` has become big enough to be heap-allocated,
// fill up `indices` to facilitate subsequent lookups.
if !var_values.is_array() {
assert!(indices.is_empty());
*indices =
var_values.iter()
.enumerate()
.map(|(i, &kind)| (kind, CanonicalVar::new(i)))
.collect();
}
// The cv is the index of the appended element.
CanonicalVar::new(var_values.len() - 1)
}
} else {
// `var_values` is large. Do a hashmap search via `indices`.
*indices
.entry(kind)
.or_insert_with(|| {
variables.push(info);
var_values.push(kind);
assert_eq!(variables.len(), var_values.len());
CanonicalVar::new(variables.len() - 1)
})
}
}

/// Given a type variable `ty_var` of the given kind, first check
Expand Down
9 changes: 5 additions & 4 deletions src/librustc/infer/canonical/mod.rs
Expand Up @@ -33,6 +33,7 @@

use infer::{InferCtxt, RegionVariableOrigin, TypeVariableOrigin};
use rustc_data_structures::indexed_vec::IndexVec;
use rustc_data_structures::small_vec::SmallVec;
use rustc_data_structures::sync::Lrc;
use serialize::UseSpecializedDecodable;
use std::ops::Index;
Expand Down Expand Up @@ -74,6 +75,10 @@ pub struct CanonicalVarValues<'tcx> {
pub var_values: IndexVec<CanonicalVar, Kind<'tcx>>,
}

/// Like CanonicalVarValues, but for use in places where a SmallVec is
/// appropriate.
pub type SmallCanonicalVarValues<'tcx> = SmallVec<[Kind<'tcx>; 8]>;

/// Information about a canonical variable that is included with the
/// canonical value. This is sufficient information for code to create
/// a copy of the canonical value in some other inference context,
Expand Down Expand Up @@ -281,10 +286,6 @@ BraceStructLiftImpl! {
}

impl<'tcx> CanonicalVarValues<'tcx> {
fn iter<'a>(&'a self) -> impl Iterator<Item = Kind<'tcx>> + 'a {
self.var_values.iter().cloned()
}

fn len(&self) -> usize {
self.var_values.len()
}
Expand Down
29 changes: 15 additions & 14 deletions src/librustc/infer/canonical/query_result.rs
Expand Up @@ -19,7 +19,7 @@

use infer::canonical::substitute::substitute_value;
use infer::canonical::{Canonical, CanonicalVarKind, CanonicalVarValues, CanonicalizedQueryResult,
Certainty, QueryRegionConstraint, QueryResult};
Certainty, QueryRegionConstraint, QueryResult, SmallCanonicalVarValues};
use infer::region_constraints::{Constraint, RegionConstraintData};
use infer::InferCtxtBuilder;
use infer::{InferCtxt, InferOk, InferResult, RegionObligation};
Expand Down Expand Up @@ -103,7 +103,7 @@ impl<'cx, 'gcx, 'tcx> InferCtxt<'cx, 'gcx, 'tcx> {
T: Debug + Lift<'gcx> + TypeFoldable<'tcx>,
{
let query_result = self.make_query_result(inference_vars, answer, fulfill_cx)?;
let (canonical_result, _) = self.canonicalize_response(&query_result);
let canonical_result = self.canonicalize_response(&query_result);

debug!(
"make_canonicalized_query_result: canonical_result = {:#?}",
Expand Down Expand Up @@ -186,7 +186,7 @@ impl<'cx, 'gcx, 'tcx> InferCtxt<'cx, 'gcx, 'tcx> {
&self,
cause: &ObligationCause<'tcx>,
param_env: ty::ParamEnv<'tcx>,
original_values: &CanonicalVarValues<'tcx>,
original_values: &SmallCanonicalVarValues<'tcx>,
query_result: &Canonical<'tcx, QueryResult<'tcx, R>>,
) -> InferResult<'tcx, R>
where
Expand Down Expand Up @@ -252,7 +252,7 @@ impl<'cx, 'gcx, 'tcx> InferCtxt<'cx, 'gcx, 'tcx> {
&self,
cause: &ObligationCause<'tcx>,
param_env: ty::ParamEnv<'tcx>,
original_values: &CanonicalVarValues<'tcx>,
original_values: &SmallCanonicalVarValues<'tcx>,
query_result: &Canonical<'tcx, QueryResult<'tcx, R>>,
output_query_region_constraints: &mut Vec<QueryRegionConstraint<'tcx>>,
) -> InferResult<'tcx, R>
Expand All @@ -274,10 +274,11 @@ impl<'cx, 'gcx, 'tcx> InferCtxt<'cx, 'gcx, 'tcx> {
// variable...
let mut obligations = vec![];

for (index, original_value) in original_values.var_values.iter_enumerated() {
for (index, original_value) in original_values.iter().enumerate() {
// ...with the value `v_r` of that variable from the query.
let result_value = query_result
.substitute_projected(self.tcx, &result_subst, |v| &v.var_values[index]);
.substitute_projected(self.tcx, &result_subst,
|v| &v.var_values[CanonicalVar::new(index)]);
match (original_value.unpack(), result_value.unpack()) {
(UnpackedKind::Lifetime(ty::ReErased), UnpackedKind::Lifetime(ty::ReErased)) => {
// no action needed
Expand Down Expand Up @@ -341,7 +342,7 @@ impl<'cx, 'gcx, 'tcx> InferCtxt<'cx, 'gcx, 'tcx> {
&self,
cause: &ObligationCause<'tcx>,
param_env: ty::ParamEnv<'tcx>,
original_values: &CanonicalVarValues<'tcx>,
original_values: &SmallCanonicalVarValues<'tcx>,
query_result: &Canonical<'tcx, QueryResult<'tcx, R>>,
) -> InferResult<'tcx, CanonicalVarValues<'tcx>>
where
Expand Down Expand Up @@ -382,7 +383,7 @@ impl<'cx, 'gcx, 'tcx> InferCtxt<'cx, 'gcx, 'tcx> {
fn query_result_substitution_guess<R>(
&self,
cause: &ObligationCause<'tcx>,
original_values: &CanonicalVarValues<'tcx>,
original_values: &SmallCanonicalVarValues<'tcx>,
query_result: &Canonical<'tcx, QueryResult<'tcx, R>>,
) -> CanonicalVarValues<'tcx>
where
Expand Down Expand Up @@ -418,14 +419,14 @@ impl<'cx, 'gcx, 'tcx> InferCtxt<'cx, 'gcx, 'tcx> {
// e.g., here `result_value` might be `?0` in the example above...
if let ty::TyInfer(ty::InferTy::CanonicalTy(index)) = result_value.sty {
// in which case we would set `canonical_vars[0]` to `Some(?U)`.
opt_values[index] = Some(original_value);
opt_values[index] = Some(*original_value);
}
}
UnpackedKind::Lifetime(result_value) => {
// e.g., here `result_value` might be `'?1` in the example above...
if let &ty::RegionKind::ReCanonical(index) = result_value {
// in which case we would set `canonical_vars[0]` to `Some('static)`.
opt_values[index] = Some(original_value);
opt_values[index] = Some(*original_value);
}
}
}
Expand Down Expand Up @@ -459,7 +460,7 @@ impl<'cx, 'gcx, 'tcx> InferCtxt<'cx, 'gcx, 'tcx> {
&self,
cause: &ObligationCause<'tcx>,
param_env: ty::ParamEnv<'tcx>,
original_values: &CanonicalVarValues<'tcx>,
original_values: &SmallCanonicalVarValues<'tcx>,
result_subst: &CanonicalVarValues<'tcx>,
query_result: &Canonical<'tcx, QueryResult<'tcx, R>>,
) -> InferResult<'tcx, ()>
Expand Down Expand Up @@ -522,13 +523,13 @@ impl<'cx, 'gcx, 'tcx> InferCtxt<'cx, 'gcx, 'tcx> {
&self,
cause: &ObligationCause<'tcx>,
param_env: ty::ParamEnv<'tcx>,
variables1: &CanonicalVarValues<'tcx>,
variables1: &SmallCanonicalVarValues<'tcx>,
variables2: impl Fn(CanonicalVar) -> Kind<'tcx>,
) -> InferResult<'tcx, ()> {
self.commit_if_ok(|_| {
let mut obligations = vec![];
for (index, value1) in variables1.var_values.iter_enumerated() {
let value2 = variables2(index);
for (index, value1) in variables1.iter().enumerate() {
let value2 = variables2(CanonicalVar::new(index));

match (value1.unpack(), value2.unpack()) {
(UnpackedKind::Type(v1), UnpackedKind::Type(v2)) => {
Expand Down
2 changes: 1 addition & 1 deletion src/librustc/infer/canonical/substitute.rs
Expand Up @@ -46,7 +46,7 @@ impl<'tcx, V> Canonical<'tcx, V> {
where
T: TypeFoldable<'tcx>,
{
assert_eq!(self.variables.len(), var_values.var_values.len());
assert_eq!(self.variables.len(), var_values.len());
let value = projection_fn(&self.value);
substitute_value(tcx, var_values, value)
}
Expand Down
4 changes: 3 additions & 1 deletion src/librustc/traits/query/dropck_outlives.rs
Expand Up @@ -10,6 +10,7 @@

use infer::at::At;
use infer::InferOk;
use rustc_data_structures::small_vec::SmallVec;
use std::iter::FromIterator;
use syntax::codemap::Span;
use ty::subst::Kind;
Expand Down Expand Up @@ -50,7 +51,8 @@ impl<'cx, 'gcx, 'tcx> At<'cx, 'gcx, 'tcx> {
}

let gcx = tcx.global_tcx();
let (c_ty, orig_values) = self.infcx.canonicalize_query(&self.param_env.and(ty));
let mut orig_values = SmallVec::new();
let c_ty = self.infcx.canonicalize_query(&self.param_env.and(ty), &mut orig_values);
let span = self.cause.span;
debug!("c_ty = {:?}", c_ty);
match &gcx.dropck_outlives(c_ty) {
Expand Down
6 changes: 4 additions & 2 deletions src/librustc/traits/query/evaluate_obligation.rs
Expand Up @@ -9,6 +9,7 @@
// except according to those terms.

use infer::InferCtxt;
use rustc_data_structures::small_vec::SmallVec;
use traits::{EvaluationResult, PredicateObligation, SelectionContext,
TraitQueryMode, OverflowError};

Expand Down Expand Up @@ -38,8 +39,9 @@ impl<'cx, 'gcx, 'tcx> InferCtxt<'cx, 'gcx, 'tcx> {
&self,
obligation: &PredicateObligation<'tcx>,
) -> EvaluationResult {
let (c_pred, _) =
self.canonicalize_query(&obligation.param_env.and(obligation.predicate));
let mut _orig_values = SmallVec::new();
let c_pred = self.canonicalize_query(&obligation.param_env.and(obligation.predicate),
&mut _orig_values);
// Run canonical query. If overflow occurs, rerun from scratch but this time
// in standard trait query mode so that overflow is handled appropriately
// within `SelectionContext`.
Expand Down

0 comments on commit f686885

Please sign in to comment.