From 32f63a92b1151551d62aab135c57f56ab25624af Mon Sep 17 00:00:00 2001 From: Googler Date: Thu, 22 Jun 2023 08:31:53 -0700 Subject: [PATCH] [Skymeld] Restructure the initialization and clearing of the various conflict checking states. Previously, these get unconditionally created (skymeld/noskymeld) but only used and reset in Skymeld mode. This CL would make it so that these states are: - Only created in Skymeld mode, once per invocation - Cleared at the end of analysis (to save memory) and unconditionally at the end of the build (to cover the case there analysis never finished) PiperOrigin-RevId: 542568005 Change-Id: Ia75a3a3cf25592c02a5f9f1107c98cb8bb73e14c --- .../lib/skyframe/BuildDriverFunction.java | 13 +++++- .../build/lib/skyframe/SkyframeBuildView.java | 18 +++++--- .../build/lib/skyframe/SkyframeExecutor.java | 46 +++++++++++-------- 3 files changed, 50 insertions(+), 27 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BuildDriverFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/BuildDriverFunction.java index 05d2229bff47c8..ad68011d2331e4 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BuildDriverFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BuildDriverFunction.java @@ -611,12 +611,16 @@ private static void declareDependenciesAndCheckValues( ImmutableMap checkActionConflicts( ActionLookupKeyOrProxy actionLookupKey, boolean strictConflictCheck) throws InterruptedException { + IncrementalArtifactConflictFinder localRef = incrementalArtifactConflictFinder.get(); + // a null value means that the conflict checker is shut down. + if (localRef == null) { + return ImmutableMap.of(); + } ActionLookupValuesCollectionResult transitiveValueCollectionResult = transitiveActionLookupValuesHelper.collect(actionLookupKey); ImmutableMap conflicts = - incrementalArtifactConflictFinder - .get() + localRef .findArtifactConflicts( transitiveValueCollectionResult.collectedValues(), strictConflictCheck) .getConflicts(); @@ -695,5 +699,10 @@ static ActionLookupValuesCollectionResult create( return new AutoValue_BuildDriverFunction_ActionLookupValuesCollectionResult( collectedValues, visitedKeys); } + + static ActionLookupValuesCollectionResult empty() { + return new AutoValue_BuildDriverFunction_ActionLookupValuesCollectionResult( + ImmutableSet.of(), ImmutableSet.of()); + } } } 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 21a525f7acc72c..050cec9f14e21b 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 @@ -660,7 +660,7 @@ public SkyframeAnalysisResult analyzeAndExecuteTargets( // Required for incremental correctness. // We unconditionally reset the states here instead of in #analysisFinishedCallback since // in case of --nokeep_going & analysis error, the analysis phase is never finished. - skyframeExecutor.resetIncrementalArtifactConflictFindingStates(); + skyframeExecutor.clearIncrementalArtifactConflictFindingStates(); skyframeExecutor.resetBuildDriverFunction(); skyframeExecutor.setTestTypeResolver(null); @@ -825,14 +825,18 @@ private void analysisFinishedCallback( throws InterruptedException { if (shouldPublishBuildGraphMetrics.get()) { // Now that we have the full picture, it's time to collect the metrics of the whole graph. - BuildGraphMetrics buildGraphMetrics = + BuildGraphMetrics.Builder buildGraphMetricsBuilder = skyframeExecutor .collectActionLookupValuesInBuild( configuredTargetKeys, buildResultListener.getAnalyzedAspects().keySet()) - .getMetrics() - .setOutputArtifactCount(skyframeExecutor.getOutputArtifactCount()) - .build(); - eventBus.post(new AnalysisGraphStatsEvent(buildGraphMetrics)); + .getMetrics(); + IncrementalArtifactConflictFinder incrementalArtifactConflictFinder = + skyframeExecutor.getIncrementalArtifactConflictFinder(); + if (incrementalArtifactConflictFinder != null) { + buildGraphMetricsBuilder.setOutputArtifactCount( + incrementalArtifactConflictFinder.getOutputArtifactCount()); + } + eventBus.post(new AnalysisGraphStatsEvent(buildGraphMetricsBuilder.build())); } if (shouldDiscardAnalysisCache) { @@ -844,7 +848,7 @@ private void analysisFinishedCallback( // At this point, it's safe to clear objects related to action conflict checking. // Clearing the states here is a performance optimization (reduce peak heap size) and isn't // required for correctness. - skyframeExecutor.resetIncrementalArtifactConflictFindingStates(); + skyframeExecutor.clearIncrementalArtifactConflictFindingStates(); // Clearing the syscall cache here to free up some heap space. // TODO(b/273225564) Would this incur more CPU cost for the execution phase cache misses? diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java index 47c0c88236b420..cd86e063b71275 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java @@ -418,17 +418,21 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory, Configur private RemoteOutputsMode lastRemoteOutputsMode; private Boolean lastRemoteCacheEnabled; + // start: Skymeld-only // This is set once every build and set to null at the end of each. @Nullable private Supplier mergedSkyframeAnalysisExecutionSupplier; // Reset after each build. - private IncrementalArtifactConflictFinder incrementalArtifactConflictFinder; + @Nullable private IncrementalArtifactConflictFinder incrementalArtifactConflictFinder; // Reset after each build. + @Nullable private TransitiveActionLookupKeysCollector incrementalTransitiveActionLookupKeysCollector; // A set of ActionLookupKeys which have been confirmed to be conflict-free. This is used for // pruning while going through the ActionLookupKeys in the build for conflict checking. // Reset after each build. private Set conflictFreeActionLookupKeysGlobalSet; + // end: Skymeld-only + private RuleContextConstraintSemantics ruleContextConstraintSemantics; private RegexFilter extraActionFilter; @Nullable private ActionExecutionInactivityWatchdog watchdog; @@ -881,15 +885,6 @@ protected final void init() { memoizingEvaluator = createEvaluator(skyFunctions(), progressReceiver, emittedEventState); skyframeExecutorConsumerOnInit.accept(this); - // Initialize the various conflict-finding states. This is unconditionally created but only used - // with Skymeld i.e. --experimental_merged_skyframe_analysis_execution. - incrementalArtifactConflictFinder = - IncrementalArtifactConflictFinder.createWithActionGraph( - new MapBasedActionGraph(actionKeyContext)); - conflictFreeActionLookupKeysGlobalSet = Sets.newConcurrentHashSet(); - incrementalTransitiveActionLookupKeysCollector = - new TransitiveActionLookupKeysCollector( - SkyframeExecutorWrappingWalkableGraph.of(this), conflictFreeActionLookupKeysGlobalSet); } @ForOverride @@ -2153,6 +2148,9 @@ EvaluationResult evaluateBuildDriverKeys( checkActive(); try { buildDriverFunction.setShouldCheckForConflict(shouldCheckForConflicts); + if (shouldCheckForConflicts.get()) { + initializeSkymeldConflictFindingStates(); + } eventHandler.post(new ConfigurationPhaseStartedEvent(configuredTargetProgress)); // For the workspace status actions. eventHandler.post(SomeExecutionStartedEvent.create()); @@ -2278,9 +2276,8 @@ public void resetBuildDriverFunction() { buildDriverFunction.resetStates(); } - /** Resets the incremental artifact conflict finder to ensure incremental correctness. */ - public void resetIncrementalArtifactConflictFindingStates() { - incrementalArtifactConflictFinder.shutdown(); + // Initialize the various conflict-finding states. These are good for 1 invocation. + private void initializeSkymeldConflictFindingStates() { incrementalArtifactConflictFinder = IncrementalArtifactConflictFinder.createWithActionGraph( new MapBasedActionGraph(actionKeyContext)); @@ -2290,12 +2287,20 @@ public void resetIncrementalArtifactConflictFindingStates() { SkyframeExecutorWrappingWalkableGraph.of(this), conflictFreeActionLookupKeysGlobalSet); } - private IncrementalArtifactConflictFinder getIncrementalArtifactConflictFinder() { - return incrementalArtifactConflictFinder; + /** Clear the incremental conflict finding states to save memory. */ + public void clearIncrementalArtifactConflictFindingStates() { + // Create a local ref for shutting down, in case there's a race. + IncrementalArtifactConflictFinder localRef = incrementalArtifactConflictFinder; + if (localRef != null) { + localRef.shutdown(); + } + incrementalArtifactConflictFinder = null; + conflictFreeActionLookupKeysGlobalSet = Sets.newConcurrentHashSet(); + incrementalTransitiveActionLookupKeysCollector = null; } - public int getOutputArtifactCount() { - return incrementalArtifactConflictFinder.getOutputArtifactCount(); + public IncrementalArtifactConflictFinder getIncrementalArtifactConflictFinder() { + return incrementalArtifactConflictFinder; } private Set getConflictFreeActionLookupKeysGlobalSet() { @@ -3183,8 +3188,13 @@ private ActionLookupValuesCollectionResult collectTransitiveActionLookupValuesOf try (SilentCloseable c = Profiler.instance().profile("SkyframeExecutor.collectTransitiveActionLookupValuesOfKey")) { if (tracksStateForIncrementality()) { + var localRef = incrementalTransitiveActionLookupKeysCollector; + // a null value means that the conflict checker is shut down. + if (localRef == null) { + return ActionLookupValuesCollectionResult.empty(); + } ImmutableMap foundTransitiveActionLookupEntities = - incrementalTransitiveActionLookupKeysCollector.collect(key); + localRef.collect(key); return ActionLookupValuesCollectionResult.create( foundTransitiveActionLookupEntities.values(), ImmutableSet.copyOf(foundTransitiveActionLookupEntities.keySet()));