From d8a5753a8ce1d9cb6604c325836683f3325e42eb Mon Sep 17 00:00:00 2001 From: janakr Date: Tue, 10 Oct 2017 08:31:32 +0200 Subject: In PerActionFileCache, tolerate requests for Artifacts that are not in the cache if we're doing input discovery: input discovery may find new artifacts we don't yet know about. Similarly, in SingleBuildFileCache, use the Artifact's path if it's available, rather than the poor man's route of the execRoot. PiperOrigin-RevId: 171635892 --- .../build/lib/exec/SingleBuildFileCache.java | 69 ++++++++++++---------- .../lib/skyframe/ActionExecutionFunction.java | 10 +++- .../build/lib/skyframe/PerActionFileCache.java | 11 +++- 3 files changed, 53 insertions(+), 37 deletions(-) (limited to 'src/main/java/com/google') diff --git a/src/main/java/com/google/devtools/build/lib/exec/SingleBuildFileCache.java b/src/main/java/com/google/devtools/build/lib/exec/SingleBuildFileCache.java index a098a8ce8f..c393332fe3 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/SingleBuildFileCache.java +++ b/src/main/java/com/google/devtools/build/lib/exec/SingleBuildFileCache.java @@ -22,6 +22,7 @@ import com.google.common.collect.Maps; import com.google.common.io.BaseEncoding; import com.google.devtools.build.lib.actions.ActionInput; import com.google.devtools.build.lib.actions.ActionInputFileCache; +import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.DigestOfDirectoryException; import com.google.devtools.build.lib.actions.cache.Metadata; import com.google.devtools.build.lib.skyframe.FileArtifactValue; @@ -52,38 +53,42 @@ public class SingleBuildFileCache implements ActionInputFileCache { // first. Further we won't need to unwrap the exception in getDigest(). private final LoadingCache pathToMetadata = CacheBuilder.newBuilder() - // We default to 10 disk read threads, but we don't expect them all to edit the map - // simultaneously. - .concurrencyLevel(8) - // Even small-ish builds, as of 11/21/2011 typically have over 10k artifacts, so it's - // unlikely that this default will adversely affect memory in most cases. - .initialCapacity(10000) - .build(new CacheLoader() { - @Override - public ActionInputMetadata load(ActionInput input) { - Path path = null; - try { - path = execRoot.getRelative(input.getExecPath()); - byte[] digest = path.getDigest(); - BaseEncoding hex = BaseEncoding.base16().lowerCase(); - ByteString hexDigest = ByteString.copyFrom(hex.encode(digest).getBytes(US_ASCII)); - // Inject reverse mapping. Doing this unconditionally in getDigest() showed up - // as a hotspot in CPU profiling. - digestToPath.put(hexDigest, input); - return new ActionInputMetadata(digest, path.getFileSize()); - } catch (IOException e) { - if (path != null && path.isDirectory()) { - // TODO(bazel-team): This is rather presumptuous- it could have been another type of - // IOException. - return new ActionInputMetadata( - new DigestOfDirectoryException( - "Input is a directory: " + input.getExecPathString())); - } else { - return new ActionInputMetadata(e); - } - } - } - }); + // We default to 10 disk read threads, but we don't expect them all to edit the map + // simultaneously. + .concurrencyLevel(8) + // Even small-ish builds, as of 11/21/2011 typically have over 10k artifacts, so it's + // unlikely that this default will adversely affect memory in most cases. + .initialCapacity(10000) + .build( + new CacheLoader() { + @Override + public ActionInputMetadata load(ActionInput input) { + Path path = + (input instanceof Artifact) + ? ((Artifact) input).getPath() + : execRoot.getRelative(input.getExecPath()); + try { + byte[] digest = path.getDigest(); + BaseEncoding hex = BaseEncoding.base16().lowerCase(); + ByteString hexDigest = + ByteString.copyFrom(hex.encode(digest).getBytes(US_ASCII)); + // Inject reverse mapping. Doing this unconditionally in getDigest() showed up + // as a hotspot in CPU profiling. + digestToPath.put(hexDigest, input); + return new ActionInputMetadata(digest, path.getFileSize()); + } catch (IOException e) { + if (path.isDirectory()) { + // TODO(bazel-team): This is rather presumptuous- it could have been another + // type of IOException. + return new ActionInputMetadata( + new DigestOfDirectoryException( + "Input is a directory: " + input.getExecPathString())); + } else { + return new ActionInputMetadata(e); + } + } + } + }); private final Map digestToPath = Maps.newConcurrentMap(); 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 a92e809800..46238c4e8b 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 @@ -388,7 +388,9 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver metadataHandler.discardOutputMetadata(); // This may be recreated if we discover inputs. - PerActionFileCache perActionFileCache = new PerActionFileCache(state.inputArtifactData); + PerActionFileCache perActionFileCache = + new PerActionFileCache( + state.inputArtifactData, /*missingArtifactsAllowed=*/ action.discoversInputs()); if (action.discoversInputs()) { if (state.discoveredInputs == null) { try { @@ -406,7 +408,8 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver if (env.valuesMissing()) { return null; } - perActionFileCache = new PerActionFileCache(state.inputArtifactData); + perActionFileCache = + new PerActionFileCache(state.inputArtifactData, /*missingArtifactsAllowed=*/ false); // Stage 1 finished, let's do stage 2. The stage 1 of input discovery will have added some // files with addDiscoveredInputs() and then have waited for those files to be available @@ -422,7 +425,8 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver if (env.valuesMissing()) { return null; } - perActionFileCache = new PerActionFileCache(state.inputArtifactData); + perActionFileCache = + new PerActionFileCache(state.inputArtifactData, /*missingArtifactsAllowed=*/ false); } metadataHandler = new ActionMetadataHandler(state.inputArtifactData, action.getOutputs(), tsgm.get()); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PerActionFileCache.java b/src/main/java/com/google/devtools/build/lib/skyframe/PerActionFileCache.java index 619e0b302b..1511c4c385 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/PerActionFileCache.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/PerActionFileCache.java @@ -35,14 +35,19 @@ import javax.annotation.Nullable; */ class PerActionFileCache implements ActionInputFileCache { private final Map inputArtifactData; + private final boolean missingArtifactsAllowed; // null until first call to getInputFromDigest() private volatile HashMap reverseMap; /** * @param inputArtifactData Map from artifact to metadata, used to return metadata upon request. + * @param missingArtifactsAllowed whether to tolerate missing artifacts: can happen during input + * discovery. */ - PerActionFileCache(Map inputArtifactData) { + PerActionFileCache( + Map inputArtifactData, boolean missingArtifactsAllowed) { this.inputArtifactData = Preconditions.checkNotNull(inputArtifactData); + this.missingArtifactsAllowed = missingArtifactsAllowed; } @Nullable @@ -51,7 +56,9 @@ class PerActionFileCache implements ActionInputFileCache { if (!(input instanceof Artifact)) { return null; } - return Preconditions.checkNotNull(inputArtifactData.get(input), input); + Metadata result = inputArtifactData.get(input); + Preconditions.checkState(missingArtifactsAllowed || result != null, "null for %s", input); + return result; } @Override -- cgit v1.2.3