diff options
author | ccalvarin <ccalvarin@google.com> | 2017-12-11 15:19:04 -0800 |
---|---|---|
committer | Copybara-Service <copybara-piper@google.com> | 2017-12-11 15:20:45 -0800 |
commit | 66693e77d0df7ab3ea1f4e8a3b7b66fabbc2af77 (patch) | |
tree | 3195ab0d6e7372d0f214cfa4b6065b91e6159549 /src/main/java/com | |
parent | 34ba7026907cfae4abfc0be2851edb48c3ee2701 (diff) |
Automated rollback of commit 9321316b34767b06c3071b2cf2a4de189874fcce.
*** Reason for rollback ***
Design change, 2 boolean flags instead of 1 enum flag
*** Original change description ***
Add --incremental_state_retention_strategy
This option is intended to replace some of the uses of --batch. It lets users specify that builds should not be incremental, and how eagerly to discard the state that is kept around for incrementality. Note that for both values discard_eargerly and keep_for_life_of_build, the build graph is kept around until the next build. This may change.
Will add tests for keep_for_life_of_build in a later change, for now it will warn that that feature is experimen...
***
ROLLBACK_OF=178661777
RELNOTES: None.
PiperOrigin-RevId: 178681472
Diffstat (limited to 'src/main/java/com')
5 files changed, 55 insertions, 107 deletions
diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequestOptions.java b/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequestOptions.java index d769c68ece..d193bb3064 100644 --- a/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequestOptions.java +++ b/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequestOptions.java @@ -20,7 +20,6 @@ import com.google.devtools.build.lib.util.OptionsUtils; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.common.options.Converters; import com.google.devtools.common.options.Converters.RangeConverter; -import com.google.devtools.common.options.EnumConverter; import com.google.devtools.common.options.Option; import com.google.devtools.common.options.OptionDocumentationCategory; import com.google.devtools.common.options.OptionEffectTag; @@ -405,51 +404,18 @@ public class BuildRequestOptions extends OptionsBase { ) public boolean useActionCache; - /** - * Timeline for retaining inmemory incremental state. - * - * <ol> - * <li>{@code KEEP_FOREVER} - the default behavior, maintains incrementality state across builds - * to minimize cost of subsequent builds. - * <li>{@code KEEP_FOR_LIFE_OF_BUILD} - during this build, maintains incrementality state like - * KEEP_FOREVER, but discards it at the end of the build. The following build will not be - * incremental. - * <li>{@code DISCARD_EAGERLY} - memory saving mode, eagerly discards state that would have been - * useful for subsequent builds but is not necessary for this one. The following build will - * not be incremental. - * </ol> - */ - public enum IncrementalStateRetentionStrategy { - KEEP_FOREVER, - KEEP_FOR_LIFE_OF_BUILD, - DISCARD_EAGERLY - } - - /** Option converter for {@link IncrementalStateRetentionStrategy}. */ - public static class IncrementalStateRetentionStrategyConverter - extends EnumConverter<IncrementalStateRetentionStrategy> { - public IncrementalStateRetentionStrategyConverter() { - super( - IncrementalStateRetentionStrategy.class, - "Timeline for retaining inmemory incremental state"); - } - } - @Option( - name = "incremental_state_retention_strategy", - defaultValue = "KEEP_FOREVER", + name = "keep_incrementality_data", + defaultValue = "true", documentationCategory = OptionDocumentationCategory.BUILD_TIME_OPTIMIZATION, effectTags = {OptionEffectTag.LOSES_INCREMENTAL_STATE}, - metadataTags = {OptionMetadataTag.EXPERIMENTAL}, - converter = IncrementalStateRetentionStrategyConverter.class, help = - "Controls how long Blaze-internal data that allows for invalidation and re-evaluation " - + "on incremental builds is kept. Default is keep_forever, which allows for " - + "incremental builds. Non-incremental options are keep_for_life_of_build, which " - + "discards state at the end of a build, and discard_eagerly, which saves memory " - + "during the build by discarding state eagerly." + "If false, discard Blaze-internal data that allows for invalidation and re-evaluation " + + "on incremental builds in order to save memory on this build. Subsequent builds " + + "will not have any incrementality with respect to this one. Usually you will want" + + "to specify the --batch startup option along with this one." ) - public IncrementalStateRetentionStrategy incrementalityStateRetentionStrategy; + public boolean keepIncrementalityData; /** Converter for jobs: [0, MAX_JOBS] or "auto". */ public static class JobsConverter extends RangeConverter { diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java b/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java index 2e5f599692..9f20f76622 100644 --- a/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java +++ b/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java @@ -438,7 +438,7 @@ public class ExecutionTool { skyframeExecutor.drainChangedFiles(); if (request.getViewOptions().discardAnalysisCache - || !skyframeExecutor.hasIncrementalState()) { + || !request.getBuildOptions().keepIncrementalityData) { // Free memory by removing cache entries that aren't going to be needed. env.getSkyframeBuildView() .clearAnalysisCache(analysisResult.getTargetsToBuild(), analysisResult.getAspects()); @@ -666,7 +666,7 @@ public class ExecutionTool { return successfulTargets; } - /** Get action cache if present or reload it from the on-disk cache. */ + private ActionCache getActionCache() throws LocalEnvironmentException { try { return env.getPersistentActionCache(); diff --git a/src/main/java/com/google/devtools/build/lib/runtime/CommandEnvironment.java b/src/main/java/com/google/devtools/build/lib/runtime/CommandEnvironment.java index 2c11eac9e0..54552a6c5f 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/CommandEnvironment.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/CommandEnvironment.java @@ -605,7 +605,7 @@ public final class CommandEnvironment { // Fail fast in the case where a Blaze command forgets to install the package path correctly. skyframeExecutor.setActive(false); - // Let skyframe figure out how much incremental state it will be keeping. + // Let skyframe figure out if it needs to store graph edges for this build. skyframeExecutor.decideKeepIncrementalState( runtime.getStartupOptionsProvider().getOptions(BlazeServerStartupOptions.class).batch, options, 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 e6c4137327..f3e4bf4da0 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 @@ -34,7 +34,6 @@ 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.configuredtargets.RuleConfiguredTarget; import com.google.devtools.build.lib.buildtool.BuildRequestOptions; -import com.google.devtools.build.lib.buildtool.BuildRequestOptions.IncrementalStateRetentionStrategy; import com.google.devtools.build.lib.cmdline.PackageIdentifier; import com.google.devtools.build.lib.concurrent.Uninterruptibles; import com.google.devtools.build.lib.events.Event; @@ -108,14 +107,17 @@ public final class SequencedSkyframeExecutor extends SkyframeExecutor { private boolean lastAnalysisDiscarded = false; + private enum IncrementalState { + NORMAL, + CLEAR_EDGES_AND_ACTIONS + } + /** - * Assume that this build is incremental. If {@link - * IncrementalStateRetentionStrategy#DISCARD_EAGERLY}, the graph will not store edges, saving + * If {@link IncrementalState#CLEAR_EDGES_AND_ACTIONS}, the graph will not store edges, saving * memory but making subsequent builds not incremental. Also, each action will be dereferenced * once it is executed, saving memory. */ - private IncrementalStateRetentionStrategy incrementalState = - IncrementalStateRetentionStrategy.KEEP_FOREVER; + private IncrementalState incrementalState = IncrementalState.NORMAL; private boolean evaluatorNeedsReset = false; @@ -502,71 +504,52 @@ public final class SequencedSkyframeExecutor extends SkyframeExecutor { SkyFunctionName.functionIs(SkyFunctions.FILE_STATE))); } - /** - * {@inheritDoc} - * - * <p>Necessary conditions to not store graph edges are either - * - * <ol> - * <li>batch (since incremental builds are not possible) and discard_analysis_cache (since - * otherwise user isn't concerned about saving memory this way). - * <li>incremental_state_retention_strategy set to discard_eagerly. - * </ol> - */ @Override public void decideKeepIncrementalState( boolean batch, OptionsProvider options, EventHandler eventHandler) { Preconditions.checkState(!active); BuildView.Options viewOptions = options.getOptions(BuildView.Options.class); BuildRequestOptions requestOptions = options.getOptions(BuildRequestOptions.class); - if (requestOptions != null - && requestOptions.incrementalityStateRetentionStrategy.equals( - IncrementalStateRetentionStrategy.KEEP_FOR_LIFE_OF_BUILD)) { - eventHandler.handle( - Event.warn( - "Incremental state retention strategy keep_for_life_of_build is experimental, use " - + "with caution.")); - } - - // First check if the incrementality state should be kept around during the build. - IncrementalStateRetentionStrategy explicitStrategyOrDefault = - requestOptions != null - ? requestOptions.incrementalityStateRetentionStrategy - : IncrementalStateRetentionStrategy.KEEP_FOREVER; - boolean explicitlyRequestedNoIncrementalState = - explicitStrategyOrDefault.equals(IncrementalStateRetentionStrategy.DISCARD_EAGERLY); - boolean implicitlyRequestedNoIncrementalState = + boolean explicitlyRequestedNoIncrementalData = + requestOptions != null && !requestOptions.keepIncrementalityData; + boolean implicitlyRequestedNoIncrementalData = batch && viewOptions != null && viewOptions.discardAnalysisCache; boolean discardingEdges = - explicitlyRequestedNoIncrementalState || implicitlyRequestedNoIncrementalState; - if (!explicitlyRequestedNoIncrementalState && implicitlyRequestedNoIncrementalState) { - eventHandler.handle( - Event.warn( - "--batch and --discard_analysis_cache specified, but " - + "--incremental_state_retention_strategy not set to discard_eagerly: " - + "incrementality data is implicitly discarded, but you may need to specify " - + "--incremental_state_retention_strategy=discard_eagerly in the future if " - + "you want to maximize memory savings.")); + explicitlyRequestedNoIncrementalData || implicitlyRequestedNoIncrementalData; + if (explicitlyRequestedNoIncrementalData != implicitlyRequestedNoIncrementalData) { + if (requestOptions != null && !explicitlyRequestedNoIncrementalData) { + eventHandler.handle( + Event.warn( + "--batch and --discard_analysis_cache specified, but --nokeep_incrementality_data " + + "not specified: incrementality data is implicitly discarded, but you may need" + + " to specify --nokeep_incrementality_data in the future if you want to " + + "maximize memory savings.")); + } + if (!batch) { + eventHandler.handle( + Event.warn( + "--batch not specified with --nokeep_incrementality_data: the server will " + + "remain running, but the next build will not be incremental on this one.")); + } } - - // Now check if it is necessary to wipe the previous state. We do this if either the previous - // or current incrementalStateRetentionStrategy requires the build to have been isolated. - IncrementalStateRetentionStrategy oldState = incrementalState; + IncrementalState oldState = incrementalState; incrementalState = - discardingEdges - ? IncrementalStateRetentionStrategy.DISCARD_EAGERLY - : explicitStrategyOrDefault; - if (!incrementalState.equals(IncrementalStateRetentionStrategy.KEEP_FOREVER) - || !oldState.equals(IncrementalStateRetentionStrategy.KEEP_FOREVER)) { - logger.info("Old incremental state will be wiped, retention strategy is " + incrementalState); + discardingEdges ? IncrementalState.CLEAR_EDGES_AND_ACTIONS : IncrementalState.NORMAL; + if (oldState != incrementalState) { + logger.info("Set incremental state to " + incrementalState); + evaluatorNeedsReset = true; + removeActionsAfterEvaluation.set( + incrementalState == IncrementalState.CLEAR_EDGES_AND_ACTIONS); + } else if (incrementalState == IncrementalState.CLEAR_EDGES_AND_ACTIONS) { evaluatorNeedsReset = true; - removeActionsAfterEvaluation.set(discardingEdges); } } @Override public boolean hasIncrementalState() { - return !incrementalState.equals(IncrementalStateRetentionStrategy.DISCARD_EAGERLY); + // TODO(bazel-team): Combine this method with clearSkyframeRelevantCaches() once legacy + // execution is removed [skyframe-execution]. + return incrementalState == IncrementalState.NORMAL; } @Override 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 c91a693029..da4d4efef1 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 @@ -686,19 +686,21 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory { } /** - * Decides if graph edges should be stored during this evaluation and checks if the state from the - * last evaluation, if any, can be kept. + * Decides if graph edges should be stored for this build. If not, notes that the next evaluation + * on the graph should reset it first. Necessary conditions to not store graph edges are: * - * <p>If not, it will mark this state for deletion. The actual cleaning is put off until {@link - * #sync}, in case no evaluation was actually called for and the existing state can be kept for - * longer. + * <ol> + * <li>batch (since incremental builds are not possible); + * <li>keep-going (since otherwise bubbling errors up may require edges of done nodes); + * <li>discard_analysis_cache (since otherwise user isn't concerned about saving memory this + * way). + * </ol> */ public void decideKeepIncrementalState( boolean batch, OptionsProvider viewOptions, EventHandler eventHandler) { // Assume incrementality. } - /** Whether this executor tracks state for the purpose of improving incremental performance. */ public boolean hasIncrementalState() { return true; } @@ -1856,9 +1858,6 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory { return pkgFactory.getPackageBuilderHelperForTesting(); } - /** - * Initializes and syncs the graph with the given options, readying it for the next evaluation. - */ public void sync( ExtendedEventHandler eventHandler, PackageCacheOptions packageCacheOptions, |