Artifact conflict check: remove unnecessary check Don't check for artifact conflicts if it's a null build and the only change since the last build is that some SkyKeys were deleted. Deleting ConfiguredTargets can only ever resolve and not create conflicts: - if the last build had a conflict, then we'd re-check anyway, because that is the same case as when building (x y) is in conflict but then null-building just (x) is not, and we re-check solely on the basis of the last build having had conflicts. - if the last build had no conflict, then deleting a CT won't change that. This CL also adds a comment that explains why we don't check in this case. This is a follow-up to commit 044264ee7290292195d0980657469ac69ba280e0. RELNOTES: none PiperOrigin-RevId: 291111167
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java index fd173c2..00b68b6 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java
@@ -122,7 +122,6 @@ // We keep the set of invalidated configuration target keys so that we can know if something // has been invalidated after graph pruning has been executed. private Set<SkyKey> dirtiedConfiguredTargetKeys = Sets.newConcurrentHashSet(); - private volatile boolean anyConfiguredTargetDeleted = false; private final ConfiguredRuleClassProvider ruleClassProvider; @@ -526,14 +525,8 @@ return true; } - if (anyConfiguredTargetDeleted) { - // No target was (re)computed, but some were deleted, which may resolve a conflict. - // TODO(laszlocsomor): get to understand this condition and give an example for it. - return true; - } - if (!dirtiedConfiguredTargetKeys.isEmpty()) { - // No target was (re)computed and none was deleted, but at least one was dirtied. + // No target was (re)computed but at least one was dirtied. // Example: (//:x //foo:y) are built, and in conflict (//:x creates foo/C and //foo:y // creates C). Then y is removed from foo/BUILD and only //:x is built, so //foo:y is // dirtied but not recomputed, and no other nodes are recomputed (and none are deleted). @@ -546,9 +539,9 @@ // Example sequence: // 1. Build (x y z), and there is a conflict. We store (x y z) as the largest checked key // set, and record the fact that there were bad actions. - // 2. Null-build (x z), so we don't evaluate or dirty or delete anything, but because we know - // there was some conflict last time but don't know exactly which targets conflicted, it - // could have been (x z), so we now check again. + // 2. Null-build (x z), so we don't evaluate or dirty anything, but because we know there was + // some conflict last time but don't know exactly which targets conflicted, it could have + // been (x z), so we now check again. return true; } @@ -556,26 +549,39 @@ // Example sequence: // 1. Build (x y z), and there is a conflict. We store (x y z) as the largest checked key // set, and record the fact that there were bad actions. - // 2. Null-build (x z), so we don't evaluate or dirty or delete anything, but because we know - // there was some conflict last time but don't know exactly which targets conflicted, it - // could have been (x z), so we now check again, and store (x z) as the largest checked - // key set. - // 3. Null-build (y z), so again we don't evaluate or dirty or delete anything, and the - // previous build had no conflicts, so no other condition is true. But because (y z) is no - // subset of (x z) and we only keep the most recent largest checked key set, we don't know - // if (y z) are conflict free, so we check. + // 2. Null-build (x z), so we don't evaluate or dirty anything, but because we know there was + // some conflict last time but don't know exactly which targets conflicted, it could have + // been (x z), so we now check again, and store (x z) as the largest checked key set. + // 3. Null-build (y z), so again we don't evaluate or dirty anything, and the previous build + // had no conflicts, so no other condition is true. But because (y z) is not a subset of + // (x z) and we only keep the most recent largest checked key set, we don't know if (y z) + // are conflict free, so we check. return true; } // We believe the conditions above are correct in the sense that we always check for conflicts // when we have to. But they are incomplete, so we sometimes check for conflicts even if we // wouldn't have to. For example: - // - if no target was evaluated (nor dirtied, nor deleted), and build sequence is (x y) - // [no conflict], (z), where z is in the transitive closure of (x y), then we shouldn't check. - // - if no target was evaluated (nor dirtied, nor deleted), and build sequence is (x y) - // [no conflict], (z), (x), then the last build shouldn't conflict-check because (x y) was - // checked earlier. But it does, since after the second build we store (z) as the largest - // checked set of which (x) is no subset. + // - if no target was evaluated nor dirtied and build sequence is (x y) [no conflict], (z), + // where z is in the transitive closure of (x y), then we shouldn't check. + // - if no target was evaluated nor dirtied and build sequence is (x y) [no conflict], (w), (x), + // then the last build shouldn't conflict-check because (x y) was checked earlier. But it + // does, because after the second build we store (w) as the largest checked set, and (x) is + // not a subset of that. + + // Case when we DON'T need to re-check: + // - a configured target is deleted. Deletion can only resolve conflicts, not introduce any, and + // if the previuos build had a conflict then skyframeActionExecutor.badActions() would not be + // empty, and if the previous build had no conflict then deleting a CT won't change that. + // Example that triggers this scenario: + // 1. genrule(name='x', srcs=['A'], ...) + // genrule(name='y', outs=['A'], ...) + // 2. Build (x y) + // 3. Rename 'x' to 'y', and 'y' to 'z' + // 4. Build (y z) + // 5. Null-build (y z) again + // We only delete the old 'x' value in (5), and we don't evaluate nor dirty anything, nor was + // (4) bad. So there's no reason to re-check just because we deleted something. return false; } @@ -912,7 +918,6 @@ /** Clear the invalidated configured targets detected during loading and analysis phases. */ public void clearInvalidatedConfiguredTargets() { dirtiedConfiguredTargetKeys = Sets.newConcurrentHashSet(); - anyConfiguredTargetDeleted = false; } /** @@ -936,16 +941,13 @@ @Override public void invalidated(SkyKey skyKey, InvalidationState state) { - if (skyKey.functionName().equals(SkyFunctions.CONFIGURED_TARGET)) { - if (state == InvalidationState.DELETED) { - anyConfiguredTargetDeleted = true; - } else { - // If the value was just dirtied and not deleted, then it may not be truly invalid, since - // it may later get re-validated. Therefore adding the key to dirtiedConfiguredTargetKeys - // is provisional--if the key is later evaluated and the value found to be clean, then we - // remove it from the set. - dirtiedConfiguredTargetKeys.add(skyKey); - } + if (skyKey.functionName().equals(SkyFunctions.CONFIGURED_TARGET) + && state != InvalidationState.DELETED) { + // If the value was just dirtied and not deleted, then it may not be truly invalid, since + // it may later get re-validated. Therefore adding the key to dirtiedConfiguredTargetKeys + // is provisional--if the key is later evaluated and the value found to be clean, then we + // remove it from the set. + dirtiedConfiguredTargetKeys.add(skyKey); } }