From e6cce762d846f3915b834c278fcb7fa0072ea530 Mon Sep 17 00:00:00 2001 From: ajurkowski Date: Fri, 24 Jul 2020 16:06:19 -0700 Subject: [PATCH] Fix false sharing in action cache due to incorrect key computation for custom Starlark command lines. Actions constructing arguments in Starlark can define them as a lazy evaluation, which will be run when we are about to execute the action, using `Args.add_all`. `Args.add_all` offers a `expand_directories` feature which causes all of the directories in the args list to be replaced with the files in the directory at the time of evaluation. When this option is used in conjunction with `map_each`, the behavior is to first expand the directory and than run the provided function on each of the elements of the expanded array. `ArtifactExpander`, used to expand the directories, is not available before execution phase. Given `Action.getKey` can be called before that (and is for instance for shared actions detection), we currently compute the fingerprint without expanding directories. In fact, current implementation, calls the provided function on the directory itself rather than expanded list of files in it. As a result of that, if we have 2 functions which return the same value for the directory, but differ for the files in it, the key will not denote them as different. That leads to false sharing of actions through the action cache, which is based on action fingerprints. Action cache checks happen during execution phase, when we technically have the ability to expand the directories. Expand the fingerprinting function to take advantage of `ArtifactExpander` when it is provided to produce the key based on the result of actual function execution. Rework the `ActionExecutionFunction` to provide `ArtifactExpander` to logic computing keys for action caches. PiperOrigin-RevId: 323090803 --- .../build/lib/actions/ActionCacheChecker.java | 13 +- .../build/lib/actions/ActionKeyCacher.java | 49 ++-- .../build/lib/actions/CommandLine.java | 14 +- .../build/lib/actions/CommandLines.java | 9 +- .../analysis/actions/CustomCommandLine.java | 5 +- .../actions/ParameterFileWriteAction.java | 2 +- .../lib/analysis/actions/SpawnAction.java | 2 +- .../starlark/StarlarkCustomCommandLine.java | 135 +++++++-- .../lib/rules/java/JavaCompileAction.java | 4 +- .../lib/skyframe/ActionExecutionFunction.java | 46 ++- .../lib/skyframe/SkyframeActionExecutor.java | 27 +- .../lib/actions/ActionCacheCheckerTest.java | 11 +- .../lib/actions/CustomCommandLineTest.java | 2 +- .../google/devtools/build/lib/skylark/BUILD | 1 + ...arlarkRuleImplementationFunctionsTest.java | 270 +++++++++++++++++- 15 files changed, 494 insertions(+), 96 deletions(-) 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 7d463eedc904c0..d7e3a634940722 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 @@ -19,6 +19,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; +import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander; import com.google.devtools.build.lib.actions.cache.ActionCache; import com.google.devtools.build.lib.actions.cache.DigestUtils; import com.google.devtools.build.lib.actions.cache.MetadataHandler; @@ -249,6 +250,7 @@ public Token getTokenIfNeedToExecute( Map clientEnv, EventHandler handler, MetadataHandler metadataHandler, + ArtifactExpander artifactExpander, Map remoteDefaultPlatformProperties) { // TODO(bazel-team): (2010) For RunfilesAction/SymlinkAction and similar actions that // produce only symlinks we should not check whether inputs are valid at all - all that matters @@ -292,6 +294,7 @@ public Token getTokenIfNeedToExecute( entry, handler, metadataHandler, + artifactExpander, actionInputs, clientEnv, remoteDefaultPlatformProperties)) { @@ -307,11 +310,12 @@ public Token getTokenIfNeedToExecute( return null; } - protected boolean mustExecute( + private boolean mustExecute( Action action, @Nullable ActionCache.Entry entry, EventHandler handler, MetadataHandler metadataHandler, + ArtifactExpander artifactExpander, NestedSet actionInputs, Map clientEnv, Map remoteDefaultPlatformProperties) { @@ -336,9 +340,7 @@ protected boolean mustExecute( reportChanged(handler, action); actionCache.accountMiss(MissReason.DIFFERENT_FILES); return true; - } else if (!entry - .getActionKey() - .equals(action.getKey(actionKeyContext, /*artifactExpander=*/ null))) { + } else if (!entry.getActionKey().equals(action.getKey(actionKeyContext, artifactExpander))) { reportCommand(handler, action); actionCache.accountMiss(MissReason.DIFFERENT_ACTION_KEY); return true; @@ -382,6 +384,7 @@ public void updateActionCache( Action action, Token token, MetadataHandler metadataHandler, + ArtifactExpander artifactExpander, Map clientEnv, Map remoteDefaultPlatformProperties) throws IOException { @@ -397,7 +400,7 @@ public void updateActionCache( computeUsedEnv(action, clientEnv, remoteDefaultPlatformProperties); ActionCache.Entry entry = new ActionCache.Entry( - action.getKey(actionKeyContext, /*artifactExpander=*/ null), + action.getKey(actionKeyContext, artifactExpander), usedEnvironment, action.discoversInputs()); for (Artifact output : action.getOutputs()) { diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionKeyCacher.java b/src/main/java/com/google/devtools/build/lib/actions/ActionKeyCacher.java index b095660928d172..0abcd571a24829 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/ActionKeyCacher.java +++ b/src/main/java/com/google/devtools/build/lib/actions/ActionKeyCacher.java @@ -28,35 +28,44 @@ public abstract class ActionKeyCacher implements ActionAnalysisMetadata { @Override public final String getKey( ActionKeyContext actionKeyContext, @Nullable ArtifactExpander artifactExpander) { + // Only cache the key when it is given all necessary information to compute a correct key. + // Practically, most of the benefit of the cache comes from execution, which does provide the + // artifactExpander. + if (artifactExpander == null) { + return computeActionKey(actionKeyContext, null); + } + if (cachedKey == null) { synchronized (this) { if (cachedKey == null) { - try { - Fingerprint fp = new Fingerprint(); - // TODO(b/153904017): Make use of the provided artifactExpander and only cache if - // present. - computeKey(actionKeyContext, /*artifactExpander=*/ null, fp); - - // Add a bool indicating whether the execution platform was set. - fp.addBoolean(getExecutionPlatform() != null); - if (getExecutionPlatform() != null) { - // Add the execution platform information. - getExecutionPlatform().addTo(fp); - } - - fp.addStringMap(getExecProperties()); - - // Compute the actual key and store it. - cachedKey = fp.hexDigestAndReset(); - } catch (CommandLineExpansionException e) { - cachedKey = KEY_ERROR; - } + cachedKey = computeActionKey(actionKeyContext, artifactExpander); } } } return cachedKey; } + private String computeActionKey( + ActionKeyContext actionKeyContext, @Nullable ArtifactExpander artifactExpander) { + try { + Fingerprint fp = new Fingerprint(); + computeKey(actionKeyContext, artifactExpander, fp); + + // Add a bool indicating whether the execution platform was set. + fp.addBoolean(getExecutionPlatform() != null); + if (getExecutionPlatform() != null) { + // Add the execution platform information. + getExecutionPlatform().addTo(fp); + } + + fp.addStringMap(getExecProperties()); + // Compute the actual key and store it. + return fp.hexDigestAndReset(); + } catch (CommandLineExpansionException e) { + return KEY_ERROR; + } + } + /** * See the javadoc for {@link Action} and {@link ActionAnalysisMetadata#getKey} for the contract * of this method. diff --git a/src/main/java/com/google/devtools/build/lib/actions/CommandLine.java b/src/main/java/com/google/devtools/build/lib/actions/CommandLine.java index 1516fb01b73cff..e369905ed156ee 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/CommandLine.java +++ b/src/main/java/com/google/devtools/build/lib/actions/CommandLine.java @@ -22,6 +22,7 @@ import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec; import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec.VisibleForSerialization; import com.google.devtools.build.lib.util.Fingerprint; +import javax.annotation.Nullable; /** A representation of a list of arguments. */ public abstract class CommandLine { @@ -52,7 +53,18 @@ public Iterable arguments(ArtifactExpander artifactExpander) return arguments(); } - public void addToFingerprint(ActionKeyContext actionKeyContext, Fingerprint fingerprint) + /** + * Adds the command line to the provided {@link Fingerprint}. + * + *

Some of the implementations may require the to expand provided directory in order to produce + * a unique key. Consequently, the result of calling this function can be different depending on + * whether the {@link ArtifactExpander} is provided. Moreover, without it, the produced key may + * not always be unique. + */ + public void addToFingerprint( + ActionKeyContext actionKeyContext, + @Nullable ArtifactExpander artifactExpander, + Fingerprint fingerprint) throws CommandLineExpansionException { for (String s : arguments()) { fingerprint.addString(s); diff --git a/src/main/java/com/google/devtools/build/lib/actions/CommandLines.java b/src/main/java/com/google/devtools/build/lib/actions/CommandLines.java index b1923b9cca96e7..5db8d35e6c5946 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/CommandLines.java +++ b/src/main/java/com/google/devtools/build/lib/actions/CommandLines.java @@ -180,19 +180,22 @@ ExpandedCommandLines expand( return new ExpandedCommandLines(arguments.build(), paramFiles); } - public void addToFingerprint(ActionKeyContext actionKeyContext, Fingerprint fingerprint) + public void addToFingerprint( + ActionKeyContext actionKeyContext, + @Nullable ArtifactExpander artifactExpander, + Fingerprint fingerprint) throws CommandLineExpansionException { // Optimize for simple case of single command line if (commandLines instanceof CommandLine) { CommandLine commandLine = (CommandLine) commandLines; - commandLine.addToFingerprint(actionKeyContext, fingerprint); + commandLine.addToFingerprint(actionKeyContext, artifactExpander, fingerprint); return; } List commandLines = getCommandLines(); for (CommandLineAndParamFileInfo pair : commandLines) { CommandLine commandLine = pair.commandLine; ParamFileInfo paramFileInfo = pair.paramFileInfo; - commandLine.addToFingerprint(actionKeyContext, fingerprint); + commandLine.addToFingerprint(actionKeyContext, artifactExpander, fingerprint); if (paramFileInfo != null) { addParamFileInfoToFingerprint(paramFileInfo, fingerprint); } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/CustomCommandLine.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/CustomCommandLine.java index c0f36de199e617..293fa65ace958b 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/actions/CustomCommandLine.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/CustomCommandLine.java @@ -1288,7 +1288,10 @@ private Object substituteTreeFileArtifactArgvFragment(Object arg) { @Override @SuppressWarnings("unchecked") - public void addToFingerprint(ActionKeyContext actionKeyContext, Fingerprint fingerprint) { + public void addToFingerprint( + ActionKeyContext actionKeyContext, + @Nullable ArtifactExpander artifactExpander, + Fingerprint fingerprint) { int count = arguments.size(); for (int i = 0; i < count; ) { Object arg = arguments.get(i++); diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/ParameterFileWriteAction.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/ParameterFileWriteAction.java index 226eaf4dfbc576..76a2319743261a 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/actions/ParameterFileWriteAction.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/ParameterFileWriteAction.java @@ -182,6 +182,6 @@ protected void computeKey( fp.addString(GUID); fp.addString(String.valueOf(makeExecutable)); fp.addString(type.toString()); - commandLine.addToFingerprint(actionKeyContext, fp); + commandLine.addToFingerprint(actionKeyContext, artifactExpander, fp); } } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java index 904be146070dfa..e426d7ae917788 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java @@ -441,7 +441,7 @@ protected void computeKey( Fingerprint fp) throws CommandLineExpansionException { fp.addString(GUID); - commandLines.addToFingerprint(actionKeyContext, fp); + commandLines.addToFingerprint(actionKeyContext, artifactExpander, fp); fp.addString(getMnemonic()); // We don't need the toolManifests here, because they are a subset of the inputManifests by // definition and the output of an action shouldn't change whether something is considered a diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkCustomCommandLine.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkCustomCommandLine.java index ba10d0e5cd0aad..1c37360c4fe4eb 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkCustomCommandLine.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkCustomCommandLine.java @@ -14,6 +14,7 @@ package com.google.devtools.build.lib.analysis.starlark; import com.google.common.base.Joiner; +import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Interner; @@ -189,12 +190,7 @@ private int eval( originalValues = arguments.subList(argi, argi + count); argi += count; } - List expandedValues = originalValues; - if (artifactExpander != null && (features & EXPAND_DIRECTORIES) != 0) { - if (hasDirectory(originalValues)) { - expandedValues = expandDirectories(artifactExpander, originalValues); - } - } + List expandedValues = maybeExpandDirectories(artifactExpander, originalValues); List stringValues; if (mapEach != null) { stringValues = new ArrayList<>(expandedValues.size()); @@ -278,6 +274,25 @@ private int eval( return argi; } + /** + * Expands the directories if {@code expand_directories} feature is enabled and a + * ArtifactExpander is available. + * + *

Technically, we should always expand the directories if the feature is requested, however + * we cannot do that in the absence of the {@link ArtifactExpander}. + */ + private List maybeExpandDirectories( + @Nullable ArtifactExpander artifactExpander, List originalValues) + throws CommandLineExpansionException { + if ((features & EXPAND_DIRECTORIES) == 0 + || artifactExpander == null + || !hasDirectory(originalValues)) { + return originalValues; + } + + return expandDirectories(artifactExpander, originalValues); + } + private static boolean hasDirectory(List originalValues) { int n = originalValues.size(); for (int i = 0; i < n; ++i) { @@ -336,7 +351,8 @@ private int addToFingerprint( int argi, ActionKeyContext actionKeyContext, Fingerprint fingerprint, - StarlarkSemantics starlarkSemantics) + StarlarkSemantics starlarkSemantics, + @Nullable ArtifactExpander artifactExpander) throws CommandLineExpansionException { final Location location = ((features & HAS_LOCATION) != 0) ? (Location) arguments.get(argi++) : null; @@ -345,36 +361,52 @@ private int addToFingerprint( if ((features & IS_NESTED_SET) != 0) { NestedSet values = (NestedSet) arguments.get(argi++); if (mapEach != null) { - CommandLineItem.MapFn commandLineItemMapFn = - new CommandLineItemMapEachAdaptor(mapEach, location, starlarkSemantics); + CommandLineItemMapEachAdaptor commandLineItemMapFn = + new CommandLineItemMapEachAdaptor( + mapEach, + location, + starlarkSemantics, + (features & EXPAND_DIRECTORIES) != 0 ? artifactExpander : null); try { actionKeyContext.addNestedSetToFingerprint(commandLineItemMapFn, fingerprint, values); } catch (UncheckedCommandLineExpansionException e) { // We wrap the CommandLineExpansionException below, unwrap here throw e.cause; + } finally { + // The cache holds an entry for a NestedSet for every (map_fn, hasArtifactExpanderBit). + // Clearing the artifactExpander itself saves us from storing the contents of it in the + // cache keys (it is no longer needed after we evaluate the value). + // NestedSet cache is cleared after every build, which means that the artifactExpander + // for a given action, if present, cannot change within the lifetime of the fingerprint + // cache (we call getKey with artifactExpander to check action key, when we are ready to + // execute the action in case of a cache miss). + commandLineItemMapFn.clearArtifactExpander(); } } else { actionKeyContext.addNestedSetToFingerprint(fingerprint, values); } } else { int count = (Integer) arguments.get(argi++); - final List originalValues = arguments.subList(argi, argi + count); + List maybeExpandedValues = + maybeExpandDirectories(artifactExpander, arguments.subList(argi, argi + count)); argi += count; if (mapEach != null) { - List stringValues = new ArrayList<>(count); + // TODO(b/160181927): If artifactExpander == null (which happens in the analysis phase) + // but expandDirectories is true, we run the map_each function on directory values without + // actually expanding them. This differs from the real evaluation behavior. This means + // that we can erroneously produce the same digest for two command lines that differ only + // in their directory expansion. Fortunately, this is only a problem for shared action + // conflict checking/aquery result, since at execution time we have an artifactExpander. applyMapEach( mapEach, - originalValues, - stringValues::add, + maybeExpandedValues, + fingerprint::addString, location, - /*artifactExpander=*/ null, + artifactExpander, starlarkSemantics); - for (String s : stringValues) { - fingerprint.addString(s); - } } else { - for (int i = 0; i < count; ++i) { - fingerprint.addString(CommandLineItem.expandToCommandLine(originalValues.get(i))); + for (Object value : maybeExpandedValues) { + fingerprint.addString(CommandLineItem.expandToCommandLine(value)); } } } @@ -659,7 +691,10 @@ public Iterable arguments(@Nullable ArtifactExpander artifactExpander) } @Override - public void addToFingerprint(ActionKeyContext actionKeyContext, Fingerprint fingerprint) + public void addToFingerprint( + ActionKeyContext actionKeyContext, + @Nullable ArtifactExpander artifactExpander, + Fingerprint fingerprint) throws CommandLineExpansionException { for (int argi = 0; argi < arguments.size(); ) { Object arg = arguments.get(argi++); @@ -667,7 +702,12 @@ public void addToFingerprint(ActionKeyContext actionKeyContext, Fingerprint fing argi = ((VectorArg) arg) .addToFingerprint( - arguments, argi, actionKeyContext, fingerprint, starlarkSemantics); + arguments, + argi, + actionKeyContext, + fingerprint, + starlarkSemantics, + artifactExpander); } else if (arg instanceof ScalarArg) { argi = ((ScalarArg) arg).addToFingerprint(arguments, argi, fingerprint); } else { @@ -676,7 +716,7 @@ public void addToFingerprint(ActionKeyContext actionKeyContext, Fingerprint fing } } - /** Used during action key evaluation when we don't have an artifact expander. * */ + /** Used during action key evaluation when we don't have an artifact expander. */ private static class NoopExpander implements DirectoryExpander { @Override public ImmutableList list(FileApi file) { @@ -769,23 +809,37 @@ private static class CommandLineItemMapEachAdaptor private final StarlarkCallable mapFn; private final Location location; private final StarlarkSemantics starlarkSemantics; + /** + * Indicates whether artifactExpander was provided on construction. This is used to distinguish + * the case where it's not provided from the case where it was provided but subsequently + * cleared. + */ + private final boolean hasArtifactExpander; + + @Nullable private ArtifactExpander artifactExpander; CommandLineItemMapEachAdaptor( - StarlarkCallable mapFn, Location location, StarlarkSemantics starlarkSemantics) { + StarlarkCallable mapFn, + Location location, + StarlarkSemantics starlarkSemantics, + @Nullable ArtifactExpander artifactExpander) { this.mapFn = mapFn; this.location = location; this.starlarkSemantics = starlarkSemantics; + this.hasArtifactExpander = artifactExpander != null; + this.artifactExpander = artifactExpander; } @Override public void expandToCommandLine(Object object, Consumer args) { + Preconditions.checkState(artifactExpander != null || !hasArtifactExpander); try { applyMapEach( mapFn, - ImmutableList.of(object), + maybeExpandDirectory(object), args, location, - /*artifactExpander=*/ null, + artifactExpander, starlarkSemantics); } catch (CommandLineExpansionException e) { // Rather than update CommandLineItem#expandToCommandLine and the numerous callers, @@ -794,6 +848,14 @@ public void expandToCommandLine(Object object, Consumer args) { } } + private List maybeExpandDirectory(Object object) throws CommandLineExpansionException { + if (artifactExpander == null || !VectorArg.isDirectory(object)) { + return ImmutableList.of(object); + } + + return VectorArg.expandDirectories(artifactExpander, ImmutableList.of(object)); + } + @Override public boolean equals(Object obj) { if (!(obj instanceof CommandLineItemMapEachAdaptor)) { @@ -803,13 +865,18 @@ public boolean equals(Object obj) { // Instance compare intentional // The normal implementation uses location + name of function, // which can conceivably conflict in tests - return mapFn == other.mapFn; + // We only compare presence of artifactExpander vs absence of it since the nestedset + // fingerprint cache is emptied after every build, therefore if the artifact expander is + // provided, it will be the same. + return mapFn == other.mapFn && hasArtifactExpander == other.hasArtifactExpander; } @Override public int hashCode() { - // identity hashcode intentional - return System.identityHashCode(mapFn); + // Force use of identityHashCode, in case the callable uses a custom hash function. (As of + // this writing, only providers seem to have a custom hashCode, and those shouldn't be used + // as map_each functions, but doesn't hurt to be safe...). + return 31 * System.identityHashCode(mapFn) + Boolean.hashCode(hasArtifactExpander); } @Override @@ -818,6 +885,18 @@ public int maxInstancesAllowed() { // always static return Integer.MAX_VALUE; } + + /** + * Clears the artifact expander in order not to prolong the lifetime of it unnecessarily. + * + *

Although this operation technically changes this object, it can be called after we add the + * object to a {@link HashSet}. Clearing the artifactExpander does not affect the result of + * {@link #equals} or {@link #hashCode}. Please note that once we call this function, we can no + * longer call {@link #expandToCommandLine}. + */ + void clearArtifactExpander() { + artifactExpander = null; + } } private static String errorMessage( diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileAction.java index 8bb9d1ad708f80..0ef18a65136cc6 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileAction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileAction.java @@ -190,8 +190,8 @@ protected void computeKey( throws CommandLineExpansionException { fp.addUUID(GUID); fp.addInt(classpathMode.ordinal()); - executableLine.addToFingerprint(actionKeyContext, fp); - flagLine.addToFingerprint(actionKeyContext, fp); + executableLine.addToFingerprint(actionKeyContext, artifactExpander, fp); + flagLine.addToFingerprint(actionKeyContext, artifactExpander, fp); // As the classpath is no longer part of commandLines implicitly, we need to explicitly add // the transitive inputs to the key here. actionKeyContext.addNestedSetToFingerprint(fp, transitiveInputs); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java index 32c0060b5150f8..f859b3d8be4ec6 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java @@ -41,6 +41,7 @@ import com.google.devtools.build.lib.actions.Actions; import com.google.devtools.build.lib.actions.AlreadyReportedActionExecutionException; import com.google.devtools.build.lib.actions.Artifact; +import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander; import com.google.devtools.build.lib.actions.ArtifactPathResolver; import com.google.devtools.build.lib.actions.DiscoveredInputsEvent; import com.google.devtools.build.lib.actions.FileArtifactValue; @@ -732,15 +733,12 @@ private ActionExecutionValue checkCacheAndExecuteIfNeeded( env.getListener(), state.discoveredInputs != null, action, actionLookupData)); } - ImmutableMap> expandedFilesets; - if (state.topLevelFilesets == null || state.topLevelFilesets.isEmpty()) { - expandedFilesets = state.filesetsInsideRunfiles; - } else { - Map> filesetsMap = - new HashMap<>(state.filesetsInsideRunfiles); - filesetsMap.putAll(state.topLevelFilesets); - expandedFilesets = ImmutableMap.copyOf(filesetsMap); - } + ImmutableMap> expandedFilesets = + state.getExpandedFilesets(); + + ArtifactExpander artifactExpander = + new Artifact.ArtifactExpanderImpl( + Collections.unmodifiableMap(state.expandedArtifacts), expandedFilesets); ArtifactPathResolver pathResolver = ArtifactPathResolver.createPathResolver( @@ -762,6 +760,7 @@ private ActionExecutionValue checkCacheAndExecuteIfNeeded( env.getListener(), action, metadataHandler, + artifactExpander, actionStartTime, state.allInputs.actionCacheInputs, clientEnv, @@ -842,7 +841,7 @@ private ActionExecutionValue checkCacheAndExecuteIfNeeded( metadataHandler, actionStartTime, actionLookupData, - Collections.unmodifiableMap(state.expandedArtifacts), + artifactExpander, expandedFilesets, state.topLevelFilesets, state.actionFileSystem, @@ -866,6 +865,11 @@ public void run( ActionMetadataHandler metadataHandler, Map clientEnv) throws InterruptedException, ActionExecutionException { + // TODO(b/160603797): For the sake of action key computation, we should not need + // state.filesetsInsideRunfiles. In fact, for the metadataHandler, we are guaranteed to not + // expand any filesets since we request metadata for input/output Artifacts only. + ImmutableMap> expandedFilesets = + state.getExpandedFilesets(); if (action.discoversInputs()) { state.discoveredInputs = action.getInputs(); switch (addDiscoveredInputs( @@ -887,7 +891,15 @@ public void run( } } Preconditions.checkState(!env.valuesMissing(), action); - skyframeActionExecutor.updateActionCache(action, metadataHandler, state.token, clientEnv); + skyframeActionExecutor.updateActionCache( + action, + metadataHandler, + new Artifact.ArtifactExpanderImpl( + // Skipping the filesets in runfiles since those cannot participate in command line + // creation. + Collections.unmodifiableMap(state.expandedArtifacts), expandedFilesets), + state.token, + clientEnv); } } @@ -1436,6 +1448,18 @@ Iterable filterKnownDiscoveredInputs() { discoveredInputs.toList(), input -> inputArtifactData.getMetadata(input) == null); } + ImmutableMap> getExpandedFilesets() { + if (topLevelFilesets == null || topLevelFilesets.isEmpty()) { + return filesetsInsideRunfiles; + } + + Map> filesetsMap = + Maps.newHashMapWithExpectedSize(filesetsInsideRunfiles.size() + topLevelFilesets.size()); + filesetsMap.putAll(filesetsInsideRunfiles); + filesetsMap.putAll(topLevelFilesets); + return ImmutableMap.copyOf(filesetsMap); + } + @Override public String toString() { return MoreObjects.toStringHelper(this) 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 d16e77355888fb..542a833bac78ae 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 @@ -52,7 +52,7 @@ import com.google.devtools.build.lib.actions.Actions; import com.google.devtools.build.lib.actions.AlreadyReportedActionExecutionException; import com.google.devtools.build.lib.actions.Artifact; -import com.google.devtools.build.lib.actions.Artifact.ArtifactExpanderImpl; +import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander; import com.google.devtools.build.lib.actions.Artifact.OwnerlessArtifactWrapper; import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact; import com.google.devtools.build.lib.actions.ArtifactPathResolver; @@ -111,7 +111,6 @@ import java.io.Closeable; import java.io.FileNotFoundException; import java.io.IOException; -import java.util.Collection; import java.util.Collections; import java.util.HashSet; import java.util.List; @@ -394,7 +393,7 @@ ActionExecutionValue executeAction( ActionMetadataHandler metadataHandler, long actionStartTime, ActionLookupData actionLookupData, - Map> expandedInputs, + ArtifactExpander artifactExpander, ImmutableMap> expandedFilesets, ImmutableMap> topLevelFilesets, @Nullable FileSystem actionFileSystem, @@ -416,8 +415,7 @@ ActionExecutionValue executeAction( env, action, metadataHandler, - expandedInputs, - expandedFilesets, + artifactExpander, topLevelFilesets, actionFileSystem, skyframeDepsResult); @@ -485,8 +483,7 @@ private ActionExecutionContext getContext( Environment env, Action action, MetadataHandler metadataHandler, - Map> expandedInputs, - ImmutableMap> expandedFilesets, + ArtifactExpander artifactExpander, ImmutableMap> topLevelFilesets, @Nullable FileSystem actionFileSystem, @Nullable Object skyframeDepsResult) { @@ -495,7 +492,7 @@ private ActionExecutionContext getContext( ArtifactPathResolver.createPathResolver(actionFileSystem, executorEngine.getExecRoot()); FileOutErr fileOutErr; if (replayActionOutErr) { - String actionKey = action.getKey(actionKeyContext, /*artifactExpander=*/ null); + String actionKey = action.getKey(actionKeyContext, artifactExpander); fileOutErr = actionLogBufferPathGenerator.persistent(actionKey, artifactPathResolver); try { fileOutErr.getErrorPath().delete(); @@ -520,7 +517,7 @@ private ActionExecutionContext getContext( : selectEventHandler(emitProgressEvents), clientEnv, topLevelFilesets, - new ArtifactExpanderImpl(expandedInputs, expandedFilesets), + artifactExpander, actionFileSystem, skyframeDepsResult, nestedSetExpander); @@ -552,6 +549,7 @@ Token checkActionCache( ExtendedEventHandler eventHandler, Action action, MetadataHandler metadataHandler, + ArtifactExpander artifactExpander, long actionStartTime, List resolvedCacheArtifacts, Map clientEnv, @@ -573,6 +571,7 @@ Token checkActionCache( ? reporter : null, metadataHandler, + artifactExpander, remoteDefaultProperties); } catch (UserExecException e) { throw e.toActionExecutionException(action); @@ -588,7 +587,7 @@ Token checkActionCache( if (replayActionOutErr) { // TODO(ulfjack): This assumes that the stdout/stderr files are unmodified. It would be // better to integrate them with the action cache and rerun the action when they change. - String actionKey = action.getKey(actionKeyContext, /*artifactExpander=*/ null); + String actionKey = action.getKey(actionKeyContext, artifactExpander); FileOutErr fileOutErr = actionLogBufferPathGenerator.persistent(actionKey, pathResolver); // Set the mightHaveOutput bit in FileOutErr. Otherwise hasRecordedOutput() doesn't check if // the file exists and just returns false. @@ -632,7 +631,11 @@ public T getContext(Class type) { } void updateActionCache( - Action action, MetadataHandler metadataHandler, Token token, Map clientEnv) + Action action, + MetadataHandler metadataHandler, + ArtifactExpander artifactExpander, + Token token, + Map clientEnv) throws ActionExecutionException { if (!actionCacheChecker.enabled()) { return; @@ -650,7 +653,7 @@ void updateActionCache( try { actionCacheChecker.updateActionCache( - action, token, metadataHandler, clientEnv, remoteDefaultProperties); + action, token, metadataHandler, artifactExpander, clientEnv, remoteDefaultProperties); } catch (IOException e) { // Skyframe has already done all the filesystem access needed for outputs and swallows // IOExceptions for inputs. So an IOException is impossible here. diff --git a/src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java b/src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java index d4f13e8e76ee09..91d2f9a7eb1c2f 100644 --- a/src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java +++ b/src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java @@ -118,10 +118,17 @@ private void runAction(Action action, Map clientEnv, Map digests = new HashMap<>(); for (CustomCommandLine commandLine : commandLines) { Fingerprint fingerprint = new Fingerprint(); - commandLine.addToFingerprint(actionKeyContext, fingerprint); + commandLine.addToFingerprint(actionKeyContext, /*artifactExpander=*/ null, fingerprint); String digest = fingerprint.hexDigestAndReset(); CustomCommandLine previous = digests.putIfAbsent(digest, commandLine); if (previous != null) { diff --git a/src/test/java/com/google/devtools/build/lib/skylark/BUILD b/src/test/java/com/google/devtools/build/lib/skylark/BUILD index 78aa925e432ef6..b5ceb255bca427 100644 --- a/src/test/java/com/google/devtools/build/lib/skylark/BUILD +++ b/src/test/java/com/google/devtools/build/lib/skylark/BUILD @@ -27,6 +27,7 @@ java_test( "//src/main/java/com/google/devtools/build/lib:runtime", "//src/main/java/com/google/devtools/build/lib:syntax", "//src/main/java/com/google/devtools/build/lib/actions", + "//src/main/java/com/google/devtools/build/lib/actions:action_lookup_key", "//src/main/java/com/google/devtools/build/lib/actions:artifacts", "//src/main/java/com/google/devtools/build/lib/analysis:actions/parameter_file_write_action", "//src/main/java/com/google/devtools/build/lib/analysis:actions/substitution", diff --git a/src/test/java/com/google/devtools/build/lib/skylark/StarlarkRuleImplementationFunctionsTest.java b/src/test/java/com/google/devtools/build/lib/skylark/StarlarkRuleImplementationFunctionsTest.java index ec3ff44e75eae9..ee96a1c538fb87 100644 --- a/src/test/java/com/google/devtools/build/lib/skylark/StarlarkRuleImplementationFunctionsTest.java +++ b/src/test/java/com/google/devtools/build/lib/skylark/StarlarkRuleImplementationFunctionsTest.java @@ -20,15 +20,18 @@ import static org.junit.Assert.fail; import com.google.common.base.Joiner; +import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Iterables; import com.google.common.collect.Lists; import com.google.devtools.build.lib.actions.Action; import com.google.devtools.build.lib.actions.ActionAnalysisMetadata; -import com.google.devtools.build.lib.actions.ActionKeyContext; +import com.google.devtools.build.lib.actions.ActionLookupKey; import com.google.devtools.build.lib.actions.Artifact; +import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander; import com.google.devtools.build.lib.actions.Artifact.ArtifactExpanderImpl; +import com.google.devtools.build.lib.actions.Artifact.DerivedArtifact; import com.google.devtools.build.lib.actions.CommandLine; import com.google.devtools.build.lib.actions.CommandLineExpansionException; import com.google.devtools.build.lib.actions.CompositeRunfilesSupplier; @@ -61,10 +64,10 @@ import com.google.devtools.build.lib.syntax.Starlark; import com.google.devtools.build.lib.syntax.StarlarkList; import com.google.devtools.build.lib.syntax.StarlarkThread; -import com.google.devtools.build.lib.syntax.util.EvaluationTestCase; import com.google.devtools.build.lib.testutil.MoreAsserts; import com.google.devtools.build.lib.util.Fingerprint; import com.google.devtools.build.lib.util.OsUtils; +import com.google.devtools.build.lib.vfs.PathFragment; import java.nio.charset.Charset; import java.nio.charset.StandardCharsets; import java.util.HashMap; @@ -87,7 +90,7 @@ @StarlarkGlobalLibrary // needed for CallUtils.getBuiltinCallable, sadly public class StarlarkRuleImplementationFunctionsTest extends BuildViewTestCase { - private final EvaluationTestCase ev = new BazelEvaluationTestCase(); + private final BazelEvaluationTestCase ev = new BazelEvaluationTestCase(); private StarlarkRuleContext createRuleContext(String label) throws Exception { return new StarlarkRuleContext( @@ -2864,12 +2867,9 @@ public void testStarlarkCustomCommandLineKeyComputation() throws Exception { "args.add_all(values, map_each=_map_each)")); // Ensure all these command lines have distinct keys - ActionKeyContext actionKeyContext = new ActionKeyContext(); Map digests = new HashMap<>(); for (CommandLine commandLine : commandLines.build()) { - Fingerprint fingerprint = new Fingerprint(); - commandLine.addToFingerprint(actionKeyContext, fingerprint); - String digest = fingerprint.hexDigestAndReset(); + String digest = getDigest(commandLine); CommandLine previous = digests.putIfAbsent(digest, commandLine); if (previous != null) { fail( @@ -2890,7 +2890,261 @@ public void testStarlarkCustomCommandLineKeyComputation() throws Exception { "args.add_all(values, map_each=_bad_fn)"); assertThrows( CommandLineExpansionException.class, - () -> commandLine.addToFingerprint(actionKeyContext, new Fingerprint())); + () -> + commandLine.addToFingerprint( + actionKeyContext, /*artifactExpander=*/ null, new Fingerprint())); + } + + @Test + public void skylarkCustomCommandLineKeyComputation_differentMapEach() throws Exception { + setRuleContext(createRuleContext("//foo:foo")); + + CommandLine commandLine1 = + getCommandLine( + "args = ruleContext.actions.args()", + "def _fun1(arg): return 'val1'", + "def _fun2(arg): return 'val2'", + "args.add_all(['a'], map_each=_fun1)"); + CommandLine commandLine2 = + getCommandLine( + "args = ruleContext.actions.args()", + "def _fun1(arg): return 'val1'", + "def _fun2(arg): return 'val2'", + "args.add_all(['a'], map_each=_fun2)"); + + assertThat(getDigest(commandLine1)).isNotEqualTo(getDigest(commandLine2)); + } + + @Test + public void skylarkCustomCommandLineKeyComputation_differentArg() throws Exception { + setRuleContext(createRuleContext("//foo:foo")); + + CommandLine commandLine1 = + getCommandLine( + "args = ruleContext.actions.args()", + "def _fun(arg): return arg", + "args.add_all(['a'], map_each=_fun)"); + CommandLine commandLine2 = + getCommandLine( + "args = ruleContext.actions.args()", + "def _fun(arg): return arg", + "args.add_all(['b'], map_each=_fun)"); + + assertThat(getDigest(commandLine1)).isNotEqualTo(getDigest(commandLine2)); + } + + @Test + public void skylarkCustomCommandLineKeyComputationWithExpander_equivalentMapEach_sameKey() + throws Exception { + setRuleContext(createRuleContext("//foo:foo")); + + CommandLine commandLine1 = + getCommandLine( + "args = ruleContext.actions.args()", + "directory = ruleContext.actions.declare_directory('dir')", + "args.add_joined([directory], join_with=',', map_each=str, expand_directories=True)"); + CommandLine commandLine2 = + getCommandLine( + "args = ruleContext.actions.args()", + "directory = ruleContext.actions.declare_directory('dir')", + "def mystr(file): return str(file)", + "args.add_joined([directory], join_with=',', map_each=mystr, expand_directories=True)"); + + ArtifactExpander expander = createArtifactExpander("foo/dir", "file"); + assertThat(getDigest(commandLine1, expander)).isEqualTo(getDigest(commandLine2, expander)); + } + + @Test + public void skylarkCustomCommandLineKeyComputationWithExpander_mapEachConstantForDir() + throws Exception { + setRuleContext(createRuleContext("//foo:foo")); + + CommandLine commandLine1 = + getCommandLine( + "args = ruleContext.actions.args()", + "directory = ruleContext.actions.declare_directory('dir')", + "def _constant_for_dir(f): return 'constant' if f.path.endswith('dir') else 'value1'", + "args.add_all([directory], map_each=_constant_for_dir, expand_directories=True)"); + CommandLine commandLine2 = + getCommandLine( + "args = ruleContext.actions.args()", + "directory = ruleContext.actions.declare_directory('dir')", + "def _constant_for_dir(f): return 'constant' if f.path.endswith('dir') else 'value2'", + "args.add_all([directory], map_each=_constant_for_dir, expand_directories=True)"); + + ArtifactExpander expander = createArtifactExpander("foo/dir", "file"); + assertThat(getDigest(commandLine1, expander)).isNotEqualTo(getDigest(commandLine2, expander)); + } + + @Test + public void skylarkCustomCommandLineKeyComputationWithExpander_constantForDirWithNestedSet() + throws Exception { + setRuleContext(createRuleContext("//foo:foo")); + + CommandLine commandLine1 = + getCommandLine( + "args = ruleContext.actions.args()", + "dir = ruleContext.actions.declare_directory('dir')", + "def _constant_for_dir(f): return 'constant' if f.path.endswith('dir') else 'value1'", + "args.add_all(depset([dir]), map_each=_constant_for_dir, expand_directories=True)"); + CommandLine commandLine2 = + getCommandLine( + "args = ruleContext.actions.args()", + "dir = ruleContext.actions.declare_directory('dir')", + "def _constant_for_dir(f): return 'constant' if f.path.endswith('dir') else 'value2'", + "args.add_all(depset([dir]), map_each=_constant_for_dir, expand_directories=True)"); + + ArtifactExpander expander = createArtifactExpander("foo/dir", "file"); + assertThat(getDigest(commandLine1, expander)).isNotEqualTo(getDigest(commandLine2, expander)); + } + + @Test + public void skylarkCustomCommandLineKeyComputationWithExpander_mapEachFailsForDir() + throws Exception { + setRuleContext(createRuleContext("//foo:foo")); + + CommandLine commandLine1 = + getCommandLine( + "args = ruleContext.actions.args()", + "directory = ruleContext.actions.declare_directory('dir')", + "ruleContext.actions.run_shell(outputs=[directory], command='')", + "def _fail_for_dir(file):", + " if file.path.endswith('dir'): fail('hello')", + " return 'value1'", + "args.add_all([directory], map_each=_fail_for_dir, expand_directories=True)"); + CommandLine commandLine2 = + getCommandLine( + "args = ruleContext.actions.args()", + "ruleContext.actions.run_shell(outputs=[directory], command='')", + "directory = ruleContext.actions.declare_directory('dir')", + "def _fail_for_dir(file):", + " if file.path.endswith('dir'): fail('hello')", + " return 'value2'", + "args.add_all([directory], map_each=_fail_for_dir, expand_directories=True)"); + + ArtifactExpander expander = createArtifactExpander("foo/dir", "file"); + assertThat(getDigest(commandLine1, expander)).isNotEqualTo(getDigest(commandLine2, expander)); + } + + @Test + public void skylarkCustomCommandLineKeyComputationWithExpander_differentExpansion() + throws Exception { + setRuleContext(createRuleContext("//foo:foo")); + CommandLine commandLine = + getCommandLine( + "args = ruleContext.actions.args()", + "directory = ruleContext.actions.declare_directory('dir')", + "ruleContext.actions.run_shell(outputs=[directory], command='')", + "def _get_path(file): return file.path", + "args.add_all([directory], map_each=_get_path, expand_directories=True)"); + + ArtifactExpander expander1 = createArtifactExpander("foo/dir", "file1"); + ArtifactExpander expander2 = createArtifactExpander("foo/dir", "file2"); + assertThat(getDigest(commandLine, expander1)).isNotEqualTo(getDigest(commandLine, expander2)); + } + + @Test + public void skylarkCustomCommandLineKeyComputationWithExpander_differentExpansionNoMapEach() + throws Exception { + setRuleContext(createRuleContext("//foo:foo")); + CommandLine commandLine = + getCommandLine( + "args = ruleContext.actions.args()", + "directory = ruleContext.actions.declare_directory('dir')", + "args.add_all([directory])"); + + ArtifactExpander expander1 = createArtifactExpander("foo/dir", "file1"); + ArtifactExpander expander2 = createArtifactExpander("foo/dir", "file2"); + assertThat(getDigest(commandLine, expander1)).isNotEqualTo(getDigest(commandLine, expander2)); + } + + @Test + public void skylarkCustomCommandLineKeyComputationWithExpander_extraFileInExpansionNoMapEach() + throws Exception { + setRuleContext(createRuleContext("//foo:foo")); + CommandLine commandLine = + getCommandLine( + "args = ruleContext.actions.args()", + "directory = ruleContext.actions.declare_directory('dir')", + "args.add_all([directory])"); + + ArtifactExpander expander1 = createArtifactExpander("foo/dir", "file1"); + ArtifactExpander expander2 = createArtifactExpander("foo/dir", "file1", "file2"); + assertThat(getDigest(commandLine, expander1)).isNotEqualTo(getDigest(commandLine, expander2)); + } + + @Test + public void skylarkCustomCommandLineKeyComputationWithExpander_constantForDirAddJoined() + throws Exception { + setRuleContext(createRuleContext("//foo:foo")); + + CommandLine commandLine1 = + getCommandLine( + "args = ruleContext.actions.args()", + "directory = ruleContext.actions.declare_directory('dir')", + "def _constant_for_dir(f): return 'constant' if f.path.endswith('dir') else 'value1'", + "args.add_joined([directory], join_with=',', map_each=_constant_for_dir," + + " expand_directories=True)"); + CommandLine commandLine2 = + getCommandLine( + "args = ruleContext.actions.args()", + "directory = ruleContext.actions.declare_directory('dir')", + "def _constant_for_dir(f): return 'constant' if f.path.endswith('dir') else 'value2'", + "args.add_joined([directory], join_with=',', map_each=_constant_for_dir," + + " expand_directories=True)"); + + ArtifactExpander expander = createArtifactExpander("foo/dir", "file"); + assertThat(getDigest(commandLine1, expander)).isNotEqualTo(getDigest(commandLine2, expander)); + } + + @Test + public void skylarkCustomCommandLineKeyComputation_inconsequentialChangeToStarlarkSemantics() + throws Exception { + setRuleContext(createRuleContext("//foo:foo")); + CommandLine commandLine1 = + getCommandLine( + "args = ruleContext.actions.args()", + "directory = ruleContext.actions.declare_directory('dir')", + "def _path(f): return f.path", + "args.add_all([directory], map_each=_path)"); + + ev.setSemantics("--incompatible_run_shell_command_string"); + // setStarlarkSemanticsOptions reinitializes the thread -- set the ruleContext on the new one. + setRuleContext(createRuleContext("//foo:foo")); + + CommandLine commandLine2 = + getCommandLine( + "args = ruleContext.actions.args()", + "directory = ruleContext.actions.declare_directory('dir')", + "def _path(f): return f.path", + "args.add_all([directory], map_each=_path)"); + + assertThat(getDigest(commandLine1)).isEqualTo(getDigest(commandLine2)); + } + + private static ArtifactExpander createArtifactExpander(String dirRelativePath, String... files) { + return (artifact, output) -> { + Preconditions.checkArgument( + artifact.getRootRelativePath().equals(PathFragment.create(dirRelativePath))); + for (String file : files) { + output.add( + new DerivedArtifact( + artifact.getRoot(), + artifact.getExecPath().getRelative(file), + (ActionLookupKey) artifact.getArtifactOwner())); + } + }; + } + + private String getDigest(CommandLine commandLine) throws CommandLineExpansionException { + return getDigest(commandLine, /*artifactExpander=*/ null); + } + + private String getDigest(CommandLine commandLine, ArtifactExpander artifactExpander) + throws CommandLineExpansionException { + Fingerprint fingerprint = new Fingerprint(); + commandLine.addToFingerprint(actionKeyContext, artifactExpander, fingerprint); + return fingerprint.hexDigestAndReset(); } private CommandLine getCommandLine(String... lines) throws Exception {