From 04b1111ae8dd97f3cf558654015b8101d270c634 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Mon, 16 Sep 2019 11:49:15 +1000 Subject: [PATCH 01/11] Name index variables consistently. Those with type `usize` are now called `i`, those with type `NodeIndex` are called `index`. --- .../obligation_forest/mod.rs | 97 +++++++++---------- 1 file changed, 47 insertions(+), 50 deletions(-) diff --git a/src/librustc_data_structures/obligation_forest/mod.rs b/src/librustc_data_structures/obligation_forest/mod.rs index 6c52e626ababd..7ef1953e2d812 100644 --- a/src/librustc_data_structures/obligation_forest/mod.rs +++ b/src/librustc_data_structures/obligation_forest/mod.rs @@ -295,14 +295,15 @@ impl ObligationForest { debug!("register_obligation_at({:?}, {:?}) - duplicate of {:?}!", obligation, parent, o.get()); let node = &mut self.nodes[o.get().get()]; - if let Some(parent) = parent { + if let Some(parent_index) = parent { // If the node is already in `waiting_cache`, it's already // been marked with a parent. (It's possible that parent // has been cleared by `apply_rewrites`, though.) So just // dump `parent` into `node.dependents`... unless it's // already in `node.dependents` or `node.parent`. - if !node.dependents.contains(&parent) && Some(parent) != node.parent { - node.dependents.push(parent); + if !node.dependents.contains(&parent_index) && + Some(parent_index) != node.parent { + node.dependents.push(parent_index); } } if let NodeState::Error = node.state.get() { @@ -316,9 +317,8 @@ impl ObligationForest { obligation, parent, self.nodes.len()); let obligation_tree_id = match parent { - Some(p) => { - let parent_node = &self.nodes[p.get()]; - parent_node.obligation_tree_id + Some(parent_index) => { + self.nodes[parent_index.get()].obligation_tree_id } None => self.obligation_tree_id_generator.next().unwrap() }; @@ -346,9 +346,9 @@ impl ObligationForest { /// This cannot be done during a snapshot. pub fn to_errors(&mut self, error: E) -> Vec> { let mut errors = vec![]; - for index in 0..self.nodes.len() { - if let NodeState::Pending = self.nodes[index].state.get() { - let backtrace = self.error_at(index); + for i in 0..self.nodes.len() { + if let NodeState::Pending = self.nodes[i].state.get() { + let backtrace = self.error_at(i); errors.push(Error { error: error.clone(), backtrace, @@ -393,16 +393,16 @@ impl ObligationForest { let mut errors = vec![]; let mut stalled = true; - for index in 0..self.nodes.len() { - debug!("process_obligations: node {} == {:?}", index, self.nodes[index]); + for i in 0..self.nodes.len() { + debug!("process_obligations: node {} == {:?}", i, self.nodes[i]); - let result = match self.nodes[index] { + let result = match self.nodes[i] { Node { ref state, ref mut obligation, .. } if state.get() == NodeState::Pending => processor.process_obligation(obligation), _ => continue }; - debug!("process_obligations: node {} got result {:?}", index, result); + debug!("process_obligations: node {} got result {:?}", i, result); match result { ProcessResult::Unchanged => { @@ -411,23 +411,23 @@ impl ObligationForest { ProcessResult::Changed(children) => { // We are not (yet) stalled. stalled = false; - self.nodes[index].state.set(NodeState::Success); + self.nodes[i].state.set(NodeState::Success); for child in children { let st = self.register_obligation_at( child, - Some(NodeIndex::new(index)) + Some(NodeIndex::new(i)) ); if let Err(()) = st { // error already reported - propagate it // to our node. - self.error_at(index); + self.error_at(i); } } } ProcessResult::Error(err) => { stalled = false; - let backtrace = self.error_at(index); + let backtrace = self.error_at(i); errors.push(Error { error: err, backtrace, @@ -473,15 +473,15 @@ impl ObligationForest { debug!("process_cycles()"); - for index in 0..self.nodes.len() { + for i in 0..self.nodes.len() { // 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. - let state = self.nodes[index].state.get(); + let state = self.nodes[i].state.get(); match state { NodeState::Waiting | NodeState::Pending | NodeState::Done | NodeState::Error => {}, - _ => self.find_cycles_from_node(&mut stack, processor, index), + _ => self.find_cycles_from_node(&mut stack, processor, i), } } @@ -491,24 +491,22 @@ impl ObligationForest { self.scratch = Some(stack); } - fn find_cycles_from_node

(&self, stack: &mut Vec, - processor: &mut P, index: usize) + fn find_cycles_from_node

(&self, stack: &mut Vec, processor: &mut P, i: usize) where P: ObligationProcessor { - let node = &self.nodes[index]; + let node = &self.nodes[i]; let state = node.state.get(); match state { NodeState::OnDfsStack => { - let index = - stack.iter().rposition(|n| *n == index).unwrap(); - processor.process_backedge(stack[index..].iter().map(GetObligation(&self.nodes)), + let i = stack.iter().rposition(|n| *n == i).unwrap(); + processor.process_backedge(stack[i..].iter().map(GetObligation(&self.nodes)), PhantomData); } NodeState::Success => { node.state.set(NodeState::OnDfsStack); - stack.push(index); - for dependent in node.parent.iter().chain(node.dependents.iter()) { - self.find_cycles_from_node(stack, processor, dependent.get()); + stack.push(i); + for index in node.parent.iter().chain(node.dependents.iter()) { + self.find_cycles_from_node(stack, processor, index.get()); } stack.pop(); node.state.set(NodeState::Done); @@ -525,33 +523,32 @@ impl ObligationForest { /// Returns a vector of obligations for `p` and all of its /// ancestors, putting them into the error state in the process. - fn error_at(&mut self, p: usize) -> Vec { + fn error_at(&mut self, mut i: usize) -> Vec { let mut error_stack = self.scratch.take().unwrap(); let mut trace = vec![]; - let mut n = p; loop { - self.nodes[n].state.set(NodeState::Error); - trace.push(self.nodes[n].obligation.clone()); - error_stack.extend(self.nodes[n].dependents.iter().map(|x| x.get())); + let node = &self.nodes[i]; + node.state.set(NodeState::Error); + trace.push(node.obligation.clone()); + error_stack.extend(node.dependents.iter().map(|index| index.get())); - // loop to the parent - match self.nodes[n].parent { - Some(q) => n = q.get(), + // Loop to the parent. + match node.parent { + Some(parent_index) => i = parent_index.get(), None => break } } while let Some(i) = error_stack.pop() { - match self.nodes[i].state.get() { + let node = &self.nodes[i]; + match node.state.get() { NodeState::Error => continue, _ => self.nodes[i].state.set(NodeState::Error), } - let node = &self.nodes[i]; - error_stack.extend( - node.parent.iter().chain(node.dependents.iter()).map(|x| x.get()) + node.parent.iter().chain(node.dependents.iter()).map(|index| index.get()) ); } @@ -689,22 +686,22 @@ impl ObligationForest { for node in &mut self.nodes { if let Some(index) = node.parent { - let new_index = node_rewrites[index.get()]; - if new_index >= nodes_len { + let new_i = node_rewrites[index.get()]; + if new_i >= nodes_len { // parent dead due to error node.parent = None; } else { - node.parent = Some(NodeIndex::new(new_index)); + node.parent = Some(NodeIndex::new(new_i)); } } let mut i = 0; while i < node.dependents.len() { - let new_index = node_rewrites[node.dependents[i].get()]; - if new_index >= nodes_len { + let new_i = node_rewrites[node.dependents[i].get()]; + if new_i >= nodes_len { node.dependents.swap_remove(i); } else { - node.dependents[i] = NodeIndex::new(new_index); + node.dependents[i] = NodeIndex::new(new_i); i += 1; } } @@ -712,11 +709,11 @@ impl ObligationForest { let mut kill_list = vec![]; for (predicate, index) in &mut self.waiting_cache { - let new_index = node_rewrites[index.get()]; - if new_index >= nodes_len { + let new_i = node_rewrites[index.get()]; + if new_i >= nodes_len { kill_list.push(predicate.clone()); } else { - *index = NodeIndex::new(new_index); + *index = NodeIndex::new(new_i); } } From 43c0d2ce8eae322e0b1ffe945e356e30c808dbb3 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Mon, 16 Sep 2019 11:53:12 +1000 Subject: [PATCH 02/11] Redefine `NodeIndex` as a `newtype_index!`. This commit removes the custom index implementation of `NodeIndex`, which probably predates `newtype_index!`. As well as eliminating code, it improves the debugging experience, because the custom implementation had the property of being incremented by 1 (so it could use `NonZeroU32`), which was incredibly confusing if you didn't expect it. For some reason, I also had to remove an `unsafe` block marker from `from_u32_unchecked()` that the compiler said was now unnecessary. --- src/librustc_data_structures/indexed_vec.rs | 2 +- .../obligation_forest/graphviz.rs | 6 +-- .../obligation_forest/mod.rs | 49 ++++++++++++------- .../obligation_forest/node_index.rs | 20 -------- 4 files changed, 35 insertions(+), 42 deletions(-) delete mode 100644 src/librustc_data_structures/obligation_forest/node_index.rs diff --git a/src/librustc_data_structures/indexed_vec.rs b/src/librustc_data_structures/indexed_vec.rs index 6f40d059be27f..6e80b48a68560 100644 --- a/src/librustc_data_structures/indexed_vec.rs +++ b/src/librustc_data_structures/indexed_vec.rs @@ -149,7 +149,7 @@ macro_rules! newtype_index { #[inline] $v const unsafe fn from_u32_unchecked(value: u32) -> Self { - unsafe { $type { private: value } } + $type { private: value } } /// Extracts the value of this index as an integer. diff --git a/src/librustc_data_structures/obligation_forest/graphviz.rs b/src/librustc_data_structures/obligation_forest/graphviz.rs index a0363e165e049..b2120b182fa7b 100644 --- a/src/librustc_data_structures/obligation_forest/graphviz.rs +++ b/src/librustc_data_structures/obligation_forest/graphviz.rs @@ -74,9 +74,9 @@ impl<'a, O: ForestObligation + 'a> dot::GraphWalk<'a> for &'a ObligationForest { /// At all times we maintain the invariant that every node appears /// at a higher index than its parent. This is needed by the /// backtrace iterator (which uses `split_at`). + /// + /// Ideally, this would be an `IndexVec>`. But that is + /// slower, because this vector is accessed so often that the + /// `u32`-to-`usize` conversions required for accesses are significant. nodes: Vec>, /// A cache of predicates that have been successfully completed. @@ -178,13 +185,19 @@ struct Node { obligation: O, state: Cell, - /// The parent of a node - the original obligation of - /// which it is a subobligation. Except for error reporting, - /// it is just like any member of `dependents`. + /// The parent of a node - the original obligation of which it is a + /// subobligation. Except for error reporting, it is just like any member + /// of `dependents`. + /// + /// Unlike `ObligationForest::nodes`, this uses `NodeIndex` rather than + /// `usize` for the index, because keeping the size down is more important + /// than the cost of converting to a `usize` for indexing. parent: Option, - /// Obligations that depend on this obligation for their - /// completion. They must all be in a non-pending state. + /// Obligations that depend on this obligation for their completion. They + /// must all be in a non-pending state. + /// + /// This uses `NodeIndex` for the same reason as `parent`. dependents: Vec, /// Identifier of the obligation tree to which this node belongs. @@ -294,7 +307,7 @@ impl ObligationForest { Entry::Occupied(o) => { debug!("register_obligation_at({:?}, {:?}) - duplicate of {:?}!", obligation, parent, o.get()); - let node = &mut self.nodes[o.get().get()]; + let node = &mut self.nodes[o.get().index()]; if let Some(parent_index) = parent { // If the node is already in `waiting_cache`, it's already // been marked with a parent. (It's possible that parent @@ -318,7 +331,7 @@ impl ObligationForest { let obligation_tree_id = match parent { Some(parent_index) => { - self.nodes[parent_index.get()].obligation_tree_id + self.nodes[parent_index.index()].obligation_tree_id } None => self.obligation_tree_id_generator.next().unwrap() }; @@ -506,7 +519,7 @@ impl ObligationForest { node.state.set(NodeState::OnDfsStack); stack.push(i); for index in node.parent.iter().chain(node.dependents.iter()) { - self.find_cycles_from_node(stack, processor, index.get()); + self.find_cycles_from_node(stack, processor, index.index()); } stack.pop(); node.state.set(NodeState::Done); @@ -531,11 +544,11 @@ impl ObligationForest { let node = &self.nodes[i]; node.state.set(NodeState::Error); trace.push(node.obligation.clone()); - error_stack.extend(node.dependents.iter().map(|index| index.get())); + error_stack.extend(node.dependents.iter().map(|index| index.index())); // Loop to the parent. match node.parent { - Some(parent_index) => i = parent_index.get(), + Some(parent_index) => i = parent_index.index(), None => break } } @@ -548,7 +561,7 @@ impl ObligationForest { } error_stack.extend( - node.parent.iter().chain(node.dependents.iter()).map(|index| index.get()) + node.parent.iter().chain(node.dependents.iter()).map(|index| index.index()) ); } @@ -560,7 +573,7 @@ impl ObligationForest { #[inline(always)] fn inlined_mark_neighbors_as_waiting_from(&self, node: &Node) { for dependent in node.parent.iter().chain(node.dependents.iter()) { - self.mark_as_waiting_from(&self.nodes[dependent.get()]); + self.mark_as_waiting_from(&self.nodes[dependent.index()]); } } @@ -686,7 +699,7 @@ impl ObligationForest { for node in &mut self.nodes { if let Some(index) = node.parent { - let new_i = node_rewrites[index.get()]; + let new_i = node_rewrites[index.index()]; if new_i >= nodes_len { // parent dead due to error node.parent = None; @@ -697,7 +710,7 @@ impl ObligationForest { let mut i = 0; while i < node.dependents.len() { - let new_i = node_rewrites[node.dependents[i].get()]; + let new_i = node_rewrites[node.dependents[i].index()]; if new_i >= nodes_len { node.dependents.swap_remove(i); } else { @@ -709,7 +722,7 @@ impl ObligationForest { let mut kill_list = vec![]; for (predicate, index) in &mut self.waiting_cache { - let new_i = node_rewrites[index.get()]; + let new_i = node_rewrites[index.index()]; if new_i >= nodes_len { kill_list.push(predicate.clone()); } else { diff --git a/src/librustc_data_structures/obligation_forest/node_index.rs b/src/librustc_data_structures/obligation_forest/node_index.rs deleted file mode 100644 index 69ea473e05461..0000000000000 --- a/src/librustc_data_structures/obligation_forest/node_index.rs +++ /dev/null @@ -1,20 +0,0 @@ -use std::num::NonZeroU32; -use std::u32; - -#[derive(Copy, Clone, Debug, PartialEq, Eq)] -pub struct NodeIndex { - index: NonZeroU32, -} - -impl NodeIndex { - #[inline] - pub fn new(value: usize) -> NodeIndex { - assert!(value < (u32::MAX as usize)); - NodeIndex { index: NonZeroU32::new((value as u32) + 1).unwrap() } - } - - #[inline] - pub fn get(self) -> usize { - (self.index.get() - 1) as usize - } -} From 1936e44c13753cdd060c3a33c4a1cc27443d52fc Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Mon, 16 Sep 2019 11:56:17 +1000 Subject: [PATCH 03/11] Factor out repeated `self.nodes[i]` expressions. --- .../obligation_forest/mod.rs | 29 +++++++++---------- 1 file changed, 14 insertions(+), 15 deletions(-) diff --git a/src/librustc_data_structures/obligation_forest/mod.rs b/src/librustc_data_structures/obligation_forest/mod.rs index cfc3b8194a65c..595323da02fd1 100644 --- a/src/librustc_data_structures/obligation_forest/mod.rs +++ b/src/librustc_data_structures/obligation_forest/mod.rs @@ -386,7 +386,6 @@ impl ObligationForest { fn insert_into_error_cache(&mut self, node_index: usize) { let node = &self.nodes[node_index]; - self.error_cache .entry(node.obligation_tree_id) .or_default() @@ -407,11 +406,12 @@ impl ObligationForest { let mut stalled = true; for i in 0..self.nodes.len() { - debug!("process_obligations: node {} == {:?}", i, self.nodes[i]); + let node = &mut self.nodes[i]; + + debug!("process_obligations: node {} == {:?}", i, node); - let result = match self.nodes[i] { - Node { ref state, ref mut obligation, .. } if state.get() == NodeState::Pending => - processor.process_obligation(obligation), + let result = match node.state.get() { + NodeState::Pending => processor.process_obligation(&mut node.obligation), _ => continue }; @@ -424,7 +424,7 @@ impl ObligationForest { ProcessResult::Changed(children) => { // We are not (yet) stalled. stalled = false; - self.nodes[i].state.set(NodeState::Success); + node.state.set(NodeState::Success); for child in children { let st = self.register_obligation_at( @@ -491,8 +491,7 @@ impl ObligationForest { // 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. - let state = self.nodes[i].state.get(); - match state { + match self.nodes[i].state.get() { NodeState::Waiting | NodeState::Pending | NodeState::Done | NodeState::Error => {}, _ => self.find_cycles_from_node(&mut stack, processor, i), } @@ -508,8 +507,7 @@ impl ObligationForest { where P: ObligationProcessor { let node = &self.nodes[i]; - let state = node.state.get(); - match state { + match node.state.get() { NodeState::OnDfsStack => { let i = stack.iter().rposition(|n| *n == i).unwrap(); processor.process_backedge(stack[i..].iter().map(GetObligation(&self.nodes)), @@ -557,7 +555,7 @@ impl ObligationForest { let node = &self.nodes[i]; match node.state.get() { NodeState::Error => continue, - _ => self.nodes[i].state.set(NodeState::Error), + _ => node.state.set(NodeState::Error), } error_stack.extend( @@ -630,7 +628,8 @@ impl ObligationForest { // self.nodes[i - dead_nodes..i] are all dead // self.nodes[i..] are unchanged for i in 0..self.nodes.len() { - match self.nodes[i].state.get() { + let node = &self.nodes[i]; + match node.state.get() { NodeState::Pending | NodeState::Waiting => { if dead_nodes > 0 { self.nodes.swap(i, i - dead_nodes); @@ -640,11 +639,11 @@ impl ObligationForest { NodeState::Done => { // Avoid cloning the key (predicate) in case it exists in the waiting cache if let Some((predicate, _)) = self.waiting_cache - .remove_entry(self.nodes[i].obligation.as_predicate()) + .remove_entry(node.obligation.as_predicate()) { self.done_cache.insert(predicate); } else { - self.done_cache.insert(self.nodes[i].obligation.as_predicate().clone()); + self.done_cache.insert(node.obligation.as_predicate().clone()); } node_rewrites[i] = nodes_len; dead_nodes += 1; @@ -653,7 +652,7 @@ impl ObligationForest { // 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(self.nodes[i].obligation.as_predicate()); + self.waiting_cache.remove(node.obligation.as_predicate()); node_rewrites[i] = nodes_len; dead_nodes += 1; self.insert_into_error_cache(i); From 3fda9578e08680b00db87c031940c907e54d1cc3 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Mon, 16 Sep 2019 12:38:24 +1000 Subject: [PATCH 04/11] Remove out-of-date comments. These refer to code that no longer exists. --- .../obligation_forest/mod.rs | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/src/librustc_data_structures/obligation_forest/mod.rs b/src/librustc_data_structures/obligation_forest/mod.rs index 595323da02fd1..731d82b7cfd72 100644 --- a/src/librustc_data_structures/obligation_forest/mod.rs +++ b/src/librustc_data_structures/obligation_forest/mod.rs @@ -61,14 +61,6 @@ //! results. This is used by the `FulfillmentContext` to decide when it //! has reached a steady state. //! -//! #### Snapshots -//! -//! The `ObligationForest` supports a limited form of snapshots; see -//! `start_snapshot`, `commit_snapshot`, and `rollback_snapshot`. In -//! particular, you can use a snapshot to roll back new root -//! obligations. However, it is an error to attempt to -//! `process_obligations` during a snapshot. -//! //! ### Implementation details //! //! For the most part, comments specific to the implementation are in the @@ -151,10 +143,6 @@ pub struct ObligationForest { /// At the end of processing, those nodes will be removed by a /// call to `compress`. /// - /// At all times we maintain the invariant that every node appears - /// at a higher index than its parent. This is needed by the - /// backtrace iterator (which uses `split_at`). - /// /// Ideally, this would be an `IndexVec>`. But that is /// slower, because this vector is accessed so often that the /// `u32`-to-`usize` conversions required for accesses are significant. @@ -288,8 +276,6 @@ impl ObligationForest { } /// Registers an obligation. - /// - /// This CAN be done in a snapshot pub fn register_obligation(&mut self, obligation: O) { // Ignore errors here - there is no guarantee of success. let _ = self.register_obligation_at(obligation, None); @@ -355,8 +341,6 @@ impl ObligationForest { } /// Converts all remaining obligations to the given error. - /// - /// This cannot be done during a snapshot. pub fn to_errors(&mut self, error: E) -> Vec> { let mut errors = vec![]; for i in 0..self.nodes.len() { From ac061dc5c8497c03e14c82b7fe349ca863924fae Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Mon, 16 Sep 2019 12:39:21 +1000 Subject: [PATCH 05/11] Fix some out-of-date names of things in comments. --- .../obligation_forest/mod.rs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/librustc_data_structures/obligation_forest/mod.rs b/src/librustc_data_structures/obligation_forest/mod.rs index 731d82b7cfd72..3bf65b5739afb 100644 --- a/src/librustc_data_structures/obligation_forest/mod.rs +++ b/src/librustc_data_structures/obligation_forest/mod.rs @@ -9,7 +9,7 @@ //! `ObligationForest` supports two main public operations (there are a //! few others not discussed here): //! -//! 1. Add a new root obligations (`push_tree`). +//! 1. Add a new root obligations (`register_obligation`). //! 2. Process the pending obligations (`process_obligations`). //! //! When a new obligation `N` is added, it becomes the root of an @@ -20,13 +20,13 @@ //! with every pending obligation (so that will include `N`, the first //! time). The callback also receives a (mutable) reference to the //! per-tree state `T`. The callback should process the obligation `O` -//! that it is given and return one of three results: +//! that it is given and return a `ProcessResult`: //! -//! - `Ok(None)` -> ambiguous result. Obligation was neither a success +//! - `Unchanged` -> ambiguous result. Obligation was neither a success //! nor a failure. It is assumed that further attempts to process the //! obligation will yield the same result unless something in the //! surrounding environment changes. -//! - `Ok(Some(C))` - the obligation was *shallowly successful*. The +//! - `Changed(C)` - the obligation was *shallowly successful*. The //! vector `C` is a list of subobligations. The meaning of this is that //! `O` was successful on the assumption that all the obligations in `C` //! are also successful. Therefore, `O` is only considered a "true" @@ -34,7 +34,7 @@ //! state and the obligations in `C` become the new pending //! obligations. They will be processed the next time you call //! `process_obligations`. -//! - `Err(E)` -> obligation failed with error `E`. We will collect this +//! - `Error(E)` -> obligation failed with error `E`. We will collect this //! error and return it from `process_obligations`, along with the //! "backtrace" of obligations (that is, the list of obligations up to //! and including the root of the failed obligation). No further @@ -47,14 +47,14 @@ //! - `completed`: a list of obligations where processing was fully //! completed without error (meaning that all transitive subobligations //! have also been completed). So, for example, if the callback from -//! `process_obligations` returns `Ok(Some(C))` for some obligation `O`, +//! `process_obligations` returns `Changed(C)` for some obligation `O`, //! then `O` will be considered completed right away if `C` is the //! empty vector. Otherwise it will only be considered completed once //! all the obligations in `C` have been found completed. //! - `errors`: a list of errors that occurred and associated backtraces //! at the time of error, which can be used to give context to the user. //! - `stalled`: if true, then none of the existing obligations were -//! *shallowly successful* (that is, no callback returned `Ok(Some(_))`). +//! *shallowly successful* (that is, no callback returned `Changed(_)`). //! This implies that all obligations were either errors or returned an //! ambiguous result, which means that any further calls to //! `process_obligations` would simply yield back further ambiguous From 6391ef4d6e627c87124d512892fe4e0eface96ae Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Mon, 16 Sep 2019 12:40:31 +1000 Subject: [PATCH 06/11] Fix incorrect comment about contents of a `Node`. --- src/librustc_data_structures/obligation_forest/mod.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/librustc_data_structures/obligation_forest/mod.rs b/src/librustc_data_structures/obligation_forest/mod.rs index 3bf65b5739afb..7904808eb38e3 100644 --- a/src/librustc_data_structures/obligation_forest/mod.rs +++ b/src/librustc_data_structures/obligation_forest/mod.rs @@ -66,11 +66,11 @@ //! For the most part, comments specific to the implementation are in the //! code. This file only contains a very high-level overview. Basically, //! the forest is stored in a vector. Each element of the vector is a node -//! in some tree. Each node in the vector has the index of an (optional) -//! parent and (for convenience) its root (which may be itself). It also -//! has a current state, described by `NodeState`. After each -//! processing step, we compress the vector to remove completed and error -//! nodes, which aren't needed anymore. +//! in some tree. Each node in the vector has the index of its dependents, +//! including the first dependent which is known as the parent. It also +//! has a current state, described by `NodeState`. After each processing +//! step, we compress the vector to remove completed and error nodes, which +//! aren't needed anymore. use crate::fx::{FxHashMap, FxHashSet}; use crate::indexed_vec::Idx; From e2492b716370dab5216f0123b3ed1cac78f8304e Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Mon, 16 Sep 2019 12:41:36 +1000 Subject: [PATCH 07/11] Add comments about `waiting_cache`. --- .../obligation_forest/mod.rs | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/src/librustc_data_structures/obligation_forest/mod.rs b/src/librustc_data_structures/obligation_forest/mod.rs index 7904808eb38e3..9dbae95dc8f60 100644 --- a/src/librustc_data_structures/obligation_forest/mod.rs +++ b/src/librustc_data_structures/obligation_forest/mod.rs @@ -151,7 +151,9 @@ pub struct ObligationForest { /// A cache of predicates that have been successfully completed. done_cache: FxHashSet, - /// An cache of the nodes in `nodes`, indexed by predicate. + /// 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, scratch: Option>, @@ -394,6 +396,11 @@ impl ObligationForest { debug!("process_obligations: node {} == {:?}", i, node); + // `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 + // happen, and code in `compress` has to allow for it. let result = match node.state.get() { NodeState::Pending => processor.process_obligation(&mut node.obligation), _ => continue @@ -621,7 +628,10 @@ impl ObligationForest { } } NodeState::Done => { - // Avoid cloning the key (predicate) in case it exists in the waiting cache + // This lookup can fail because the contents of + // `self.waiting_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()) { @@ -703,6 +713,8 @@ impl ObligationForest { } } + // This updating of `self.waiting_cache` is necessary because the + // removal of nodes within `compress` can fail. See above. let mut kill_list = vec![]; for (predicate, index) in &mut self.waiting_cache { let new_i = node_rewrites[index.index()]; From 6e48053d5d0d1c81d9a7e6548cead05c4bbac63d Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Mon, 16 Sep 2019 12:43:16 +1000 Subject: [PATCH 08/11] Use iterators in `error_at` and `process_cycle`. This makes the code a little faster, presumably because bounds checks aren't needed on `nodes` accesses. It requires making `scratch` a `RefCell`, which is not unreasonable. --- .../obligation_forest/mod.rs | 34 ++++++++++--------- 1 file changed, 18 insertions(+), 16 deletions(-) diff --git a/src/librustc_data_structures/obligation_forest/mod.rs b/src/librustc_data_structures/obligation_forest/mod.rs index 9dbae95dc8f60..0fa1f707d7be3 100644 --- a/src/librustc_data_structures/obligation_forest/mod.rs +++ b/src/librustc_data_structures/obligation_forest/mod.rs @@ -76,7 +76,7 @@ use crate::fx::{FxHashMap, FxHashSet}; use crate::indexed_vec::Idx; use crate::newtype_index; -use std::cell::Cell; +use std::cell::{Cell, RefCell}; use std::collections::hash_map::Entry; use std::fmt::Debug; use std::hash; @@ -156,7 +156,9 @@ pub struct ObligationForest { /// comments in `process_obligation` for details. waiting_cache: FxHashMap, - scratch: Option>, + /// A scratch vector reused in various operations, to avoid allocating new + /// vectors. + scratch: RefCell>, obligation_tree_id_generator: ObligationTreeIdGenerator, @@ -265,7 +267,7 @@ impl ObligationForest { nodes: vec![], done_cache: Default::default(), waiting_cache: Default::default(), - scratch: Some(vec![]), + scratch: RefCell::new(vec![]), obligation_tree_id_generator: (0..).map(ObligationTreeId), error_cache: Default::default(), } @@ -345,8 +347,8 @@ impl ObligationForest { /// Converts all remaining obligations to the given error. pub fn to_errors(&mut self, error: E) -> Vec> { let mut errors = vec![]; - for i in 0..self.nodes.len() { - if let NodeState::Pending = self.nodes[i].state.get() { + for (i, node) in self.nodes.iter().enumerate() { + if let NodeState::Pending = node.state.get() { let backtrace = self.error_at(i); errors.push(Error { error: error.clone(), @@ -469,20 +471,20 @@ impl ObligationForest { /// report all cycles between them. This should be called /// after `mark_as_waiting` marks all nodes with pending /// subobligations as NodeState::Waiting. - fn process_cycles

(&mut self, processor: &mut P) + fn process_cycles

(&self, processor: &mut P) where P: ObligationProcessor { - let mut stack = self.scratch.take().unwrap(); + let mut stack = self.scratch.replace(vec![]); debug_assert!(stack.is_empty()); debug!("process_cycles()"); - for i in 0..self.nodes.len() { + for (i, 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. - match self.nodes[i].state.get() { + match node.state.get() { NodeState::Waiting | NodeState::Pending | NodeState::Done | NodeState::Error => {}, _ => self.find_cycles_from_node(&mut stack, processor, i), } @@ -491,7 +493,7 @@ impl ObligationForest { debug!("process_cycles: complete"); debug_assert!(stack.is_empty()); - self.scratch = Some(stack); + self.scratch.replace(stack); } fn find_cycles_from_node

(&self, stack: &mut Vec, processor: &mut P, i: usize) @@ -525,8 +527,8 @@ impl ObligationForest { /// Returns a vector of obligations for `p` and all of its /// ancestors, putting them into the error state in the process. - fn error_at(&mut self, mut i: usize) -> Vec { - let mut error_stack = self.scratch.take().unwrap(); + fn error_at(&self, mut i: usize) -> Vec { + let mut error_stack = self.scratch.replace(vec![]); let mut trace = vec![]; loop { @@ -554,7 +556,7 @@ impl ObligationForest { ); } - self.scratch = Some(error_stack); + self.scratch.replace(error_stack); trace } @@ -608,7 +610,7 @@ impl ObligationForest { #[inline(never)] fn compress(&mut self, do_completed: DoCompleted) -> Option> { let nodes_len = self.nodes.len(); - let mut node_rewrites: Vec<_> = self.scratch.take().unwrap(); + let mut node_rewrites: Vec<_> = self.scratch.replace(vec![]); node_rewrites.extend(0..nodes_len); let mut dead_nodes = 0; @@ -658,7 +660,7 @@ impl ObligationForest { // No compression needed. if dead_nodes == 0 { node_rewrites.truncate(0); - self.scratch = Some(node_rewrites); + self.scratch.replace(node_rewrites); return if do_completed == DoCompleted::Yes { Some(vec![]) } else { None }; } @@ -682,7 +684,7 @@ impl ObligationForest { self.apply_rewrites(&node_rewrites); node_rewrites.truncate(0); - self.scratch = Some(node_rewrites); + self.scratch.replace(node_rewrites); successful } From f22bb2e72267890782897c208bbc9114d023dfc7 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Mon, 16 Sep 2019 12:43:48 +1000 Subject: [PATCH 09/11] Use `retain` for `waiting_cache` in `apply_rewrites()`. It's more concise, more idiomatic, and measurably faster. --- src/librustc_data_structures/obligation_forest/mod.rs | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/librustc_data_structures/obligation_forest/mod.rs b/src/librustc_data_structures/obligation_forest/mod.rs index 0fa1f707d7be3..5b95bb136e962 100644 --- a/src/librustc_data_structures/obligation_forest/mod.rs +++ b/src/librustc_data_structures/obligation_forest/mod.rs @@ -717,17 +717,15 @@ impl ObligationForest { // This updating of `self.waiting_cache` is necessary because the // removal of nodes within `compress` can fail. See above. - let mut kill_list = vec![]; - for (predicate, index) in &mut self.waiting_cache { + self.waiting_cache.retain(|_predicate, index| { let new_i = node_rewrites[index.index()]; if new_i >= nodes_len { - kill_list.push(predicate.clone()); + false } else { *index = NodeIndex::new(new_i); + true } - } - - for predicate in kill_list { self.waiting_cache.remove(&predicate); } + }); } } From 201afa6455969c1c38e8b46d30cb9be9392207bc Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Mon, 16 Sep 2019 12:47:04 +1000 Subject: [PATCH 10/11] Minor comment tweaks. --- .../obligation_forest/mod.rs | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/src/librustc_data_structures/obligation_forest/mod.rs b/src/librustc_data_structures/obligation_forest/mod.rs index 5b95bb136e962..cb28a7285a75c 100644 --- a/src/librustc_data_structures/obligation_forest/mod.rs +++ b/src/librustc_data_structures/obligation_forest/mod.rs @@ -285,7 +285,7 @@ impl ObligationForest { let _ = self.register_obligation_at(obligation, None); } - // returns Err(()) if we already know this obligation failed. + // Returns Err(()) if we already know this obligation failed. fn register_obligation_at(&mut self, obligation: O, parent: Option) -> Result<(), ()> { @@ -425,7 +425,7 @@ impl ObligationForest { Some(NodeIndex::new(i)) ); if let Err(()) = st { - // error already reported - propagate it + // Error already reported - propagate it // to our node. self.error_at(i); } @@ -454,8 +454,6 @@ impl ObligationForest { self.mark_as_waiting(); self.process_cycles(processor); - - // Now we have to compress the result let completed = self.compress(do_completed); debug!("process_obligations: complete"); @@ -516,11 +514,11 @@ impl ObligationForest { node.state.set(NodeState::Done); }, NodeState::Waiting | NodeState::Pending => { - // this node is still reachable from some pending node. We + // This node is still reachable from some pending node. We // will get to it when they are all processed. } NodeState::Done | NodeState::Error => { - // already processed that node + // Already processed that node. } }; } @@ -664,8 +662,7 @@ impl ObligationForest { return if do_completed == DoCompleted::Yes { Some(vec![]) } else { None }; } - // Pop off all the nodes we killed and extract the success - // stories. + // Pop off all the nodes we killed and extract the success stories. let successful = if do_completed == DoCompleted::Yes { Some((0..dead_nodes) .map(|_| self.nodes.pop().unwrap()) @@ -696,7 +693,6 @@ impl ObligationForest { if let Some(index) = node.parent { let new_i = node_rewrites[index.index()]; if new_i >= nodes_len { - // parent dead due to error node.parent = None; } else { node.parent = Some(NodeIndex::new(new_i)); @@ -745,7 +741,7 @@ impl Node { } } -// I need a Clone closure +// I need a Clone closure. #[derive(Clone)] struct GetObligation<'a, O>(&'a [Node]); From 4ecd94e1215882efde5f003c0042bd72a5f8acb2 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Mon, 16 Sep 2019 12:47:46 +1000 Subject: [PATCH 11/11] Move `impl Node` just after `struct Node`. --- .../obligation_forest/mod.rs | 32 +++++++++---------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/src/librustc_data_structures/obligation_forest/mod.rs b/src/librustc_data_structures/obligation_forest/mod.rs index cb28a7285a75c..189506bf8ab76 100644 --- a/src/librustc_data_structures/obligation_forest/mod.rs +++ b/src/librustc_data_structures/obligation_forest/mod.rs @@ -196,6 +196,22 @@ struct Node { obligation_tree_id: ObligationTreeId, } +impl Node { + fn new( + parent: Option, + obligation: O, + obligation_tree_id: ObligationTreeId + ) -> Node { + Node { + obligation, + state: Cell::new(NodeState::Pending), + parent, + dependents: vec![], + obligation_tree_id, + } + } +} + /// The state of one node in some tree within the forest. This /// represents the current state of processing for the obligation (of /// type `O`) associated with this node. @@ -725,22 +741,6 @@ impl ObligationForest { } } -impl Node { - fn new( - parent: Option, - obligation: O, - obligation_tree_id: ObligationTreeId - ) -> Node { - Node { - obligation, - state: Cell::new(NodeState::Pending), - parent, - dependents: vec![], - obligation_tree_id, - } - } -} - // I need a Clone closure. #[derive(Clone)] struct GetObligation<'a, O>(&'a [Node]);