Skip to content

Commit

Permalink
Prevent unnecessary duplicated calls to graph.getBatch() with the sam…
Browse files Browse the repository at this point in the history
…e set

of keys. That's just a waste.

RELNOTES: None.
PiperOrigin-RevId: 245546337
  • Loading branch information
djasper authored and Copybara-Service committed Apr 27, 2019
1 parent 95f01a0 commit 9a32b86
Showing 1 changed file with 15 additions and 8 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@
* translating a set of requested top-level nodes into actions, or constructing an evaluation
* result. Derived classes should do this.
*/
public abstract class AbstractParallelEvaluator {
abstract class AbstractParallelEvaluator {
private static final Logger logger = Logger.getLogger(AbstractParallelEvaluator.class.getName());

final ProcessableGraph graph;
Expand Down Expand Up @@ -232,6 +232,7 @@ private DirtyOutcome maybeHandleDirtyNode(NodeEntry state) throws InterruptedExc
graph.get(skyKey, Reason.RDEP_REMOVAL, ErrorTransienceValue.KEY).removeReverseDep(skyKey);
return DirtyOutcome.NEEDS_EVALUATION;
}
Map<SkyKey, ? extends NodeEntry> entriesToCheck = null;
if (!evaluatorContext.keepGoing()) {
// This check ensures that we maintain the invariant that if a node with an error is
// reached during a no-keep-going build, none of its currently building parents
Expand All @@ -240,8 +241,7 @@ private DirtyOutcome maybeHandleDirtyNode(NodeEntry state) throws InterruptedExc
// is done, then it is the parent's responsibility to notice that, which we do here.
// We check the deps for errors so that we don't continue building this node if it has
// a child error.
Map<SkyKey, ? extends NodeEntry> entriesToCheck =
graph.getBatch(skyKey, Reason.OTHER, directDepsToCheck);
entriesToCheck = graph.getBatch(skyKey, Reason.OTHER, directDepsToCheck);
for (Map.Entry<SkyKey, ? extends NodeEntry> entry : entriesToCheck.entrySet()) {
NodeEntry nodeEntryToCheck = entry.getValue();
SkyValue valueMaybeWithMetadata = nodeEntryToCheck.getValueMaybeWithMetadata();
Expand Down Expand Up @@ -296,8 +296,11 @@ private DirtyOutcome maybeHandleDirtyNode(NodeEntry state) throws InterruptedExc
unknownStatusDeps);
continue;
}
if (entriesToCheck == null || depsReport.hasInformation()) {
entriesToCheck = graph.getBatch(skyKey, Reason.ENQUEUING_CHILD, unknownStatusDeps);
}
handleKnownChildrenForDirtyNode(
unknownStatusDeps, state, globalEnqueuedIndex.incrementAndGet());
unknownStatusDeps, entriesToCheck, state, globalEnqueuedIndex.incrementAndGet());
return DirtyOutcome.ALREADY_PROCESSED;
}
switch (state.getDirtyState()) {
Expand Down Expand Up @@ -348,10 +351,11 @@ private DirtyOutcome maybeHandleDirtyNode(NodeEntry state) throws InterruptedExc
}

private void handleKnownChildrenForDirtyNode(
Collection<SkyKey> knownChildren, NodeEntry state, int childEvaluationPriority)
Collection<SkyKey> knownChildren,
Map<SkyKey, ? extends NodeEntry> oldChildren,
NodeEntry state,
int childEvaluationPriority)
throws InterruptedException {
Map<SkyKey, ? extends NodeEntry> oldChildren =
graph.getBatch(skyKey, Reason.ENQUEUING_CHILD, knownChildren);
if (oldChildren.size() != knownChildren.size()) {
GraphInconsistencyReceiver inconsistencyReceiver =
evaluatorContext.getGraphInconsistencyReceiver();
Expand Down Expand Up @@ -670,7 +674,10 @@ public void run() {
graph.createIfAbsentBatchAsync(
skyKey, Reason.RDEP_ADDITION, newDepsThatWerentInTheLastEvaluation);
handleKnownChildrenForDirtyNode(
newDepsThatWereInTheLastEvaluation, state, childEvaluationPriority);
newDepsThatWereInTheLastEvaluation,
graph.getBatch(skyKey, Reason.ENQUEUING_CHILD, newDepsThatWereInTheLastEvaluation),
state,
childEvaluationPriority);

// Due to multi-threading, this can potentially cause the current node to be re-enqueued if
// all 'new' children of this node are already done. Therefore, there should not be any
Expand Down

0 comments on commit 9a32b86

Please sign in to comment.