Skip to content

Commit

Permalink
Audit all production calls of Path#getDigest and either replace them …
Browse files Browse the repository at this point in the history
…with DigestUtils#getDigestWithManualFallback or document why Path#getFastDigest is not useful to them.

This is inspired by cleaning up in preparation for #11662: currently, some FileSystem implementations call Path#getFastDigest inside Path#getDigest because they have a fast digest implementation and want to guard against expensive digest computations.

After this change, ~all overrides of FileSystem#getDigest can be removed, since the callers will already have called FileSystem#getFastDigest.

PiperOrigin-RevId: 331142210
  • Loading branch information
janakdr authored and copybara-github committed Sep 11, 2020
1 parent 4893483 commit 95704d1
Show file tree
Hide file tree
Showing 20 changed files with 151 additions and 114 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
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.MetadataDigestUtils;
import com.google.devtools.build.lib.actions.cache.MetadataHandler;
import com.google.devtools.build.lib.actions.cache.Protos.ActionCacheStatistics.MissReason;
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
Expand Down Expand Up @@ -159,7 +159,7 @@ private static boolean validateArtifacts(
for (Artifact artifact : actionInputs.toList()) {
mdMap.put(artifact.getExecPathString(), getMetadataMaybe(metadataHandler, artifact));
}
return !Arrays.equals(DigestUtils.fromMetadata(mdMap), entry.getFileDigest());
return !Arrays.equals(MetadataDigestUtils.fromMetadata(mdMap), entry.getFileDigest());
}

private void reportCommand(EventHandler handler, Action action) {
Expand Down Expand Up @@ -347,7 +347,8 @@ private boolean mustExecute(
}
Map<String, String> usedEnvironment =
computeUsedEnv(action, clientEnv, remoteDefaultPlatformProperties);
if (!Arrays.equals(entry.getUsedClientEnvDigest(), DigestUtils.fromEnv(usedEnvironment))) {
if (!Arrays.equals(
entry.getUsedClientEnvDigest(), MetadataDigestUtils.fromEnv(usedEnvironment))) {
reportClientEnv(handler, action, usedEnvironment);
actionCache.accountMiss(MissReason.DIFFERENT_ENVIRONMENT);
return true;
Expand Down
6 changes: 2 additions & 4 deletions src/main/java/com/google/devtools/build/lib/actions/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ java_library(
"LocalHostResourceFallback.java",
"MiddlemanType.java",
"ResourceSet.java",
"cache/DigestUtils.java",
"cache/MetadataDigestUtils.java",
],
),
deps = [
Expand Down Expand Up @@ -213,14 +213,12 @@ java_library(
"FileStateValue.java",
"FileValue.java",
"InconsistentFilesystemException.java",
"cache/DigestUtils.java",
"cache/MetadataDigestUtils.java",
],
deps = [
":artifacts",
":has_digest",
"//src/main/java/com/google/devtools/build/lib/clock",
"//src/main/java/com/google/devtools/build/lib/concurrent",
"//src/main/java/com/google/devtools/build/lib/profiler",
"//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec",
"//src/main/java/com/google/devtools/build/lib/util",
"//src/main/java/com/google/devtools/build/lib/util:var_int",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,12 @@
import com.google.common.base.Preconditions;
import com.google.common.hash.HashFunction;
import com.google.common.io.BaseEncoding;
import com.google.devtools.build.lib.actions.cache.DigestUtils;
import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
import com.google.devtools.build.lib.util.BigIntegerFingerprint;
import com.google.devtools.build.lib.util.Fingerprint;
import com.google.devtools.build.lib.vfs.DigestUtils;
import com.google.devtools.build.lib.vfs.FileStatus;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ final class Entry {

public Entry(String key, Map<String, String> usedClientEnv, boolean discoversInputs) {
actionKey = key;
this.usedClientEnvDigest = DigestUtils.fromEnv(usedClientEnv);
this.usedClientEnvDigest = MetadataDigestUtils.fromEnv(usedClientEnv);
files = discoversInputs ? new ArrayList<String>() : null;
mdMap = new HashMap<>();
}
Expand Down Expand Up @@ -141,7 +141,7 @@ public byte[] getUsedClientEnvDigest() {
*/
public byte[] getFileDigest() {
if (digest == null) {
digest = DigestUtils.fromMetadata(mdMap);
digest = MetadataDigestUtils.fromMetadata(mdMap);
mdMap = null;
}
return digest;
Expand Down Expand Up @@ -182,7 +182,9 @@ public String toString() {
.append("\n");
builder.append(" digestKey = ");
if (digest == null) {
builder.append(formatDigest(DigestUtils.fromMetadata(mdMap))).append(" (from mdMap)\n");
builder
.append(formatDigest(MetadataDigestUtils.fromMetadata(mdMap)))
.append(" (from mdMap)\n");
} else {
builder.append(formatDigest(digest)).append("\n");
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import com.google.devtools.build.lib.util.PersistentMap;
import com.google.devtools.build.lib.util.StringIndexer;
import com.google.devtools.build.lib.util.VarInt;
import com.google.devtools.build.lib.vfs.DigestUtils;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.UnixGlob;
import java.io.ByteArrayOutputStream;
Expand Down Expand Up @@ -381,14 +382,14 @@ private static byte[] encode(StringIndexer indexer, ActionCache.Entry entry) {
VarInt.putVarInt(actionKeyBytes.length, sink);
sink.write(actionKeyBytes);

DigestUtils.write(entry.getFileDigest(), sink);
MetadataDigestUtils.write(entry.getFileDigest(), sink);

VarInt.putVarInt(entry.discoversInputs() ? files.size() : NO_INPUT_DISCOVERY_COUNT, sink);
for (String file : files) {
VarInt.putVarInt(indexer.getOrCreateIndex(file), sink);
}

DigestUtils.write(entry.getUsedClientEnvDigest(), sink);
MetadataDigestUtils.write(entry.getUsedClientEnvDigest(), sink);

return sink.toByteArray();
} catch (IOException e) {
Expand All @@ -410,7 +411,7 @@ private static ActionCache.Entry decode(StringIndexer indexer, byte[] data) thro
source.get(actionKeyBytes);
String actionKey = new String(actionKeyBytes, ISO_8859_1);

byte[] digest = DigestUtils.read(source);
byte[] digest = MetadataDigestUtils.read(source);

int count = VarInt.getVarInt(source);
if (count != NO_INPUT_DISCOVERY_COUNT && count < 0) {
Expand All @@ -430,7 +431,7 @@ private static ActionCache.Entry decode(StringIndexer indexer, byte[] data) thro
files = builder.build();
}

byte[] usedClientEnvDigest = DigestUtils.read(source);
byte[] usedClientEnvDigest = MetadataDigestUtils.read(source);

if (source.remaining() > 0) {
throw new IOException("serialized entry data has not been fully decoded");
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
// Copyright 2020 The Bazel Authors. All rights reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package com.google.devtools.build.lib.actions.cache;

import com.google.devtools.build.lib.actions.FileArtifactValue;
import com.google.devtools.build.lib.util.Fingerprint;
import com.google.devtools.build.lib.util.VarInt;
import com.google.devtools.build.lib.vfs.DigestUtils;
import java.io.IOException;
import java.io.OutputStream;
import java.nio.ByteBuffer;
import java.util.Map;
import javax.annotation.Nullable;

/** Utility class for digests/metadata relating to the action cache. */
public final class MetadataDigestUtils {
private MetadataDigestUtils() {}

/**
* @param source the byte buffer source.
* @return the digest from the given buffer.
*/
public static byte[] read(ByteBuffer source) {
int size = VarInt.getVarInt(source);
byte[] bytes = new byte[size];
source.get(bytes);
return bytes;
}

/** Write the digest to the output stream. */
public static void write(byte[] digest, OutputStream sink) throws IOException {
VarInt.putVarInt(digest.length, sink);
sink.write(digest);
}

/**
* @param mdMap A collection of (execPath, FileArtifactValue) pairs. Values may be null.
* @return an <b>order-independent</b> digest from the given "set" of (path, metadata) pairs.
*/
public static byte[] fromMetadata(Map<String, FileArtifactValue> mdMap) {
byte[] result = new byte[1]; // reserve the empty string
// Profiling showed that MessageDigest engine instantiation was a hotspot, so create one
// instance for this computation to amortize its cost.
Fingerprint fp = new Fingerprint();
for (Map.Entry<String, FileArtifactValue> entry : mdMap.entrySet()) {
result = DigestUtils.xor(result, getDigest(fp, entry.getKey(), entry.getValue()));
}
return result;
}

/**
* @param env A collection of (String, String) pairs.
* @return an order-independent digest of the given set of pairs.
*/
public static byte[] fromEnv(Map<String, String> env) {
byte[] result = new byte[0];
Fingerprint fp = new Fingerprint();
for (Map.Entry<String, String> entry : env.entrySet()) {
fp.addString(entry.getKey());
fp.addString(entry.getValue());
result = DigestUtils.xor(result, fp.digestAndReset());
}
return result;
}

private static byte[] getDigest(Fingerprint fp, String execPath, @Nullable FileArtifactValue md) {
fp.addString(execPath);
if (md != null) {
md.addTo(fp);
}
return fp.digestAndReset();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import com.google.devtools.build.lib.profiler.Profiler;
import com.google.devtools.build.lib.profiler.ProfilerTask;
import com.google.devtools.build.lib.util.io.OutErr;
import com.google.devtools.build.lib.vfs.DigestUtils;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
import java.io.IOException;
Expand Down Expand Up @@ -80,7 +81,9 @@ private static void updateRunfilesTree(
// symbolic link, it is likely a symbolic link to the input manifest, so we cannot trust it as
// an up-to-date check.
if (!outputManifest.isSymbolicLink()
&& Arrays.equals(outputManifest.getDigest(), inputManifest.getDigest())) {
&& Arrays.equals(
DigestUtils.getDigestWithManualFallbackWhenSizeUnknown(outputManifest),
DigestUtils.getDigestWithManualFallbackWhenSizeUnknown(inputManifest))) {
return;
}
} catch (IOException e) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import com.google.devtools.build.lib.remote.options.RemoteOptions;
import com.google.devtools.build.lib.util.io.MessageOutputStream;
import com.google.devtools.build.lib.vfs.DigestHashFunction;
import com.google.devtools.build.lib.vfs.DigestUtils;
import com.google.devtools.build.lib.vfs.Dirent;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
Expand Down Expand Up @@ -92,7 +93,7 @@ public void logSpawn(
ActionInput input = e.getValue();
Path inputPath = execRoot.getRelative(input.getExecPathString());
if (inputPath.isDirectory()) {
listDirectoryContents(inputPath, (file) -> builder.addInputs(file), metadataProvider);
listDirectoryContents(inputPath, builder::addInputs, metadataProvider);
} else {
Digest digest = computeDigest(input, null, metadataProvider);
builder.addInputsBuilder().setPath(input.getExecPathString()).setDigest(digest);
Expand All @@ -110,7 +111,7 @@ public void logSpawn(
for (Map.Entry<Path, ActionInput> e : existingOutputs.entrySet()) {
Path path = e.getKey();
if (path.isDirectory()) {
listDirectoryContents(path, (file) -> builder.addActualOutputs(file), metadataProvider);
listDirectoryContents(path, builder::addActualOutputs, metadataProvider);
} else {
File.Builder outputBuilder = builder.addActualOutputsBuilder();
outputBuilder.setPath(path.relativeTo(execRoot).toString());
Expand Down Expand Up @@ -233,9 +234,11 @@ private Digest computeDigest(
path = execRoot.getRelative(input.getExecPath());
}
// Compute digest manually.
long fileSize = path.getFileSize();
return digest
.setHash(HashCode.fromBytes(path.getDigest()).toString())
.setSizeBytes(path.getFileSize())
.setHash(
HashCode.fromBytes(DigestUtils.getDigestWithManualFallback(path, fileSize)).toString())
.setSizeBytes(fileSize)
.build();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,10 @@
import com.google.common.hash.HashCode;
import com.google.common.hash.HashingOutputStream;
import com.google.common.io.BaseEncoding;
import com.google.devtools.build.lib.actions.cache.DigestUtils;
import com.google.devtools.build.lib.actions.cache.VirtualActionInput;
import com.google.devtools.build.lib.remote.common.RemoteCacheClient.ActionKey;
import com.google.devtools.build.lib.vfs.DigestHashFunction;
import com.google.devtools.build.lib.vfs.DigestUtils;
import com.google.devtools.build.lib.vfs.Path;
import com.google.protobuf.Message;
import java.io.ByteArrayOutputStream;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.io.BaseEncoding;
import com.google.devtools.build.lib.actions.FileStateValue.RegularFileStateValue;
import com.google.devtools.build.lib.actions.FileValue;
import com.google.devtools.build.lib.analysis.BlazeDirectories;
import com.google.devtools.build.lib.analysis.RuleDefinition;
Expand Down Expand Up @@ -251,8 +250,9 @@ private static boolean verifyLabelMarkerData(Rule rule, String key, String value
public static String fileValueToMarkerValue(FileValue fileValue) throws IOException {
Preconditions.checkArgument(fileValue.isFile() && !fileValue.isSpecialFile());
// Return the file content digest in hex. fileValue may or may not have the digest available.
byte[] digest = ((RegularFileStateValue) fileValue.realFileStateValue()).getDigest();
byte[] digest = fileValue.realFileStateValue().getDigest();
if (digest == null) {
// Fast digest not available, or it would have been in the FileValue.
digest = fileValue.realRootedPath().asPath().getDigest();
}
return BaseEncoding.base16().lowerCase().encode(digest);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,10 @@
import com.google.common.base.Preconditions;
import com.google.common.cache.CacheStats;
import com.google.common.flogger.GoogleLogger;
import com.google.devtools.build.lib.actions.cache.DigestUtils;
import com.google.devtools.build.lib.buildtool.BuildRequest;
import com.google.devtools.build.lib.exec.ExecutionOptions;
import com.google.devtools.build.lib.exec.ExecutorBuilder;
import com.google.devtools.build.lib.vfs.DigestUtils;

/** Enables the caching of file digests in {@link DigestUtils}. */
public class CacheFileDigestsModule extends BlazeModule {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,9 @@
import com.google.devtools.build.lib.actions.FilesetManifest;
import com.google.devtools.build.lib.actions.FilesetManifest.RelativeSymlinkBehavior;
import com.google.devtools.build.lib.actions.FilesetOutputSymlink;
import com.google.devtools.build.lib.actions.cache.DigestUtils;
import com.google.devtools.build.lib.actions.cache.MetadataHandler;
import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor;
import com.google.devtools.build.lib.vfs.DigestUtils;
import com.google.devtools.build.lib.vfs.Dirent;
import com.google.devtools.build.lib.vfs.FileStatus;
import com.google.devtools.build.lib.vfs.FileStatusWithDigest;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact;
import com.google.devtools.build.lib.actions.FileArtifactValue;
import com.google.devtools.build.lib.actions.HasDigest;
import com.google.devtools.build.lib.actions.cache.DigestUtils;
import com.google.devtools.build.lib.actions.cache.MetadataDigestUtils;
import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
import com.google.devtools.build.lib.skyframe.serialization.autocodec.SerializationConstant;
import com.google.devtools.build.lib.util.Fingerprint;
Expand Down Expand Up @@ -131,7 +131,7 @@ static ArchivedRepresentation create(
@AutoCodec.VisibleForSerialization
static final TreeArtifactValue EMPTY =
new TreeArtifactValue(
DigestUtils.fromMetadata(ImmutableMap.of()),
MetadataDigestUtils.fromMetadata(ImmutableMap.of()),
ImmutableSortedMap.of(),
/*archivedRepresentation=*/ null,
/*entirelyRemote=*/ false);
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/com/google/devtools/build/lib/ssd/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ java_library(
srcs = glob(["*.java"]),
deps = [
"//src/main/java/com/google/devtools/build/lib:runtime",
"//src/main/java/com/google/devtools/build/lib/actions:file_metadata",
"//src/main/java/com/google/devtools/build/lib/vfs",
"//src/main/java/com/google/devtools/common/options",
"//third_party:guava",
],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,9 @@
package com.google.devtools.build.lib.ssd;

import com.google.common.collect.ImmutableList;
import com.google.devtools.build.lib.actions.cache.DigestUtils;
import com.google.devtools.build.lib.runtime.BlazeModule;
import com.google.devtools.build.lib.runtime.CommandEnvironment;
import com.google.devtools.build.lib.vfs.DigestUtils;
import com.google.devtools.common.options.OptionsBase;

/**
Expand Down
1 change: 0 additions & 1 deletion src/main/java/com/google/devtools/build/lib/vfs/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/concurrent",
"//src/main/java/com/google/devtools/build/lib/profiler",
"//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec",
"//src/main/java/com/google/devtools/build/lib/util:TestType",
"//src/main/java/com/google/devtools/build/lib/util:filetype",
"//src/main/java/com/google/devtools/common/options",
"//third_party:guava",
Expand Down
Loading

0 comments on commit 95704d1

Please sign in to comment.