diff options
author | 2016-07-20 19:06:20 +0000 | |
---|---|---|
committer | 2016-07-20 19:54:37 +0000 | |
commit | 2e1dbd79ae73753a7a14ddc991fa0b374bbf8605 (patch) | |
tree | c48764f6a07324664b67473ce376f221347fa3bd /src/main/java/com/google | |
parent | ebf601d340c570d2666a1293a144975938a063f2 (diff) |
Use byte[] rather than ByteString for file digests.
ActionInputFileCache:
Change getDigest() to return the underlying byte[16] owned by each
FileArtifactValue.
Remove throws clause from getInputFromDigest(); this should be an
in-memory operation, and no implementation actually throws.
PerActionFileCache:
Invert mapping from artifact to digest only if needed. Remove interner,
as it was used only for the reverse map keys, not the returned values.
This should be a significant cpu savings as eagerly constructing the
reverse maps was a noticeable hotspot.
--
MOS_MIGRATED_REVID=127972359
Diffstat (limited to 'src/main/java/com/google')
7 files changed, 62 insertions, 43 deletions
diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionInputFileCache.java b/src/main/java/com/google/devtools/build/lib/actions/ActionInputFileCache.java index f37cc09384..decfc8d7bf 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/ActionInputFileCache.java +++ b/src/main/java/com/google/devtools/build/lib/actions/ActionInputFileCache.java @@ -34,6 +34,8 @@ public interface ActionInputFileCache { * then t >= p. Aside from these properties, t can be any value and may vary arbitrarily across * calls. * + * The return value is owned by the cache and must not be modified. + * * @param input the input to retrieve the digest for * @return the artifact's digest or null if digest cannot be obtained (due to artifact * non-existence, lookup errors, or any other reason) @@ -43,7 +45,7 @@ public interface ActionInputFileCache { * */ @Nullable - ByteString getDigest(ActionInput input) throws IOException; + byte[] getDigest(ActionInput input) throws IOException; /** * Retrieves whether or not the input Artifact is a file or symlink to an existing file. @@ -68,7 +70,7 @@ public interface ActionInputFileCache { * Checks if the file is available locally, based on the assumption that previous operations on * the ActionInputFileCache would have created a cache entry for it. * - * @param digest the digest to lookup. + * @param digest the digest to lookup (as lowercase hex). * @return true if the specified digest is backed by a locally-readable file, false otherwise */ boolean contentsAvailableLocally(ByteString digest); @@ -77,11 +79,11 @@ public interface ActionInputFileCache { * Concrete subclasses must implement this to provide a mapping from digest to file path, * based on files previously seen as inputs. * - * @param digest the digest. + * @param digest the digest (as lowercase hex). * @return an ActionInput corresponding to the given digest. */ @Nullable - ActionInput getInputFromDigest(ByteString digest) throws IOException; + ActionInput getInputFromDigest(ByteString digest); /** * The absolute path that this input is located at. The usual {@link ActionInput} implementation 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 03725d6054..48e6f95e17 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 @@ -70,13 +70,12 @@ public class SingleBuildFileCache implements ActionInputFileCache { Path path = null; try { path = fs.getPath(fullPath(input)); + byte[] digest = path.getMD5Digest(); BaseEncoding hex = BaseEncoding.base16().lowerCase(); - ByteString digest = ByteString.copyFrom( - hex.encode(path.getMD5Digest()) - .getBytes(US_ASCII)); + 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(digest, input); + digestToPath.put(hexDigest, input); return new ActionInputMetadata(digest, path.getFileSize()); } catch (IOException e) { if (path != null && path.isDirectory()) { @@ -110,7 +109,7 @@ public class SingleBuildFileCache implements ActionInputFileCache { } @Override - public ByteString getDigest(ActionInput input) throws IOException { + public byte[] getDigest(ActionInput input) throws IOException { return pathToMetadata.getUnchecked(input).getDigest(); } @@ -137,12 +136,12 @@ public class SingleBuildFileCache implements ActionInputFileCache { /** Container class for caching I/O around ActionInputs. */ private static class ActionInputMetadata { - private final ByteString digest; + private final byte[] digest; private final long size; private final IOException exceptionOnAccess; /** Constructor for a successful lookup. */ - ActionInputMetadata(ByteString digest, long size) { + ActionInputMetadata(byte[] digest, long size) { this.digest = digest; this.size = size; this.exceptionOnAccess = null; @@ -156,7 +155,7 @@ public class SingleBuildFileCache implements ActionInputFileCache { } /** Returns digest or throws the exception encountered calculating it/ */ - ByteString getDigest() throws IOException { + byte[] getDigest() throws IOException { maybeRaiseException(); return digest; } diff --git a/src/main/java/com/google/devtools/build/lib/remote/MemcacheActionCache.java b/src/main/java/com/google/devtools/build/lib/remote/MemcacheActionCache.java index e821f1289f..8d29bea68a 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/MemcacheActionCache.java +++ b/src/main/java/com/google/devtools/build/lib/remote/MemcacheActionCache.java @@ -66,8 +66,7 @@ public final class MemcacheActionCache implements RemoteActionCache { @Override public String putFileIfNotExist(ActionInputFileCache cache, ActionInput file) throws IOException { - // PerActionFileCache already converted this to a lowercase ascii string.. it's not consistent! - String contentKey = new String(cache.getDigest(file).toByteArray()); + String contentKey = HashCode.fromBytes(cache.getDigest(file)).toString(); if (containsFile(contentKey)) { return contentKey; } diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnStrategy.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnStrategy.java index 7184f79dfd..17b821af79 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnStrategy.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnStrategy.java @@ -118,7 +118,7 @@ final class RemoteSpawnStrategy implements SpawnActionContext { // changes. It might not be sufficient to identify the input file globally in the // remote action cache. Consider upgrading this to a better hash algorithm with // less collision. - hasher.putBytes(inputFileCache.getDigest(input).toByteArray()); + hasher.putBytes(inputFileCache.getDigest(input)); } catch (IOException e) { throw new UserExecException("Failed to get digest for input.", e); } 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 270090478d..c5576d9dfb 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 @@ -13,8 +13,6 @@ // limitations under the License. package com.google.devtools.build.lib.skyframe; -import com.google.common.collect.Interner; -import com.google.common.collect.Interners; import com.google.common.io.BaseEncoding; import com.google.devtools.build.lib.actions.ActionInput; import com.google.devtools.build.lib.actions.ActionInputFileCache; @@ -25,8 +23,8 @@ import com.google.protobuf.ByteString; import java.io.IOException; import java.nio.charset.StandardCharsets; +import java.util.HashMap; import java.util.Map; -import java.util.concurrent.ConcurrentHashMap; import javax.annotation.Nullable; @@ -39,10 +37,8 @@ import javax.annotation.Nullable; */ class PerActionFileCache implements ActionInputFileCache { private final Map<Artifact, FileArtifactValue> inputArtifactData; - // Populated lazily, on calls to #getDigest. - private final Map<ByteString, Artifact> reverseMap = new ConcurrentHashMap<>(); - - private static final Interner<ByteString> BYTE_INTERNER = Interners.newWeakInterner(); + // null until first call to getInputFromDigest() + private volatile HashMap<ByteString, Artifact> reverseMap; /** * @param inputArtifactData Map from artifact to metadata, used to return metadata upon request. @@ -74,12 +70,6 @@ class PerActionFileCache implements ActionInputFileCache { return -1; } - @Nullable - @Override - public Artifact getInputFromDigest(ByteString digest) throws IOException { - return reverseMap.get(digest); - } - @Override public Path getInputPath(ActionInput input) { return ((Artifact) input).getPath(); @@ -87,16 +77,10 @@ class PerActionFileCache implements ActionInputFileCache { @Nullable @Override - public ByteString getDigest(ActionInput input) throws IOException { + public byte[] getDigest(ActionInput input) throws IOException { FileArtifactValue value = getInputFileArtifactValue(input); if (value != null) { - byte[] bytes = value.getDigest(); - if (bytes != null) { - ByteString digest = ByteString.copyFrom(BaseEncoding.base16().lowerCase().encode(bytes) - .getBytes(StandardCharsets.US_ASCII)); - reverseMap.put(BYTE_INTERNER.intern(digest), (Artifact) input); - return digest; - } + return value.getDigest(); } return null; } @@ -109,6 +93,38 @@ class PerActionFileCache implements ActionInputFileCache { @Override public boolean contentsAvailableLocally(ByteString digest) { - return reverseMap.containsKey(digest); + return getInputFromDigest(digest) != null; + } + + @Nullable + @Override + public Artifact getInputFromDigest(ByteString digest) { + HashMap<ByteString, Artifact> r = reverseMap; // volatile load + if (r == null) { + r = buildReverseMap(); + } + return r.get(digest); + } + + private synchronized HashMap<ByteString, Artifact> buildReverseMap() { + HashMap<ByteString, Artifact> r = reverseMap; // volatile load + if (r != null) { + return r; + } + r = new HashMap<>(inputArtifactData.size()); + // It would be nice to have a view of the entries which treats them as a map keyed on digest but + // does not require constructing another wrapper object for each entry. Java doesn't come with + // any collections which can do this, but cloning RegularImmutableSet and adding the necessary + // features wouldn't be too bad. + for (Map.Entry<Artifact, FileArtifactValue> e : inputArtifactData.entrySet()) { + byte[] bytes = e.getValue().getDigest(); + if (bytes != null) { + ByteString digest = ByteString.copyFrom(BaseEncoding.base16().lowerCase().encode(bytes) + .getBytes(StandardCharsets.US_ASCII)); + r.put(digest, e.getKey()); + } + } + reverseMap = r; // volatile store + return r; } } 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 a9cf5833f1..b5c0863d98 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 @@ -1105,8 +1105,8 @@ public final class SkyframeActionExecutor implements ActionExecutionContextFacto } @Override - public ByteString getDigest(ActionInput actionInput) throws IOException { - ByteString digest = perActionCache.getDigest(actionInput); + public byte[] getDigest(ActionInput actionInput) throws IOException { + byte[] digest = perActionCache.getDigest(actionInput); return digest != null ? digest : perBuildFileCache.getDigest(actionInput); } @@ -1130,7 +1130,7 @@ public final class SkyframeActionExecutor implements ActionExecutionContextFacto @Nullable @Override - public ActionInput getInputFromDigest(ByteString digest) throws IOException { + public ActionInput getInputFromDigest(ByteString digest) { ActionInput file = perActionCache.getInputFromDigest(digest); return file != null ? file : perBuildFileCache.getInputFromDigest(digest); } diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnStrategy.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnStrategy.java index b84e96991e..53b68f4744 100644 --- a/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnStrategy.java +++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnStrategy.java @@ -152,9 +152,12 @@ public final class WorkerSpawnStrategy implements SpawnActionContext { spawn.getInputFiles(), actionExecutionContext.getArtifactExpander()); for (ActionInput input : inputs) { - ByteString digest = inputFileCache.getDigest(input); - if (digest == null) { + byte[] digestBytes = inputFileCache.getDigest(input); + ByteString digest; + if (digestBytes == null) { digest = ByteString.EMPTY; + } else { + digest = ByteString.copyFromUtf8(HashCode.fromBytes(digestBytes).toString()); } requestBuilder @@ -208,7 +211,7 @@ public final class WorkerSpawnStrategy implements SpawnActionContext { Hasher hasher = Hashing.sha256().newHasher(); for (ActionInput tool : toolFiles) { hasher.putString(tool.getExecPathString(), Charset.defaultCharset()); - hasher.putBytes(actionInputFileCache.getDigest(tool).toByteArray()); + hasher.putBytes(actionInputFileCache.getDigest(tool)); } return hasher.hash(); } |