From 89eefd710afb7428bff331a9ae4c2e84d2a21624 Mon Sep 17 00:00:00 2001 From: Ulf Adams Date: Wed, 23 Sep 2015 08:00:43 +0000 Subject: Move ownership of SkyframeBuildView to SkyframeExecutor. Also move ownership of ArtifactFactory to SkyframeBuildView; simplify the code. -- MOS_MIGRATED_REVID=103722228 --- .../devtools/build/lib/analysis/BuildView.java | 37 +++------------- .../lib/analysis/CachingAnalysisEnvironment.java | 2 +- .../devtools/build/lib/runtime/BlazeRuntime.java | 2 +- .../lib/skyframe/SequencedSkyframeExecutor.java | 13 ++++-- .../skyframe/SequencedSkyframeExecutorFactory.java | 3 ++ .../build/lib/skyframe/SkyframeBuildView.java | 21 +++++---- .../build/lib/skyframe/SkyframeExecutor.java | 51 ++++++++-------------- .../lib/skyframe/SkyframeExecutorFactory.java | 3 ++ 8 files changed, 50 insertions(+), 82 deletions(-) (limited to 'src/main/java') diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java index 92290ac959..eb8ec0fde4 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java @@ -220,8 +220,6 @@ public class BuildView { private final ConfiguredRuleClassProvider ruleClassProvider; - private final ArtifactFactory artifactFactory; - /** * A factory class to create the coverage report action. May be null. */ @@ -266,23 +264,9 @@ public class BuildView { this.packageManager = skyframeExecutor.getPackageManager(); this.binTools = binTools; this.coverageReportActionFactory = coverageReportActionFactory; - this.artifactFactory = new ArtifactFactory(directories.getExecRoot()); this.ruleClassProvider = ruleClassProvider; this.skyframeExecutor = Preconditions.checkNotNull(skyframeExecutor); - this.skyframeBuildView = - new SkyframeBuildView( - new ConfiguredTargetFactory(ruleClassProvider), - artifactFactory, - skyframeExecutor, - new Runnable() { - @Override - public void run() { - clear(); - } - }, - binTools, - ruleClassProvider); - skyframeExecutor.setSkyframeBuildView(skyframeBuildView); + this.skyframeBuildView = skyframeExecutor.getSkyframeBuildView(); } /** Returns the action graph. */ @@ -312,16 +296,8 @@ public class BuildView { skyframeBuildView.setTopLevelHostConfiguration(configurations.getHostConfiguration()); } - /** - * Clear the graphs of ConfiguredTargets and Artifacts. - */ - @VisibleForTesting - public void clear() { - artifactFactory.clear(); - } - public ArtifactFactory getArtifactFactory() { - return artifactFactory; + return skyframeBuildView.getArtifactFactory(); } @VisibleForTesting @@ -627,7 +603,6 @@ public class BuildView { skyframeExecutor.dropConfiguredTargets(); skyframeCacheWasInvalidated = true; - clear(); } skyframeAnalysisWasDiscarded = false; this.configurations = configurations; @@ -741,7 +716,7 @@ public class BuildView { actionsWrapper = coverageReportActionFactory.createCoverageReportActionsWrapper( allTargetsToTest, baselineCoverageArtifacts, - artifactFactory, + getArtifactFactory(), CoverageReportValue.ARTIFACT_OWNER); if (actionsWrapper != null) { ImmutableList actions = actionsWrapper.getActions(); @@ -952,7 +927,7 @@ public class BuildView { realPackageRoots.put(entry.getKey(), root); } // Source Artifact roots: - artifactFactory.setPackageRoots(realPackageRoots); + getArtifactFactory().setPackageRoots(realPackageRoots); // Derived Artifact roots: ImmutableList.Builder roots = ImmutableList.builder(); @@ -965,7 +940,7 @@ public class BuildView { roots.addAll(cfg.getRoots()); } - artifactFactory.setDerivedArtifactRoots(roots.build()); + getArtifactFactory().setDerivedArtifactRoots(roots.build()); } /** @@ -995,7 +970,7 @@ public class BuildView { BuildConfigurationCollection configurations) throws InterruptedException { BuildConfiguration config = target.getConfiguration(); CachingAnalysisEnvironment analysisEnvironment = - new CachingAnalysisEnvironment(artifactFactory, + new CachingAnalysisEnvironment(getArtifactFactory(), new ConfiguredTargetKey(target.getLabel(), config), /*isSystemEnv=*/false, config.extendedSanityChecks(), eventHandler, /*skyframeEnv=*/null, config.isActionsEnabled(), binTools); diff --git a/src/main/java/com/google/devtools/build/lib/analysis/CachingAnalysisEnvironment.java b/src/main/java/com/google/devtools/build/lib/analysis/CachingAnalysisEnvironment.java index bef594b828..4c885a3663 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/CachingAnalysisEnvironment.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/CachingAnalysisEnvironment.java @@ -103,7 +103,7 @@ public class CachingAnalysisEnvironment implements AnalysisEnvironment { this.errorEventListener = errorEventListener; this.skyframeEnv = env; this.allowRegisteringActions = allowRegisteringActions; - this.binTools = binTools; + this.binTools = Preconditions.checkNotNull(binTools); middlemanFactory = new MiddlemanFactory(artifactFactory, this); artifacts = new HashMap<>(); } diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java b/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java index 368812f671..6584af68ec 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java @@ -621,7 +621,6 @@ public final class BlazeRuntime { /** Removes skyframe cache and other caches that must be kept synchronized with skyframe. */ private void clearSkyframeRelevantCaches() { skyframeExecutor.resetEvaluator(); - view.clear(); } /** @@ -1598,6 +1597,7 @@ public final class BlazeRuntime { pkgFactory, timestampMonitor, directories, + binTools, workspaceStatusActionFactory, ruleClassProvider.getBuildInfoFactories(), immutableDirectories, diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java index 6461e3f085..9f677143aa 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java @@ -31,6 +31,7 @@ import com.google.devtools.build.lib.analysis.ConfiguredTarget; import com.google.devtools.build.lib.analysis.WorkspaceStatusAction; import com.google.devtools.build.lib.analysis.WorkspaceStatusAction.Factory; import com.google.devtools.build.lib.analysis.buildinfo.BuildInfoFactory; +import com.google.devtools.build.lib.analysis.config.BinTools; import com.google.devtools.build.lib.cmdline.PackageIdentifier; import com.google.devtools.build.lib.events.Reporter; import com.google.devtools.build.lib.packages.Package; @@ -100,6 +101,7 @@ public final class SequencedSkyframeExecutor extends SkyframeExecutor { PackageFactory pkgFactory, TimestampGranularityMonitor tsgm, BlazeDirectories directories, + BinTools binTools, Factory workspaceStatusActionFactory, ImmutableList buildInfoFactories, Set immutableDirectories, @@ -115,6 +117,7 @@ public final class SequencedSkyframeExecutor extends SkyframeExecutor { pkgFactory, tsgm, directories, + binTools, workspaceStatusActionFactory, buildInfoFactories, immutableDirectories, @@ -132,6 +135,7 @@ public final class SequencedSkyframeExecutor extends SkyframeExecutor { PackageFactory pkgFactory, TimestampGranularityMonitor tsgm, BlazeDirectories directories, + BinTools binTools, Factory workspaceStatusActionFactory, ImmutableList buildInfoFactories, Set immutableDirectories, @@ -148,6 +152,7 @@ public final class SequencedSkyframeExecutor extends SkyframeExecutor { pkgFactory, tsgm, directories, + binTools, workspaceStatusActionFactory, buildInfoFactories, immutableDirectories, @@ -163,7 +168,7 @@ public final class SequencedSkyframeExecutor extends SkyframeExecutor { @VisibleForTesting public static SequencedSkyframeExecutor create(Reporter reporter, PackageFactory pkgFactory, - TimestampGranularityMonitor tsgm, BlazeDirectories directories, + TimestampGranularityMonitor tsgm, BlazeDirectories directories, BinTools binTools, WorkspaceStatusAction.Factory workspaceStatusActionFactory, ImmutableList buildInfoFactories, Set immutableDirectories, @@ -173,6 +178,7 @@ public final class SequencedSkyframeExecutor extends SkyframeExecutor { pkgFactory, tsgm, directories, + binTools, workspaceStatusActionFactory, buildInfoFactories, immutableDirectories, @@ -498,9 +504,8 @@ public final class SequencedSkyframeExecutor extends SkyframeExecutor { @Override public void dropConfiguredTargets() { - if (skyframeBuildView != null) { - skyframeBuildView.clearInvalidatedConfiguredTargets(); - } + skyframeBuildView.clearInvalidatedConfiguredTargets(); + skyframeBuildView.clearLegacyData(); memoizingEvaluator.delete( // We delete any value that can hold an action -- all subclasses of ActionLookupValue -- as // well as ActionExecutionValues, since they do not depend on ActionLookupValues. diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorFactory.java b/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorFactory.java index b39707f3f4..df68c74191 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorFactory.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutorFactory.java @@ -19,6 +19,7 @@ import com.google.common.collect.ImmutableMap; import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.analysis.WorkspaceStatusAction.Factory; import com.google.devtools.build.lib.analysis.buildinfo.BuildInfoFactory; +import com.google.devtools.build.lib.analysis.config.BinTools; import com.google.devtools.build.lib.events.Reporter; import com.google.devtools.build.lib.packages.PackageFactory; import com.google.devtools.build.lib.packages.Preprocessor; @@ -41,6 +42,7 @@ public class SequencedSkyframeExecutorFactory implements SkyframeExecutorFactory PackageFactory pkgFactory, TimestampGranularityMonitor tsgm, BlazeDirectories directories, + BinTools binTools, Factory workspaceStatusActionFactory, ImmutableList buildInfoFactories, Set immutableDirectories, @@ -55,6 +57,7 @@ public class SequencedSkyframeExecutorFactory implements SkyframeExecutorFactory pkgFactory, tsgm, directories, + binTools, workspaceStatusActionFactory, buildInfoFactories, immutableDirectories, 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 718fd5037b..0c6d78e800 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 @@ -34,8 +34,10 @@ import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictEx import com.google.devtools.build.lib.analysis.AnalysisEnvironment; import com.google.devtools.build.lib.analysis.AnalysisFailureEvent; import com.google.devtools.build.lib.analysis.Aspect; +import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.analysis.CachingAnalysisEnvironment; import com.google.devtools.build.lib.analysis.ConfiguredAspectFactory; +import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider; import com.google.devtools.build.lib.analysis.ConfiguredTarget; import com.google.devtools.build.lib.analysis.ConfiguredTargetFactory; import com.google.devtools.build.lib.analysis.LabelAndConfiguration; @@ -88,7 +90,6 @@ public final class SkyframeBuildView { private final ConfiguredTargetFactory factory; private final ArtifactFactory artifactFactory; private final SkyframeExecutor skyframeExecutor; - private final Runnable legacyDataCleaner; private final BinTools binTools; private boolean enableAnalysis = false; @@ -115,16 +116,14 @@ public final class SkyframeBuildView { private Map>, BuildConfiguration> hostConfigurationCache = Maps.newConcurrentMap(); - public SkyframeBuildView(ConfiguredTargetFactory factory, ArtifactFactory artifactFactory, - SkyframeExecutor skyframeExecutor, Runnable legacyDataCleaner, BinTools binTools, - RuleClassProvider ruleClassProvider) { - this.factory = factory; - this.artifactFactory = artifactFactory; + public SkyframeBuildView(BlazeDirectories directories, + SkyframeExecutor skyframeExecutor, BinTools binTools, + ConfiguredRuleClassProvider ruleClassProvider) { + this.factory = new ConfiguredTargetFactory(ruleClassProvider); + this.artifactFactory = new ArtifactFactory(directories.getExecRoot()); this.skyframeExecutor = skyframeExecutor; - this.legacyDataCleaner = legacyDataCleaner; this.binTools = binTools; this.ruleClassProvider = ruleClassProvider; - skyframeExecutor.setArtifactFactoryAndBinTools(artifactFactory, binTools); } public void resetEvaluatedConfiguredTargetKeysSet() { @@ -375,7 +374,7 @@ public final class SkyframeBuildView { } } - ArtifactFactory getArtifactFactory() { + public ArtifactFactory getArtifactFactory() { return artifactFactory; } @@ -480,12 +479,12 @@ public final class SkyframeBuildView { } /** - * Workaround to clear all legacy data, like the action graph and the artifact factory. We need + * Workaround to clear all legacy data, like the artifact factory. We need * to clear them to avoid conflicts. * TODO(bazel-team): Remove this workaround. [skyframe-execution] */ void clearLegacyData() { - legacyDataCleaner.run(); + artifactFactory.clear(); } /** 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 443a444452..bc624d1837 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 @@ -49,6 +49,7 @@ import com.google.devtools.build.lib.analysis.Aspect; import com.google.devtools.build.lib.analysis.AspectWithParameters; import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.analysis.BuildView.Options; +import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider; import com.google.devtools.build.lib.analysis.ConfiguredTarget; import com.google.devtools.build.lib.analysis.DependencyResolver.Dependency; import com.google.devtools.build.lib.analysis.RuleConfiguredTarget; @@ -229,7 +230,7 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory { private final Set immutableDirectories; - private BinTools binTools = null; + private final BinTools binTools; private boolean needToInjectEmbeddedArtifacts = true; private boolean needToInjectPrecomputedValuesForAnalysis = true; protected int modifiedFiles; @@ -256,6 +257,7 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory { PackageFactory pkgFactory, TimestampGranularityMonitor tsgm, BlazeDirectories directories, + BinTools binTools, Factory workspaceStatusActionFactory, ImmutableList buildInfoFactories, Set immutableDirectories, @@ -288,6 +290,14 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory { this.extraSkyFunctions = extraSkyFunctions; this.extraPrecomputedValues = extraPrecomputedValues; this.errorOnExternalFiles = errorOnExternalFiles; + this.binTools = binTools; + + this.skyframeBuildView = new SkyframeBuildView( + directories, + this, + binTools, + (ConfiguredRuleClassProvider) pkgFactory.getRuleClassProvider()); + this.artifactFactory.set(skyframeBuildView.getArtifactFactory()); } private ImmutableMap skyFunctions( @@ -479,9 +489,7 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory { public void resetEvaluator() { init(); emittedEventState.clear(); - if (skyframeBuildView != null) { - skyframeBuildView.clearLegacyData(); - } + skyframeBuildView.clearLegacyData(); reinjectConstantValuesLazily(); } @@ -890,13 +898,8 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory { } } - /** - * Specifies the current {@link SkyframeBuildView} instance. This should only be set once over the - * lifetime of the Blaze server, except in tests. - */ - public void setSkyframeBuildView(SkyframeBuildView skyframeBuildView) { - this.skyframeBuildView = skyframeBuildView; - this.artifactFactory.set(skyframeBuildView.getArtifactFactory()); + public SkyframeBuildView getSkyframeBuildView() { + return skyframeBuildView; } /** @@ -1062,11 +1065,6 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory { public ImmutableMap getConfiguredTargetMap( BuildConfiguration originalConfig, Iterable keys, boolean useOriginalConfig) { checkActive(); - if (skyframeBuildView == null) { - // If build view has not yet been initialized, no configured targets can have been created. - // This is most likely to happen after a failed loading phase. - return ImmutableMap.of(); - } Map configs; if (originalConfig != null) { @@ -1620,15 +1618,6 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory { this.statusReporterRef.set(statusReporter); } - /** - * This should be called at most once in the lifetime of the SkyframeExecutor (except for - * tests), and it should be called before the execution phase. - */ - void setArtifactFactoryAndBinTools(ArtifactFactory artifactFactory, BinTools binTools) { - this.artifactFactory.set(artifactFactory); - this.binTools = binTools; - } - public void prepareExecution(boolean checkOutputFiles, @Nullable Range lastExecutionTimeRange) throws AbruptExitException, InterruptedException { @@ -1704,9 +1693,7 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory { if (ignoreInvalidations) { return; } - if (skyframeBuildView != null) { - skyframeBuildView.getInvalidationReceiver().invalidated(skyKey, state); - } + skyframeBuildView.getInvalidationReceiver().invalidated(skyKey, state); } @Override @@ -1714,9 +1701,7 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory { if (ignoreInvalidations) { return; } - if (skyframeBuildView != null) { - skyframeBuildView.getInvalidationReceiver().enqueueing(skyKey); - } + skyframeBuildView.getInvalidationReceiver().enqueueing(skyKey); if (executionProgressReceiver != null) { executionProgressReceiver.enqueueing(skyKey); } @@ -1730,9 +1715,7 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory { if (ignoreInvalidations) { return; } - if (skyframeBuildView != null) { - skyframeBuildView.getInvalidationReceiver().evaluated(skyKey, valueSupplier, state); - } + skyframeBuildView.getInvalidationReceiver().evaluated(skyKey, valueSupplier, state); if (executionProgressReceiver != null) { executionProgressReceiver.evaluated(skyKey, valueSupplier, state); } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutorFactory.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutorFactory.java index 7710fb0b67..f5e3ae5d64 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutorFactory.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutorFactory.java @@ -19,6 +19,7 @@ import com.google.common.collect.ImmutableMap; import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.analysis.WorkspaceStatusAction.Factory; import com.google.devtools.build.lib.analysis.buildinfo.BuildInfoFactory; +import com.google.devtools.build.lib.analysis.config.BinTools; import com.google.devtools.build.lib.events.Reporter; import com.google.devtools.build.lib.packages.PackageFactory; import com.google.devtools.build.lib.packages.Preprocessor; @@ -43,6 +44,7 @@ public interface SkyframeExecutorFactory { * @param pkgFactory the package factory * @param tsgm timestamp granularity monitor * @param directories Blaze directories + * @param binTools the embedded tools * @param workspaceStatusActionFactory a factory for creating WorkspaceStatusAction objects * @param buildInfoFactories list of BuildInfoFactories * @param diffAwarenessFactories @@ -59,6 +61,7 @@ public interface SkyframeExecutorFactory { PackageFactory pkgFactory, TimestampGranularityMonitor tsgm, BlazeDirectories directories, + BinTools binTools, Factory workspaceStatusActionFactory, ImmutableList buildInfoFactories, Set immutableDirectories, -- cgit v1.2.3