From 13459b4ee0e743da8f8416fc16bce64687f0c406 Mon Sep 17 00:00:00 2001 From: Michajlo Matijkiw Date: Thu, 12 Mar 2015 19:43:20 +0000 Subject: add baseline functionality for not saving unused artifacts We define unused artifacts as those that aren't consumed by any action. This can be because an action produced more outputs than a dependent action needed, or because it's a top level artifact and we don't care about its contents, just that it was built without issue. Actions may prevent outputs from being discarded by declaring them as mandatory. This is particularly useful for test outputs. The motivation behind this change is to reduce storage overhead for things we can do without. It is worth noting this change doesn't cover all cases. In particular it has difficulty identifying *_binary artifacts as orphaned. This is due to the insertion of a virtual runfiles artifact which depends upon the rule's outputs. -- MOS_MIGRATED_REVID=88467504 --- .../devtools/build/lib/actions/AbstractAction.java | 5 +++++ .../devtools/build/lib/actions/ActionCacheChecker.java | 10 ++++++---- .../devtools/build/lib/actions/ActionMetadata.java | 8 ++++++++ .../build/lib/actions/cache/MetadataHandler.java | 18 +++++++++++++++++- .../build/lib/rules/test/TestRunnerAction.java | 6 ++++++ .../build/lib/skyframe/FileAndMetadataCache.java | 16 ++++++++++++++++ .../devtools/build/lib/skyframe/FileArtifactValue.java | 12 ++++++++++++ .../build/lib/skyframe/SkyframeActionExecutor.java | 18 ++++++++++++++++-- .../build/lib/actions/ResourceManagerTest.java | 5 +++++ 9 files changed, 91 insertions(+), 7 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java b/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java index 82d9248bfc..09457474bd 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java +++ b/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java @@ -335,6 +335,11 @@ public abstract class AbstractAction implements Action { .setMnemonic(getMnemonic()); } + @Override + public ImmutableSet getMandatoryOutputs() { + return ImmutableSet.of(); + } + /** * Returns input files that need to be present to allow extra_action rules to shadow this action * correctly when run remotely. This is at least the normal inputs of the action, but may include diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java b/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java index 07b997785f..f40003b1a5 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java +++ b/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java @@ -216,10 +216,12 @@ public class ActionCacheChecker { if (!key.equals(execPath)) { actionCache.remove(key); } - // Output files *must* exist and be accessible after successful action execution. - Metadata metadata = metadataHandler.getMetadata(output); - Preconditions.checkState(metadata != null); - entry.addFile(output.getExecPath(), metadata); + if (!metadataHandler.artifactOmitted(output)) { + // Output files *must* exist and be accessible after successful action execution. + Metadata metadata = metadataHandler.getMetadata(output); + Preconditions.checkState(metadata != null); + entry.addFile(output.getExecPath(), metadata); + } } for (Artifact input : action.getInputs()) { entry.addFile(input.getExecPath(), metadataHandler.getMetadataMaybe(input)); diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionMetadata.java b/src/main/java/com/google/devtools/build/lib/actions/ActionMetadata.java index 3569f07a8b..64813aed07 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/ActionMetadata.java +++ b/src/main/java/com/google/devtools/build/lib/actions/ActionMetadata.java @@ -123,6 +123,14 @@ public interface ActionMetadata { */ ImmutableSet getOutputs(); + /** + * Returns the set of output Artifacts that are required to be saved. This is + * used to identify items that would otherwise be potentially identified as + * orphaned (not consumed by any downstream {@link Action}s and potentially + * discarded during the build process. + */ + public ImmutableSet getMandatoryOutputs(); + /** * Returns the "primary" input of this action, if applicable. * diff --git a/src/main/java/com/google/devtools/build/lib/actions/cache/MetadataHandler.java b/src/main/java/com/google/devtools/build/lib/actions/cache/MetadataHandler.java index 9d288dbbff..61a5d6d211 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/cache/MetadataHandler.java +++ b/src/main/java/com/google/devtools/build/lib/actions/cache/MetadataHandler.java @@ -48,11 +48,27 @@ public interface MetadataHandler { */ void injectDigest(ActionInput output, FileStatus statNoFollow, byte[] digest); - /** Returns true iff artifact exists. */ + /** + * Marks an artifact as intentionally omitted. Acknowledges that this Artifact could have + * existed, but was intentionally not saved, most likely as an optimization. + */ + void markOmitted(ActionInput output); + + /** + * Returns true iff artifact exists. + * + *

It is important to note that implementations may cache non-existence as a side effect + * of this method. If there is a possibility an artifact was intentionally omitted then + * {@link #artifactOmitted(Artifact)} should be checked first to avoid the side effect.

+ */ boolean artifactExists(Artifact artifact); + /** Returns true iff artifact is a regular file. */ boolean isRegularFile(Artifact artifact); + /** Returns true iff artifact was intentionally omitted (not saved). */ + boolean artifactOmitted(Artifact artifact); + /** * @return Whether the artifact's data was injected. * @throws IOException if implementation tried to stat artifact which threw an exception. diff --git a/src/main/java/com/google/devtools/build/lib/rules/test/TestRunnerAction.java b/src/main/java/com/google/devtools/build/lib/rules/test/TestRunnerAction.java index 13ec62cedc..9e9c180360 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/test/TestRunnerAction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/test/TestRunnerAction.java @@ -17,6 +17,7 @@ package com.google.devtools.build.lib.rules.test; import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; import com.google.devtools.build.lib.actions.AbstractAction; import com.google.devtools.build.lib.actions.ActionExecutionContext; @@ -535,6 +536,11 @@ public class TestRunnerAction extends AbstractAction implements NotifyOnActionCa return "TestRunner"; } + @Override + public ImmutableSet getMandatoryOutputs() { + return getOutputs(); + } + /** * The same set of paths as the parent test action, resolved against a given exec root. */ diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/FileAndMetadataCache.java b/src/main/java/com/google/devtools/build/lib/skyframe/FileAndMetadataCache.java index 2a0de78064..5014c0f5f4 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/FileAndMetadataCache.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/FileAndMetadataCache.java @@ -76,6 +76,7 @@ public class FileAndMetadataCache implements ActionInputFileCache, MetadataHandl private final Map reverseMap = new ConcurrentHashMap<>(); private final ConcurrentMap outputArtifactData = new ConcurrentHashMap<>(); + private final Set omittedOutputs = Sets.newConcurrentHashSet(); // See #getAdditionalOutputData for documentation of this field. private final ConcurrentMap additionalOutputData = new ConcurrentHashMap<>(); @@ -327,6 +328,20 @@ public class FileAndMetadataCache implements ActionInputFileCache, MetadataHandl } } + @Override + public void markOmitted(ActionInput output) { + if (output instanceof Artifact) { + Artifact artifact = (Artifact) output; + Preconditions.checkState(omittedOutputs.add(artifact), artifact); + additionalOutputData.put(artifact, FileArtifactValue.OMITTED_FILE_MARKER); + } + } + + @Override + public boolean artifactOmitted(Artifact artifact) { + return omittedOutputs.contains(artifact); + } + @Override public void discardMetadata(Collection artifactList) { Preconditions.checkState(injectedArtifacts.isEmpty(), @@ -337,6 +352,7 @@ public class FileAndMetadataCache implements ActionInputFileCache, MetadataHandl @Override public boolean artifactExists(Artifact artifact) { + Preconditions.checkState(!artifactOmitted(artifact), artifact); return getMetadataMaybe(artifact) != null; } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/FileArtifactValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/FileArtifactValue.java index 7acc38c994..f1f0b3e12a 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/FileArtifactValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/FileArtifactValue.java @@ -36,6 +36,18 @@ public class FileArtifactValue extends ArtifactValue { /** Data that marks that a file is not present on the filesystem. */ static final FileArtifactValue MISSING_FILE_MARKER = new FileArtifactValue(null, 1, 0); + /** + * Represents an omitted file- we are aware of it but it doesn't exist. All access methods + * are unsupported. + */ + static final FileArtifactValue OMITTED_FILE_MARKER = new FileArtifactValue(null, 2, 0) { + @Override public byte[] getDigest() { throw new UnsupportedOperationException(); } + @Override public long getSize() { throw new UnsupportedOperationException(); } + @Override public long getModifiedTime() { throw new UnsupportedOperationException(); } + @Override public boolean equals(Object o) { return this == o; } + @Override public String toString() { return "OMITTED_FILE_MARKER"; } + }; + @Nullable private final byte[] digest; private final long mtime; private final long size; diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java index ebae390f50..7b71fba451 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java @@ -268,6 +268,17 @@ public final class SkyframeActionExecutor { // intra-build. perActionHandler.discardMetadata(artifactList); } + + @Override + public void markOmitted(ActionInput output) { + perActionHandler.markOmitted(output); + } + + @Override + public boolean artifactOmitted(Artifact artifact) { + return perActionHandler.artifactOmitted(artifact); + } + } /** @@ -944,14 +955,17 @@ public final class SkyframeActionExecutor { } /** - * Validates that all action outputs were created. + * Validates that all action outputs were created or intentionally omitted. * * @return false if some outputs are missing, true - otherwise. */ private boolean checkOutputs(Action action, MetadataHandler metadataHandler) { boolean success = true; for (Artifact output : action.getOutputs()) { - if (!metadataHandler.artifactExists(output)) { + // artifactExists has the side effect of potentially adding the artifact to the cache, + // therefore we only call it if we know the artifact is indeed not omitted to avoid any + // unintended side effects. + if (!(metadataHandler.artifactOmitted(output) || metadataHandler.artifactExists(output))) { reportMissingOutputFile(action, output, reporter, output.getPath().isSymbolicLink()); success = false; } diff --git a/src/test/java/com/google/devtools/build/lib/actions/ResourceManagerTest.java b/src/test/java/com/google/devtools/build/lib/actions/ResourceManagerTest.java index c8e0af4bee..8b2191e99a 100644 --- a/src/test/java/com/google/devtools/build/lib/actions/ResourceManagerTest.java +++ b/src/test/java/com/google/devtools/build/lib/actions/ResourceManagerTest.java @@ -431,5 +431,10 @@ public class ResourceManagerTest { public String describeKey() { throw new IllegalStateException(); } + + @Override + public ImmutableSet getMandatoryOutputs() { + return ImmutableSet.of(); + } } } -- cgit v1.2.3