Skip to content

Commit

Permalink
Revert parts of #66405.
Browse files Browse the repository at this point in the history 
Because it caused major performance regressions in some cases.

That PR had five commits, two of which affected performance, and three
of which were refactorings. This change undoes the performance-affecting
changes, while keeping the refactorings in place.

Fixes #67454.
  • Loading branch information
@nnethercote @Mark-Simulacrum
nnethercote authored and Mark-Simulacrum committed Dec 31, 2019
1 parent 71bb0ff commit 18a3669
Showing 1 changed file with 80 additions and 111 deletions.
191 changes: 80 additions & 111 deletions src/librustc_data_structures/obligation_forest/mod.rs
View file
Expand Up @@ -129,18 +129,14 @@ type ObligationTreeIdGenerator =

pub struct ObligationForest<O: ForestObligation> {
/// The list of obligations. In between calls to `process_obligations`,
/// this list only contains nodes in the `Pending` or `Success` state.
/// this list only contains nodes in the `Pending` or `Waiting` state.
///
/// `usize` indices are used here and throughout this module, rather than
/// `rustc_index::newtype_index!` indices, because this code is hot enough
/// that the `u32`-to-`usize` conversions that would be required are
/// significant, and space considerations are not important.
nodes: Vec<Node<O>>,

/// The process generation is 1 on the first call to `process_obligations`,
/// 2 on the second call, etc.
gen: u32,

/// A cache of predicates that have been successfully completed.
done_cache: FxHashSet<O::Predicate>,

Expand Down Expand Up @@ -196,9 +192,9 @@ impl<O> Node<O> {
}
}

/// 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.
/// 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.
///
/// The non-`Error` state transitions are as follows.
/// ```
Expand All @@ -210,51 +206,47 @@ impl<O> Node<O> {
/// |
/// | process_obligations()
/// v
/// Success(not_waiting())
/// | |
/// | | mark_still_waiting_nodes()
/// Success
/// | ^
/// | | mark_successes()
/// | v
/// | Success(still_waiting())
/// | |
/// | | compress()
/// v v
/// | Waiting
/// |
/// | process_cycles()
/// v
/// Done
/// |
/// | compress()
/// v
/// (Removed)
/// ```
/// The `Error` state can be introduced in several places, via `error_at()`.
///
/// Outside of `ObligationForest` methods, nodes should be either `Pending` or
/// `Success`.
/// `Waiting`.
#[derive(Debug, Copy, Clone, PartialEq, Eq)]
enum NodeState {
/// This obligation has not yet been selected successfully. Cannot have
/// subobligations.
Pending,

/// This obligation was selected successfully, but it may be waiting on one
/// or more pending subobligations, as indicated by the `WaitingState`.
Success(WaitingState),
/// This obligation was selected successfully, but may or may not have
/// subobligations.
Success,

/// This obligation was selected successfully, but it has a pending
/// subobligation.
Waiting,

/// This obligation, along with its subobligations, are complete, and will
/// be removed in the next collection.
Done,

/// This obligation was resolved to an error. It will be removed by the
/// next compression step.
Error,
}

/// Indicates when a `Success` node was last (if ever) waiting on one or more
/// `Pending` nodes. The notion of "when" comes from `ObligationForest::gen`.
/// - 0: "Not waiting". This is a special value, set by `process_obligation`,
/// and usable because generation counting starts at 1.
/// - 1..ObligationForest::gen: "Was waiting" in a previous generation, but
/// waiting no longer. In other words, finished.
/// - ObligationForest::gen: "Still waiting" in this generation.
///
/// Things to note about this encoding:
/// - Every time `ObligationForest::gen` is incremented, all the "still
/// waiting" nodes automatically become "was waiting".
/// - `ObligationForest::is_still_waiting` is very cheap.
///
#[derive(Clone, Copy, Debug, PartialEq, Eq, PartialOrd)]
struct WaitingState(u32);

#[derive(Debug)]
pub struct Outcome<O, E> {
/// Obligations that were completely evaluated, including all
Expand Down Expand Up @@ -291,7 +283,6 @@ impl<O: ForestObligation> ObligationForest<O> {
pub fn new() -> ObligationForest<O> {
ObligationForest {
nodes: vec![],
gen: 0,
done_cache: Default::default(),
active_cache: Default::default(),
node_rewrites: RefCell::new(vec![]),
Expand Down Expand Up @@ -392,18 +383,6 @@ impl<O: ForestObligation> ObligationForest<O> {
.insert(node.obligation.as_predicate().clone());
}

fn not_waiting() -> WaitingState {
WaitingState(0)
}

fn still_waiting(&self) -> WaitingState {
WaitingState(self.gen)
}

fn is_still_waiting(&self, waiting: WaitingState) -> bool {
waiting.0 == self.gen
}

/// Performs a pass through the obligation list. This must
/// be called in a loop until `outcome.stalled` is false.
///
Expand All @@ -416,8 +395,6 @@ impl<O: ForestObligation> ObligationForest<O> {
where
P: ObligationProcessor<Obligation = O>,
{
self.gen += 1;

let mut errors = vec![];
let mut stalled = true;

Expand Down Expand Up @@ -450,7 +427,7 @@ impl<O: ForestObligation> ObligationForest<O> {
ProcessResult::Changed(children) => {
// We are not (yet) stalled.
stalled = false;
node.state.set(NodeState::Success(Self::not_waiting()));
node.state.set(NodeState::Success);

for child in children {
let st = self.register_obligation_at(child, Some(index));
Expand Down Expand Up @@ -479,7 +456,7 @@ impl<O: ForestObligation> ObligationForest<O> {
};
}

self.mark_still_waiting_nodes();
self.mark_successes();
self.process_cycles(processor);
let completed = self.compress(do_completed);

Expand Down Expand Up @@ -519,41 +496,50 @@ impl<O: ForestObligation> ObligationForest<O> {
trace
}

/// Mark all `Success` nodes that depend on a pending node as still
/// waiting. Upon completion, any `Success` nodes that aren't still waiting
/// can be removed by `compress`.
fn mark_still_waiting_nodes(&self) {
/// Mark all `Waiting` nodes as `Success`, except those that depend on a
/// pending node.
fn mark_successes(&self) {
// Convert all `Waiting` nodes to `Success`.
for node in &self.nodes {
if node.state.get() == NodeState::Waiting {
node.state.set(NodeState::Success);
}
}

// Convert `Success` nodes that depend on a pending node back to
// `Waiting`.
for node in &self.nodes {
if node.state.get() == NodeState::Pending {
// This call site is hot.
self.inlined_mark_dependents_as_still_waiting(node);
self.inlined_mark_dependents_as_waiting(node);
}
}
}

// This always-inlined function is for the hot call site.
#[inline(always)]
fn inlined_mark_dependents_as_still_waiting(&self, node: &Node<O>) {
fn inlined_mark_dependents_as_waiting(&self, node: &Node<O>) {
for &index in node.dependents.iter() {
let node = &self.nodes[index];
if let NodeState::Success(waiting) = node.state.get() {
if !self.is_still_waiting(waiting) {
node.state.set(NodeState::Success(self.still_waiting()));
// This call site is cold.
self.uninlined_mark_dependents_as_still_waiting(node);
}
let state = node.state.get();
if state == NodeState::Success {
node.state.set(NodeState::Waiting);
// This call site is cold.
self.uninlined_mark_dependents_as_waiting(node);
} else {
debug_assert!(state == NodeState::Waiting || state == NodeState::Error)
}
}
}

// This never-inlined function is for the cold call site.
#[inline(never)]
fn uninlined_mark_dependents_as_still_waiting(&self, node: &Node<O>) {
self.inlined_mark_dependents_as_still_waiting(node)
fn uninlined_mark_dependents_as_waiting(&self, node: &Node<O>) {
self.inlined_mark_dependents_as_waiting(node)
}

/// Report cycles between all `Success` nodes that aren't still waiting.
/// This must be called after `mark_still_waiting_nodes`.
/// Report cycles between all `Success` nodes, and convert all `Success`
/// nodes to `Done`. This must be called after `mark_successes`.
fn process_cycles<P>(&self, processor: &mut P)
where
P: ObligationProcessor<Obligation = O>,
Expand All @@ -564,63 +550,51 @@ impl<O: ForestObligation> ObligationForest<O> {
// 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.
if let NodeState::Success(waiting) = node.state.get() {
if !self.is_still_waiting(waiting) {
self.find_cycles_from_node(&mut stack, processor, index, index);
}
if node.state.get() == NodeState::Success {
self.find_cycles_from_node(&mut stack, processor, index);
}
}

debug_assert!(stack.is_empty());
}

fn find_cycles_from_node<P>(
&self,
stack: &mut Vec<usize>,
processor: &mut P,
min_index: usize,
index: usize,
) where
fn find_cycles_from_node<P>(&self, stack: &mut Vec<usize>, processor: &mut P, index: usize)
where
P: ObligationProcessor<Obligation = O>,
{
let node = &self.nodes[index];
if let NodeState::Success(waiting) = node.state.get() {
if !self.is_still_waiting(waiting) {
match stack.iter().rposition(|&n| n == index) {
None => {
stack.push(index);
for &dep_index in node.dependents.iter() {
// The index check avoids re-considering a node.
if dep_index >= min_index {
self.find_cycles_from_node(stack, processor, min_index, dep_index);
}
}
stack.pop();
}
Some(rpos) => {
// Cycle detected.
processor.process_backedge(
stack[rpos..].iter().map(GetObligation(&self.nodes)),
PhantomData,
);
if node.state.get() == NodeState::Success {
match stack.iter().rposition(|&n| n == index) {
None => {
stack.push(index);
for &dep_index in node.dependents.iter() {
self.find_cycles_from_node(stack, processor, dep_index);
}
stack.pop();
node.state.set(NodeState::Done);
}
Some(rpos) => {
// Cycle detected.
processor.process_backedge(
stack[rpos..].iter().map(GetObligation(&self.nodes)),
PhantomData,
);
}
}
}
}

/// Compresses the vector, removing all popped nodes. This adjusts the
/// indices and hence invalidates any outstanding indices. `process_cycles`
/// must be run beforehand to remove any cycles on not-still-waiting
/// `Success` nodes.
/// must be run beforehand to remove any cycles on `Success` nodes.
#[inline(never)]
fn compress(&mut self, do_completed: DoCompleted) -> Option<Vec<O>> {
let orig_nodes_len = self.nodes.len();
let mut node_rewrites: Vec<_> = self.node_rewrites.replace(vec![]);
debug_assert!(node_rewrites.is_empty());
node_rewrites.extend(0..orig_nodes_len);
let mut dead_nodes = 0;
let mut removed_success_obligations: Vec<O> = vec![];
let mut removed_done_obligations: Vec<O> = vec![];

// Move removable nodes to the end, preserving the order of the
// remaining nodes.
Expand All @@ -632,19 +606,13 @@ impl<O: ForestObligation> ObligationForest<O> {
for index in 0..orig_nodes_len {
let node = &self.nodes[index];
match node.state.get() {
NodeState::Pending => {
if dead_nodes > 0 {
self.nodes.swap(index, index - dead_nodes);
node_rewrites[index] -= dead_nodes;
}
}
NodeState::Success(waiting) if self.is_still_waiting(waiting) => {
NodeState::Pending | NodeState::Waiting => {
if dead_nodes > 0 {
self.nodes.swap(index, index - dead_nodes);
node_rewrites[index] -= dead_nodes;
}
}
NodeState::Success(_) => {
NodeState::Done => {
// This lookup can fail because the contents of
// `self.active_cache` are not guaranteed to match those of
// `self.nodes`. See the comment in `process_obligation`
Expand All @@ -658,7 +626,7 @@ impl<O: ForestObligation> ObligationForest<O> {
}
if do_completed == DoCompleted::Yes {
// Extract the success stories.
removed_success_obligations.push(node.obligation.clone());
removed_done_obligations.push(node.obligation.clone());
}
node_rewrites[index] = orig_nodes_len;
dead_nodes += 1;
Expand All @@ -672,6 +640,7 @@ impl<O: ForestObligation> ObligationForest<O> {
node_rewrites[index] = orig_nodes_len;
dead_nodes += 1;
}
NodeState::Success => unreachable!(),
}
}

Expand All @@ -684,7 +653,7 @@ impl<O: ForestObligation> ObligationForest<O> {
node_rewrites.truncate(0);
self.node_rewrites.replace(node_rewrites);

if do_completed == DoCompleted::Yes { Some(removed_success_obligations) } else { None }
if do_completed == DoCompleted::Yes { Some(removed_done_obligations) } else { None }
}

fn apply_rewrites(&mut self, node_rewrites: &[usize]) {
Expand Down

0 comments on commit 18a3669

Please sign in to comment.