diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionEnvironment.java b/src/main/java/com/google/devtools/build/lib/actions/ActionEnvironment.java index 2378a00650ea42..f1ff0a0aa849e3 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/ActionEnvironment.java +++ b/src/main/java/com/google/devtools/build/lib/actions/ActionEnvironment.java @@ -13,11 +13,10 @@ // limitations under the License. package com.google.devtools.build.lib.actions; -import com.google.common.base.Preconditions; +import static com.google.common.base.Preconditions.checkNotNull; + import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; -import com.google.common.collect.Iterables; -import com.google.devtools.build.lib.collect.CollectionUtils; import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec; import com.google.devtools.build.lib.util.Fingerprint; import java.util.LinkedHashMap; @@ -152,10 +151,9 @@ public static ActionEnvironment split(Map env) { } private final EnvironmentVariables fixedEnv; - private final Iterable inheritedEnv; + private final ImmutableSet inheritedEnv; - private ActionEnvironment(EnvironmentVariables fixedEnv, Iterable inheritedEnv) { - CollectionUtils.checkImmutable(inheritedEnv); + private ActionEnvironment(EnvironmentVariables fixedEnv, ImmutableSet inheritedEnv) { this.fixedEnv = fixedEnv; this.inheritedEnv = inheritedEnv; } @@ -167,15 +165,15 @@ private ActionEnvironment(EnvironmentVariables fixedEnv, Iterable inheri */ @AutoCodec.Instantiator public static ActionEnvironment create( - EnvironmentVariables fixedEnv, Iterable inheritedEnv) { - if (fixedEnv.isEmpty() && Iterables.isEmpty(inheritedEnv)) { + EnvironmentVariables fixedEnv, ImmutableSet inheritedEnv) { + if (fixedEnv.isEmpty() && inheritedEnv.isEmpty()) { return EMPTY; } return new ActionEnvironment(fixedEnv, inheritedEnv); } public static ActionEnvironment create( - Map fixedEnv, Iterable inheritedEnv) { + Map fixedEnv, ImmutableSet inheritedEnv) { return new ActionEnvironment(SimpleEnvironmentVariables.create(fixedEnv), inheritedEnv); } @@ -193,7 +191,7 @@ public ActionEnvironment addFixedVariables(Map vars) { /** Returns the combined size of the fixed and inherited environments. */ public int size() { - return fixedEnv.size() + Iterables.size(inheritedEnv); + return fixedEnv.size() + inheritedEnv.size(); } /** @@ -211,7 +209,7 @@ public EnvironmentVariables getFixedEnv() { * be used for testing and to compute the cache keys of actions. Use {@link #resolve} instead to * get the complete environment. */ - public Iterable getInheritedEnv() { + public ImmutableSet getInheritedEnv() { return inheritedEnv; } @@ -222,7 +220,7 @@ public Iterable getInheritedEnv() { *

We pass in a map to mutate to avoid creating and merging intermediate maps. */ public void resolve(Map result, Map clientEnv) { - Preconditions.checkNotNull(clientEnv); + checkNotNull(clientEnv); result.putAll(fixedEnv.toMap()); for (String var : inheritedEnv) { String value = clientEnv.get(var); 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 f609a2f9b8a012..6992fecfa0fe68 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 @@ -56,6 +56,10 @@ public final String getKey(ActionKeyContext actionKeyContext) { /** * See the javadoc for {@link Action} and {@link ActionAnalysisMetadata#getKey} for the contract * of this method. + * + *

TODO(b/150305897): subtypes of this are not consistent about adding the UUID as stated in + * the ActionAnalysisMetadata. Perhaps ActionKeyCacher should just mandate subclasses provide a + * UUID and then add that UUID itself in getKey. */ protected abstract void computeKey(ActionKeyContext actionKeyContext, Fingerprint fp) throws CommandLineExpansionException; diff --git a/src/main/java/com/google/devtools/build/lib/actions/Artifacts.java b/src/main/java/com/google/devtools/build/lib/actions/Artifacts.java new file mode 100644 index 00000000000000..0ae61ada7a70a3 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/actions/Artifacts.java @@ -0,0 +1,36 @@ +// 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; + +import com.google.devtools.build.lib.util.Fingerprint; +import java.util.Collection; + +/** Helper functions for dealing with {@link Artifacts} */ +public final class Artifacts { + + private Artifacts() {} + + public static void addToFingerprint(Fingerprint fp, Artifact artifact) { + fp.addString(artifact.getExecPathString()); + } + + // TODO(bazel-team): Add option to sort collection of artifacts before adding to fingerprint? + /** Appends a description of a complete collection of {@link Artifact} to the fingerprint. */ + public static void addToFingerprint(Fingerprint fp, Collection artifacts) { + fp.addInt(artifacts.size()); + for (Artifact artifact : artifacts) { + addToFingerprint(fp, artifact); + } + } +} diff --git a/src/main/java/com/google/devtools/build/lib/actions/CommandLineItem.java b/src/main/java/com/google/devtools/build/lib/actions/CommandLineItem.java index 070f41f5ae30e7..6445e43731df33 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/CommandLineItem.java +++ b/src/main/java/com/google/devtools/build/lib/actions/CommandLineItem.java @@ -75,7 +75,7 @@ interface CapturingMapFn extends MapFn {} * CommandLineItem#expandToCommandLine} method, else we call {@link Object#toString()}. */ static String expandToCommandLine(Object object) { - // TODO(bazel-team): The fallback on toString() isn't great. Particularly so for + // TODO(b/150322434): The fallback on toString() isn't great. Particularly so for // SkylarkCustomCommandLine, since toString() does not necessarily give the same results as // Skylark's str() or repr(). // 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 711c240aeeb957..c0f36de199e617 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 @@ -122,20 +122,20 @@ public int addToFingerprint( *

  • Simply add all arguments * * - *
    -   *   Examples:
    +   * 
    {@code
    +   * Examples:
        *
    -   *   List values = ImmutableList.of("1", "2", "3");
    +   * List values = ImmutableList.of("1", "2", "3");
        *
    -   *   commandBuilder.addAll(VectorArg.format("-l%s").each(values))
    -   *   -> ["-l1", "-l2", "-l3"]
    +   * commandBuilder.addAll(VectorArg.format("-l%s").each(values))
    +   * -> ["-l1", "-l2", "-l3"]
        *
    -   *   commandBuilder.addAll(VectorArg.addBefore("-l").each(values))
    -   *   -> ["-l", "1", "-l", "2", "-l", "3"]
    +   * commandBuilder.addAll(VectorArg.addBefore("-l").each(values))
    +   * -> ["-l", "1", "-l", "2", "-l", "3"]
        *
    -   *   commandBuilder.addAll(VectorArg.join(":").each(values))
    -   *   -> ["1:2:3"]
    -   * 
    + * commandBuilder.addAll(VectorArg.join(":").each(values)) + * -> ["1:2:3"] + * }
    */ @AutoCodec public static class VectorArg { @@ -444,7 +444,7 @@ public int addToFingerprint( int count = (Integer) arguments.get(argi++); if (mapFn != null) { for (int i = 0; i < count; ++i) { - mapFn.expandToCommandLine(arguments.get(argi++), fingerprint); + mapFn.expandToCommandLine(arguments.get(argi++), fingerprint::addString); } } else { for (int i = 0; i < count; ++i) { @@ -1079,9 +1079,9 @@ public Builder addExpandedTreeArtifactExecPaths(String arg, Artifact treeArtifac } /** - * Adds the arguments for all {@link TreeFileArtifact}s under - * {@code treeArtifact}, one argument per file. Using {@code expandingFunction} to expand each - * {@link TreeFileArtifact} to expected argument. + * Adds the arguments for all {@link TreeFileArtifact}s under {@code treeArtifact}, one argument + * per file. Using {@code expandFunction} to expand each {@link TreeFileArtifact} to expected + * argument. * * @param treeArtifact the TreeArtifact containing the {@link TreeFileArtifact}s to add. * @param expandFunction the function to generate the argument for each{@link TreeFileArtifact}. diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/BaselineCoverageAction.java b/src/main/java/com/google/devtools/build/lib/analysis/test/BaselineCoverageAction.java index 95244b46834b26..424c95e666017c 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/test/BaselineCoverageAction.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/test/BaselineCoverageAction.java @@ -20,6 +20,7 @@ import com.google.devtools.build.lib.actions.ActionKeyContext; import com.google.devtools.build.lib.actions.ActionOwner; import com.google.devtools.build.lib.actions.Artifact; +import com.google.devtools.build.lib.actions.Artifacts; import com.google.devtools.build.lib.actions.NotifyOnActionCacheHit; import com.google.devtools.build.lib.analysis.RuleContext; import com.google.devtools.build.lib.analysis.actions.AbstractFileWriteAction; @@ -36,8 +37,6 @@ import java.io.IOException; import java.io.OutputStream; import java.io.PrintWriter; -import java.util.ArrayList; -import java.util.List; /** Generates baseline (empty) coverage for the given non-test target. */ @VisibleForTesting @@ -61,15 +60,9 @@ public String getMnemonic() { @Override public void computeKey(ActionKeyContext actionKeyContext, Fingerprint fp) { - fp.addStrings(getInstrumentedFilePathStrings()); - } - - private Iterable getInstrumentedFilePathStrings() { - List result = new ArrayList<>(); - for (Artifact instrumentedFile : instrumentedFiles.toList()) { - result.add(instrumentedFile.getExecPathString()); - } - return result; + // TODO(b/150305897): No UUID? + // TODO(b/150308417): Sort? + Artifacts.addToFingerprint(fp, instrumentedFiles.toList()); } @Override @@ -78,8 +71,8 @@ public DeterministicWriter newDeterministicWriter(ActionExecutionContext ctx) { @Override public void writeOutputFile(OutputStream out) throws IOException { PrintWriter writer = new PrintWriter(out); - for (String execPath : getInstrumentedFilePathStrings()) { - writer.write("SF:" + execPath + "\n"); + for (Artifact file : instrumentedFiles.toList()) { + writer.write("SF:" + file.getExecPathString() + "\n"); writer.write("end_of_record\n"); } writer.flush(); diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/InstrumentedFileManifestAction.java b/src/main/java/com/google/devtools/build/lib/analysis/test/InstrumentedFileManifestAction.java index b242c167baeec9..0fe662e8a1c8b4 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/test/InstrumentedFileManifestAction.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/test/InstrumentedFileManifestAction.java @@ -17,13 +17,12 @@ import static java.nio.charset.StandardCharsets.ISO_8859_1; import com.google.common.annotations.VisibleForTesting; -import com.google.common.base.Function; import com.google.common.base.Preconditions; -import com.google.common.collect.Iterables; import com.google.devtools.build.lib.actions.ActionExecutionContext; import com.google.devtools.build.lib.actions.ActionKeyContext; import com.google.devtools.build.lib.actions.ActionOwner; import com.google.devtools.build.lib.actions.Artifact; +import com.google.devtools.build.lib.actions.Artifacts; import com.google.devtools.build.lib.analysis.RuleContext; import com.google.devtools.build.lib.analysis.actions.AbstractFileWriteAction; import com.google.devtools.build.lib.collect.nestedset.NestedSet; @@ -42,13 +41,6 @@ */ @Immutable final class InstrumentedFileManifestAction extends AbstractFileWriteAction { - private static final Function TO_EXEC_PATH = new Function() { - @Override - public String apply(Artifact artifact) { - return artifact.getExecPathString(); - } - }; - private static final String GUID = "3833f0a3-7ea1-4d9f-b96f-66eff4c922b0"; private final NestedSet files; @@ -70,7 +62,7 @@ public DeterministicWriter newDeterministicWriter(ActionExecutionContext ctx) { public void writeOutputFile(OutputStream out) throws IOException { // Sort the exec paths before writing them out. String[] fileNames = - Iterables.toArray(Iterables.transform(files.toList(), TO_EXEC_PATH), String.class); + files.toList().stream().map(Artifact::getExecPathString).toArray(String[]::new); Arrays.sort(fileNames); try (Writer writer = new OutputStreamWriter(out, ISO_8859_1)) { for (String name : fileNames) { @@ -84,9 +76,10 @@ public void writeOutputFile(OutputStream out) throws IOException { @Override protected void computeKey(ActionKeyContext actionKeyContext, Fingerprint fp) { + // TODO(b/150305897): use addUUID? fp.addString(GUID); - // Not sorting here is probably cheaper, though it might lead to unnecessary re-execution. - fp.addStrings(Iterables.transform(files.toList(), TO_EXEC_PATH)); + // TODO(b/150308417): Not sorting is probably cheaper, might lead to unnecessary re-execution. + Artifacts.addToFingerprint(fp, files.toList()); } /** diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java index 45d7668d50abe9..f67777a9e3a081 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java @@ -334,8 +334,9 @@ public ImmutableList> getTestOutputsMapping( @Override protected void computeKey(ActionKeyContext actionKeyContext, Fingerprint fp) throws CommandLineExpansionException { + // TODO(b/150305897): use addUUID? fp.addString(GUID); - fp.addStrings(executionSettings.getArgs().arguments()); + fp.addIterableStrings(executionSettings.getArgs().arguments()); fp.addString(Strings.nullToEmpty(executionSettings.getTestFilter())); fp.addBoolean(executionSettings.getTestRunnerFailFast()); RunUnder runUnder = executionSettings.getRunUnder(); @@ -349,12 +350,12 @@ protected void computeKey(ActionKeyContext actionKeyContext, Fingerprint fp) fp.addString(testProperties.getSize().toString()); fp.addString(testProperties.getTimeout().toString()); fp.addStrings(testProperties.getTags()); - fp.addInt(testProperties.isRemotable() ? 1 : 0); + fp.addBoolean(testProperties.isRemotable()); fp.addInt(shardNum); fp.addInt(executionSettings.getTotalShards()); fp.addInt(runNumber); fp.addInt(executionSettings.getTotalRuns()); - fp.addInt(configuration.isCodeCoverageEnabled() ? 1 : 0); + fp.addBoolean(configuration.isCodeCoverageEnabled()); fp.addStringMap(getExecutionInfo()); } diff --git a/src/main/java/com/google/devtools/build/lib/collect/nestedset/NestedSetFingerprintCache.java b/src/main/java/com/google/devtools/build/lib/collect/nestedset/NestedSetFingerprintCache.java index e8201ea8739d1f..46eddd8eef1771 100644 --- a/src/main/java/com/google/devtools/build/lib/collect/nestedset/NestedSetFingerprintCache.java +++ b/src/main/java/com/google/devtools/build/lib/collect/nestedset/NestedSetFingerprintCache.java @@ -60,7 +60,7 @@ public void addNestedSetToFingerprint( private void addNestedSetToFingerprintSlow( MapFn mapFn, Fingerprint fingerprint, NestedSet nestedSet) { for (T object : nestedSet.toList()) { - mapFn.expandToCommandLine(object, fingerprint); + addToFingerprint(mapFn, fingerprint, object); } } @@ -92,7 +92,7 @@ private void addToFingerprint( @VisibleForTesting void addToFingerprint( CommandLineItem.MapFn mapFn, Fingerprint fingerprint, T object) { - mapFn.expandToCommandLine(object, fingerprint); + mapFn.expandToCommandLine(object, fingerprint::addString); } private static Map, DigestMap> createMap() { diff --git a/src/main/java/com/google/devtools/build/lib/util/BigIntegerFingerprint.java b/src/main/java/com/google/devtools/build/lib/util/BigIntegerFingerprint.java index 6e418d2d3d1581..b35d7e64f35955 100644 --- a/src/main/java/com/google/devtools/build/lib/util/BigIntegerFingerprint.java +++ b/src/main/java/com/google/devtools/build/lib/util/BigIntegerFingerprint.java @@ -18,6 +18,7 @@ import java.math.BigInteger; import java.util.ArrayList; import java.util.List; +import java.util.UUID; import javax.annotation.Nullable; /** @@ -27,7 +28,9 @@ * appropriately smeared across our fingerprint range, and therefore composable more cheaply than by * hashing. */ +// TODO(b/150308424): Deprecate BigIntegerFingerprint public class BigIntegerFingerprint { + private static final UUID MARKER = UUID.fromString("28481318-fe19-454e-a60f-47922b398bca"); private final Fingerprint fingerprint = new Fingerprint(); private final List alreadySmearedFingerprints = new ArrayList<>(); private boolean seenNull = false; @@ -64,9 +67,12 @@ public BigIntegerFingerprint addPath(PathFragment pathFragment) { public BigIntegerFingerprint addBigIntegerOrdered(BigInteger bigInteger) { alreadySmearedFingerprints.add(bigInteger); // Make sure the ordering of this BigInteger with respect to the items added to the fingerprint - // is reflected in the output. Because no other method calls #addSInt, we can use it as a - // marker. - fingerprint.addSInt(1); + // is reflected in the output. Use a UUID as a marker since extremely unlikely for there to + // be a collision. + // TODO(b/150308424): This class should just add a boolean in each add call: + // true here and false for all others. OR, this add is entirely unnecessary if the location + // of BigInteger adds are not data-dependent. + fingerprint.addUUID(MARKER); return this; } @@ -83,6 +89,7 @@ public BigInteger getFingerprint() { return null; } BigInteger fp = new BigInteger(1, fingerprint.digestAndReset()); + // TODO(b/150312032): Is this still actually faster than hashing? for (BigInteger bigInteger : alreadySmearedFingerprints) { fp = BigIntegerFingerprintUtils.composeOrdered(fp, bigInteger); } diff --git a/src/main/java/com/google/devtools/build/lib/util/Fingerprint.java b/src/main/java/com/google/devtools/build/lib/util/Fingerprint.java index b52732327240af..b627011e008be1 100644 --- a/src/main/java/com/google/devtools/build/lib/util/Fingerprint.java +++ b/src/main/java/com/google/devtools/build/lib/util/Fingerprint.java @@ -14,6 +14,8 @@ package com.google.devtools.build.lib.util; +import static java.nio.charset.StandardCharsets.UTF_8; + import com.google.common.io.ByteStreams; import com.google.devtools.build.lib.vfs.DigestHashFunction; import com.google.devtools.build.lib.vfs.Path; @@ -21,21 +23,43 @@ import com.google.protobuf.ByteString; import com.google.protobuf.CodedOutputStream; import java.io.IOException; -import java.nio.charset.StandardCharsets; import java.security.DigestException; import java.security.DigestOutputStream; import java.security.MessageDigest; +import java.util.Collection; import java.util.Map; import java.util.UUID; -import java.util.function.Consumer; import javax.annotation.Nullable; /** - * Simplified wrapper for computing message digests. + * Simplified wrapper for using {@link MessageDigest} to generate fingerprints. + * + *

    A fingerprint is a cryptographic hash of a message that encodes the representation of an + * object. Two objects of the same type have the same fingerprint if and only if they are equal. + * This property allows fingerprints to be used as unique identifiers for objects of a particular + * type, and for fingerprint equivalence to be used as a proxy for object equivalence, and these + * properties hold even outside the process. Note that this is a stronger requirement than {@link + * Object#hashCode}, which allows unequal objects to share the same hash code. + * + *

    Values are added to the fingerprint by converting them to bytes and digesting the bytes. + * Therefore, there are two potential sources of bugs: 1) a proper hash collision where two distinct + * streams of bytes produce the same digest, and 2) a programming oversight whereby two unequal + * values produce the same bytes, or conversely, two equal values produce distinct bytes. + * + *

    The case of a hash collision is statistically very unlikely, so we just need to ensure a + * one-to-one relationship between equality classes of values and their byte representation. A good + * way to do this is to literally serialize the values such that there is enough information to + * unambiguously deserialize them. For example, when serializing a list of strings ({@link + * #addStrings}, it is enough to write each string's content along with its length, plus the overall + * number of strings in the list. This ensures that no other list of strings can generate the same + * bytes. Note that it is not necessary to avoid collisions between different fingerprinting methods + * (e.g., between {@link #addStrings} and {@link #addString}) because the caller will only use one + * or the other in a given context, or else the user is required to write a disambiguating tag if + * both are possible. * * @see java.security.MessageDigest */ -public final class Fingerprint implements Consumer { +public final class Fingerprint { // Make novel use of a CodedOutputStream, which is good at efficiently serializing data. By // flushing at the end of each digest we can continue to use the stream. @@ -101,8 +125,11 @@ public String hexDigestAndReset() { } /** - * Updates the digest with 0 or more bytes. Same as {@link #addBytes(byte[])}, but potentially - * more performant when only a {@link ByteString} is available. + * Appends the specified bytes to the fingerprint message. Same as {@link #addBytes(byte[])}, but + * faster when only a {@link ByteString} is available. + * + *

    The fingerprint directly injects the bytes with no framing or tags added. Thus, not + * guaranteed to be unambiguous; especially if input length is data-dependent. */ public Fingerprint addBytes(ByteString bytes) { try { @@ -113,13 +140,18 @@ public Fingerprint addBytes(ByteString bytes) { return this; } - /** Updates the digest with 0 or more bytes. */ + /** Appends the specified bytes to the fingerprint message. */ public Fingerprint addBytes(byte[] input) { addBytes(input, 0, input.length); return this; } - /** Updates the digest with the specified number of bytes starting at offset. */ + /** + * Appends the specified bytes to the fingerprint message, starting at offset. + * + *

    The bytes are directly injected into the fingerprint with no framing or tags added. Thus, + * not guaranteed to be unambiguous; especially if len is data-dependent. + */ public Fingerprint addBytes(byte[] input, int offset, int len) { try { codedOut.write(input, offset, len); @@ -134,7 +166,7 @@ public Fingerprint addBoolean(boolean input) { try { codedOut.writeBoolNoTag(input); } catch (IOException e) { - throw new IllegalStateException(e); + throw new IllegalStateException("failed to write bool", e); } return this; } @@ -150,30 +182,20 @@ public Fingerprint addNullableBoolean(Boolean input) { return this; } - /** Updates the digest with the varint representation of input. */ - public Fingerprint addInt(int input) { + /** Appends an int to the fingerprint message. */ + public Fingerprint addInt(int x) { try { - codedOut.writeInt32NoTag(input); + codedOut.writeInt32NoTag(x); } catch (IOException e) { - throw new IllegalStateException(e); + throw new IllegalStateException("failed to write int", e); } return this; } - /** Updates the digest with the signed varint representation of input. */ - Fingerprint addSInt(int input) { + /** Appends a long to the fingerprint message. */ + public Fingerprint addLong(long x) { try { - codedOut.writeSInt32NoTag(input); - } catch (IOException e) { - throw new IllegalStateException(e); - } - return this; - } - - /** Updates the digest with the varint representation of a long value. */ - public Fingerprint addLong(long input) { - try { - codedOut.writeInt64NoTag(input); + codedOut.writeInt64NoTag(x); } catch (IOException e) { throw new IllegalStateException("failed to write long", e); } @@ -191,14 +213,14 @@ public Fingerprint addNullableInt(@Nullable Integer input) { return this; } - /** Updates the digest with a UUID. */ + /** Appends a {@link UUID} to the fingerprint message. */ public Fingerprint addUUID(UUID uuid) { addLong(uuid.getLeastSignificantBits()); addLong(uuid.getMostSignificantBits()); return this; } - /** Updates the digest with a String using UTF8 encoding. */ + /** Appends a String to the fingerprint message. */ public Fingerprint addString(String input) { try { codedOut.writeStringNoTag(input); @@ -219,28 +241,45 @@ public Fingerprint addNullableString(@Nullable String input) { return this; } - /** Updates the digest with a {@link Path}. */ + /** Appends a {@link Path} to the fingerprint message. */ public Fingerprint addPath(Path input) { addString(input.getPathString()); return this; } - /** Updates the digest with a {@link PathFragment}. */ + /** Appends a {@link PathFragment} to the fingerprint message. */ public Fingerprint addPath(PathFragment input) { return addString(input.getPathString()); } /** - * Add the supplied sequence of {@link String}s to the digest as an atomic unit, that is this is - * different from adding them each individually. + * Appends a collection of strings to the fingerprint message as a unit. The collection must have + * a deterministic iteration order. + * + *

    The fingerprint effectively records the sequence of calls, not just the elements. That is, + * addStrings(x+y).addStrings(z) is different from addStrings(x).addStrings(y+z). + */ + public Fingerprint addStrings(Collection inputs) { + addInt(inputs.size()); + for (String input : inputs) { + addString(input); + } + + return this; + } + + /** + * Appends an arbitrary sequence of Strings as a unit. + * + *

    This is slightly less efficient than {@link #addStrings}. */ - public Fingerprint addStrings(Iterable inputs) { - int count = 0; + // TODO(b/150312032): Deprecate this method. + public Fingerprint addIterableStrings(Iterable inputs) { for (String input : inputs) { + addBoolean(true); addString(input); - count++; } - addInt(count); + addBoolean(false); return this; } @@ -256,20 +295,12 @@ public Fingerprint addStringMap(Map inputs) { return this; } - /** - * Add the supplied sequence of {@link PathFragment}s to the digest as an atomic unit, that is - * this is different from adding each item individually. - * - * @param inputs the paths with which to update the digest - */ - public Fingerprint addPaths(Iterable inputs) { - int count = 0; - for (PathFragment path : inputs) { - addPath(path); - count++; + /** Like {@link #addStrings} but for {@link PathFragment}. */ + public Fingerprint addPaths(Collection inputs) { + addInt(inputs.size()); + for (PathFragment input : inputs) { + addPath(input); } - addInt(count); - return this; } @@ -296,13 +327,6 @@ public static String getHexDigest(String input) { // loading in a few places, before setDefault() has been called, so these call-sites should be // removed before this can be done safely. return hexDigest( - DigestHashFunction.SHA256 - .cloneOrCreateMessageDigest() - .digest(input.getBytes(StandardCharsets.UTF_8))); - } - - @Override - public void accept(String s) { - addString(s); + DigestHashFunction.SHA256.cloneOrCreateMessageDigest().digest(input.getBytes(UTF_8))); } }