Skip to content

Commit

Permalink
Auto merge of #64627 - nnethercote:ObligForest-even-more, r=nikomatsakis
Browse files Browse the repository at this point in the history 
Even more `ObligationForest` improvements

Following on from #64545, more speed and readability improvements.

r? @nikomatsakis
  • Loading branch information
@bors
bors committed Sep 25, 2019
2 parents b7820b2 + aa10abb commit c7bc0bf
Show file tree
Hide file tree
Showing 6 changed files with 85 additions and 60 deletions.
4 changes: 2 additions & 2 deletions Cargo.lock
View file
Original file line number Diff line number Diff line change
Expand Up @@ -917,9 +917,9 @@ dependencies = [

[[package]]
name = "ena"
version = "0.13.0"
version = "0.13.1"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "3dc01d68e08ca384955a3aeba9217102ca1aa85b6e168639bf27739f1d749d87"
checksum = "8944dc8fa28ce4a38f778bd46bf7d923fe73eed5a439398507246c8e017e6f36"
dependencies = [
"log",
]
Expand Down
9 changes: 5 additions & 4 deletions src/librustc/infer/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1600,29 +1600,30 @@ impl<'a, 'tcx> ShallowResolver<'a, 'tcx> {

// `resolver.shallow_resolve_changed(ty)` is equivalent to
// `resolver.shallow_resolve(ty) != ty`, but more efficient. It's always
// inlined, despite being large, because it has a single call site that is
// extremely hot.
// inlined, despite being large, because it has only two call sites that
// are extremely hot.
#[inline(always)]
pub fn shallow_resolve_changed(&mut self, typ: Ty<'tcx>) -> bool {
match typ.sty {
ty::Infer(ty::TyVar(v)) => {
use self::type_variable::TypeVariableValue;

// See the comment in `shallow_resolve()`.
match self.infcx.type_variables.borrow_mut().probe(v) {
match self.infcx.type_variables.borrow_mut().inlined_probe(v) {
TypeVariableValue::Known { value: t } => self.fold_ty(t) != typ,
TypeVariableValue::Unknown { .. } => false,
}
}

ty::Infer(ty::IntVar(v)) => {
match self.infcx.int_unification_table.borrow_mut().probe_value(v) {
match self.infcx.int_unification_table.borrow_mut().inlined_probe_value(v) {
Some(v) => v.to_type(self.infcx.tcx) != typ,
None => false,
}
}

ty::Infer(ty::FloatVar(v)) => {
// Not `inlined_probe_value(v)` because this call site is colder.
match self.infcx.float_unification_table.borrow_mut().probe_value(v) {
Some(v) => v.to_type(self.infcx.tcx) != typ,
None => false,
Expand Down
8 changes: 7 additions & 1 deletion src/librustc/infer/type_variable.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,13 @@ impl<'tcx> TypeVariableTable<'tcx> {
/// Retrieves the type to which `vid` has been instantiated, if
/// any.
pub fn probe(&mut self, vid: ty::TyVid) -> TypeVariableValue<'tcx> {
self.eq_relations.probe_value(vid)
self.inlined_probe(vid)
}

/// An always-inlined variant of `probe`, for very hot call sites.
#[inline(always)]
pub fn inlined_probe(&mut self, vid: ty::TyVid) -> TypeVariableValue<'tcx> {
self.eq_relations.inlined_probe_value(vid)
}

/// If `t` is a type-inference variable, and it has been
Expand Down
55 changes: 36 additions & 19 deletions src/librustc/traits/fulfill.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -256,29 +256,46 @@ impl<'a, 'b, 'tcx> ObligationProcessor for FulfillProcessor<'a, 'b, 'tcx> {
&mut self,
pending_obligation: &mut Self::Obligation,
) -> ProcessResult<Self::Obligation, Self::Error> {
// If we were stalled on some unresolved variables, first check
// whether any of them have been resolved; if not, don't bother
// doing more work yet
if !pending_obligation.stalled_on.is_empty() {
let mut changed = false;
// This `for` loop was once a call to `all()`, but this lower-level
// form was a perf win. See #64545 for details.
for &ty in &pending_obligation.stalled_on {
if ShallowResolver::new(self.selcx.infcx()).shallow_resolve_changed(ty) {
changed = true;
break;
}
// If we were stalled on some unresolved variables, first check whether
// any of them have been resolved; if not, don't bother doing more work
// yet.
let change = match pending_obligation.stalled_on.len() {
// Match arms are in order of frequency, which matters because this
// code is so hot. 1 and 0 dominate; 2+ is fairly rare.
1 => {
let ty = pending_obligation.stalled_on[0];
ShallowResolver::new(self.selcx.infcx()).shallow_resolve_changed(ty)
}
0 => {
// In this case we haven't changed, but wish to make a change.
true
}
if !changed {
debug!("process_predicate: pending obligation {:?} still stalled on {:?}",
self.selcx.infcx()
.resolve_vars_if_possible(&pending_obligation.obligation),
pending_obligation.stalled_on);
return ProcessResult::Unchanged;
_ => {
// This `for` loop was once a call to `all()`, but this lower-level
// form was a perf win. See #64545 for details.
(|| {
for &ty in &pending_obligation.stalled_on {
if ShallowResolver::new(self.selcx.infcx()).shallow_resolve_changed(ty) {
return true;
}
}
false
})()
}
pending_obligation.stalled_on = vec![];
};

if !change {
debug!("process_predicate: pending obligation {:?} still stalled on {:?}",
self.selcx.infcx()
.resolve_vars_if_possible(&pending_obligation.obligation),
pending_obligation.stalled_on);
return ProcessResult::Unchanged;
}

// This part of the code is much colder.

pending_obligation.stalled_on.truncate(0);

let obligation = &mut pending_obligation.obligation;

if obligation.predicate.has_infer_types() {
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_data_structures/Cargo.toml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ path = "lib.rs"
doctest = false

[dependencies]
ena = "0.13"
ena = "0.13.1"
indexmap = "1"
log = "0.4"
jobserver_crate = { version = "0.1.13", package = "jobserver" }
Expand Down
67 changes: 34 additions & 33 deletions src/librustc_data_structures/obligation_forest/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ pub struct ObligationForest<O: ForestObligation> {
/// A cache of the nodes in `nodes`, indexed by predicate. Unfortunately,
/// its contents are not guaranteed to match those of `nodes`. See the
/// comments in `process_obligation` for details.
waiting_cache: FxHashMap<O::Predicate, usize>,
active_cache: FxHashMap<O::Predicate, usize>,

/// A scratch vector reused in various operations, to avoid allocating new
/// vectors.
Expand Down Expand Up @@ -278,7 +278,7 @@ impl<O: ForestObligation> ObligationForest<O> {
ObligationForest {
nodes: vec![],
done_cache: Default::default(),
waiting_cache: Default::default(),
active_cache: Default::default(),
scratch: RefCell::new(vec![]),
obligation_tree_id_generator: (0..).map(ObligationTreeId),
error_cache: Default::default(),
Expand All @@ -303,15 +303,15 @@ impl<O: ForestObligation> ObligationForest<O> {
return Ok(());
}

match self.waiting_cache.entry(obligation.as_predicate().clone()) {
match self.active_cache.entry(obligation.as_predicate().clone()) {
Entry::Occupied(o) => {
debug!("register_obligation_at({:?}, {:?}) - duplicate of {:?}!",
obligation, parent, o.get());
let node = &mut self.nodes[*o.get()];
if let Some(parent_index) = parent {
// If the node is already in `waiting_cache`, it has
// already had its chance to be marked with a parent. So if
// it's not already present, just dump `parent` into the
// If the node is already in `active_cache`, it has already
// had its chance to be marked with a parent. So if it's
// not already present, just dump `parent` into the
// dependents as a non-parent.
if !node.dependents.contains(&parent_index) {
node.dependents.push(parent_index);
Expand Down Expand Up @@ -355,10 +355,9 @@ impl<O: ForestObligation> ObligationForest<O> {
let mut errors = vec![];
for (index, node) in self.nodes.iter().enumerate() {
if let NodeState::Pending = node.state.get() {
let backtrace = self.error_at(index);
errors.push(Error {
error: error.clone(),
backtrace,
backtrace: self.error_at(index),
});
}
}
Expand Down Expand Up @@ -406,8 +405,8 @@ impl<O: ForestObligation> ObligationForest<O> {

// `processor.process_obligation` can modify the predicate within
// `node.obligation`, and that predicate is the key used for
// `self.waiting_cache`. This means that `self.waiting_cache` can
// get out of sync with `nodes`. It's not very common, but it does
// `self.active_cache`. This means that `self.active_cache` can get
// out of sync with `nodes`. It's not very common, but it does
// happen, and code in `compress` has to allow for it.
let result = match node.state.get() {
NodeState::Pending => processor.process_obligation(&mut node.obligation),
Expand Down Expand Up @@ -439,10 +438,9 @@ impl<O: ForestObligation> ObligationForest<O> {
}
ProcessResult::Error(err) => {
stalled = false;
let backtrace = self.error_at(index);
errors.push(Error {
error: err,
backtrace,
backtrace: self.error_at(index),
});
}
}
Expand Down Expand Up @@ -484,13 +482,16 @@ impl<O: ForestObligation> ObligationForest<O> {
debug!("process_cycles()");

for (index, node) in self.nodes.iter().enumerate() {
// For rustc-benchmarks/inflate-0.1.0 this state test is extremely
// hot and the state is almost always `Pending` or `Waiting`. It's
// a win to handle the no-op cases immediately to avoid the cost of
// the function call.
// For some benchmarks this state test is extremely
// hot. It's a win to handle the no-op cases immediately to avoid
// the cost of the function call.
match node.state.get() {
NodeState::Waiting | NodeState::Pending | NodeState::Done | NodeState::Error => {},
_ => self.find_cycles_from_node(&mut stack, processor, index),
// Match arms are in order of frequency. Pending, Success and
// Waiting dominate; the others are rare.
NodeState::Pending => {},
NodeState::Success => self.find_cycles_from_node(&mut stack, processor, index),
NodeState::Waiting | NodeState::Done | NodeState::Error => {},
NodeState::OnDfsStack => self.find_cycles_from_node(&mut stack, processor, index),
}
}

Expand All @@ -506,8 +507,8 @@ impl<O: ForestObligation> ObligationForest<O> {
let node = &self.nodes[index];
match node.state.get() {
NodeState::OnDfsStack => {
let index = stack.iter().rposition(|&n| n == index).unwrap();
processor.process_backedge(stack[index..].iter().map(GetObligation(&self.nodes)),
let rpos = stack.iter().rposition(|&n| n == index).unwrap();
processor.process_backedge(stack[rpos..].iter().map(GetObligation(&self.nodes)),
PhantomData);
}
NodeState::Success => {
Expand Down Expand Up @@ -636,11 +637,11 @@ impl<O: ForestObligation> ObligationForest<O> {
}
NodeState::Done => {
// This lookup can fail because the contents of
// `self.waiting_cache` is not guaranteed to match those of
// `self.active_cache` is not guaranteed to match those of
// `self.nodes`. See the comment in `process_obligation`
// for more details.
if let Some((predicate, _)) = self.waiting_cache
.remove_entry(node.obligation.as_predicate())
if let Some((predicate, _)) =
self.active_cache.remove_entry(node.obligation.as_predicate())
{
self.done_cache.insert(predicate);
} else {
Expand All @@ -653,7 +654,7 @@ impl<O: ForestObligation> ObligationForest<O> {
// We *intentionally* remove the node from the cache at this point. Otherwise
// tests must come up with a different type on every type error they
// check against.
self.waiting_cache.remove(node.obligation.as_predicate());
self.active_cache.remove(node.obligation.as_predicate());
node_rewrites[index] = nodes_len;
dead_nodes += 1;
self.insert_into_error_cache(index);
Expand Down Expand Up @@ -697,25 +698,25 @@ impl<O: ForestObligation> ObligationForest<O> {
let nodes_len = node_rewrites.len();

for node in &mut self.nodes {
let mut index = 0;
while index < node.dependents.len() {
let new_index = node_rewrites[node.dependents[index]];
let mut i = 0;
while i < node.dependents.len() {
let new_index = node_rewrites[node.dependents[i]];
if new_index >= nodes_len {
node.dependents.swap_remove(index);
if index == 0 && node.has_parent {
node.dependents.swap_remove(i);
if i == 0 && node.has_parent {
// We just removed the parent.
node.has_parent = false;
}
} else {
node.dependents[index] = new_index;
index += 1;
node.dependents[i] = new_index;
i += 1;
}
}
}

// This updating of `self.waiting_cache` is necessary because the
// This updating of `self.active_cache` is necessary because the
// removal of nodes within `compress` can fail. See above.
self.waiting_cache.retain(|_predicate, index| {
self.active_cache.retain(|_predicate, index| {
let new_index = node_rewrites[*index];
if new_index >= nodes_len {
false
Expand Down

0 comments on commit c7bc0bf

Please sign in to comment.