From b44fc9741e0198202ced47dfa3f51439c2295262 Mon Sep 17 00:00:00 2001 From: Stu Hood Date: Sat, 27 Jun 2020 14:06:54 -0700 Subject: [PATCH] Remove stale docstrings and lock acquisitions re: dirty running nodes post #10139. --- src/rust/engine/graph/src/entry.rs | 7 ------- src/rust/engine/graph/src/lib.rs | 25 ++++--------------------- 2 files changed, 4 insertions(+), 28 deletions(-) diff --git a/src/rust/engine/graph/src/entry.rs b/src/rust/engine/graph/src/entry.rs index e827b26948d..c9c89842de8 100644 --- a/src/rust/engine/graph/src/entry.rs +++ b/src/rust/engine/graph/src/entry.rs @@ -453,12 +453,6 @@ impl Entry { /// result should be used. This special case exists to avoid 1) cloning the result to call this /// method, and 2) comparing the current/previous results unnecessarily. /// - /// Takes a &mut InnerGraph to ensure that completing nodes doesn't race with dirtying them. - /// The important relationship being guaranteed here is that if the Graph is calling - /// invalidate_from_roots, it may mark us, or our dependencies, as dirty. We don't want to - /// complete _while_ a batch of nodes are being marked as dirty, and this exclusive access ensures - /// that can't happen. - /// pub(crate) fn complete( &mut self, context: &N::Context, @@ -466,7 +460,6 @@ impl Entry { dep_generations: Vec, result: Option>, has_uncacheable_deps: bool, - _graph: &mut super::InnerGraph, ) { let mut state = self.state.lock(); diff --git a/src/rust/engine/graph/src/lib.rs b/src/rust/engine/graph/src/lib.rs index c4d0e7b6087..2d8c8aa6126 100644 --- a/src/rust/engine/graph/src/lib.rs +++ b/src/rust/engine/graph/src/lib.rs @@ -766,17 +766,8 @@ impl Graph { /// /// When the Executor finishes executing a Node it calls back to store the result value. We use - /// the run_token and dirty bits to determine whether the Node changed while we were busy - /// executing it, so that we can discard the work. - /// - /// We use the dirty bit in addition to the RunToken in order to avoid cases where dependencies - /// change while we're running. In order for a dependency to "change" it must have been cleared - /// or been marked dirty. But if our dependencies have been cleared or marked dirty, then we will - /// have been as well. We can thus use the dirty bit as a signal that the generation values of - /// our dependencies are still accurate. The dirty bit is safe to rely on as it is only ever - /// mutated, and dependencies' dirty bits are only read, under the InnerGraph lock - this is only - /// reliably the case because Entry happens to require a &mut InnerGraph reference; it would be - /// great not to violate that in the future. + /// the run_token to determine whether the Node changed while we were busy executing it, so that + /// we can discard the work. /// /// TODO: We don't track which generation actually added which edges, so over time nodes will end /// up with spurious dependencies. This is mostly sound, but may lead to over-invalidation and @@ -789,13 +780,6 @@ impl Graph { /// consideration of locking (probably it would require merging the EntryState locks and Graph /// locks, or working out something clever). /// - /// It would also require careful consideration of nodes in the Running EntryState - these may - /// have previous RunToken edges and next RunToken edges which collapse into the same Generation - /// edges; when working out whether a dirty node is really clean, care must be taken to avoid - /// spurious cycles. Currently we handle this as a special case by, if we detect a cycle that - /// contains dirty nodes, clearing those nodes (removing any edges from them). This is a little - /// hacky, but will tide us over until we fully solve this problem. - /// fn complete( &self, context: &N::Context, @@ -807,7 +791,8 @@ impl Graph { let inner = self.inner.lock(); let mut has_uncacheable_deps = false; // Get the Generations of all dependencies of the Node. We can trust that these have not changed - // since we began executing, as long as we are not currently marked dirty (see the method doc). + // since we began executing, as long as the entry's RunToken is still valid (confirmed in + // Entry::complete). let dep_generations = inner .pg .neighbors_directed(entry_id, Direction::Outgoing) @@ -830,14 +815,12 @@ impl Graph { ) }; if let Some(mut entry) = entry { - let mut inner = self.inner.lock(); entry.complete( context, run_token, dep_generations, result, has_uncacheable_deps, - &mut inner, ); } }