Change Fingerprint to prefix-based framing/tagging. Clarify Fingerpri…

…nt docs.

This essentially reverts unknown commit. The primary file of interest for this CL is Fingerprint.java:

* addStrings and addPaths were be changed to take a Collection and prefix the length. See new docstring for why prefix is preferable.

* Add and use addIterableStrings (using a unary encoding for the length) for the one place where only Iterable<String> is available. See b/150312032.

* Add and use Artifacts.addToFingerprint static methods.

* Remove addSInt as was redundant to addInt.

* Fingerprint no longer implements Consumer<String>.

In other files:

* Added some TODOs/comments in ActionKeyCacher as subtypes are NOT consistent about the GUID part of the contract for computeKey. See b/150305897.

* BigIntegerFingerprint now uses a UUID as a marker, rather than -1. Various concerns raised about the need for the class. See b/150308424.

* ActionEnvironment.inheritedEnv is now Collection<> instead of Iterable<>

RELNOTES: None
PiperOrigin-RevId: 297927885

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<String, String> env) {
   }
 
   private final EnvironmentVariables fixedEnv;
-  private final Iterable<String> inheritedEnv;
+  private final ImmutableSet<String> inheritedEnv;
 
-  private ActionEnvironment(EnvironmentVariables fixedEnv, Iterable<String> inheritedEnv) {
-    CollectionUtils.checkImmutable(inheritedEnv);
+  private ActionEnvironment(EnvironmentVariables fixedEnv, ImmutableSet<String> inheritedEnv) {
     this.fixedEnv = fixedEnv;
     this.inheritedEnv = inheritedEnv;
   }
@@ -167,15 +165,15 @@ private ActionEnvironment(EnvironmentVariables fixedEnv, Iterable<String> inheri
    */
   @AutoCodec.Instantiator
   public static ActionEnvironment create(
-      EnvironmentVariables fixedEnv, Iterable<String> inheritedEnv) {
-    if (fixedEnv.isEmpty() && Iterables.isEmpty(inheritedEnv)) {
+      EnvironmentVariables fixedEnv, ImmutableSet<String> inheritedEnv) {
+    if (fixedEnv.isEmpty() && inheritedEnv.isEmpty()) {
       return EMPTY;
     }
     return new ActionEnvironment(fixedEnv, inheritedEnv);
   }
 
   public static ActionEnvironment create(
-      Map<String, String> fixedEnv, Iterable<String> inheritedEnv) {
+      Map<String, String> fixedEnv, ImmutableSet<String> inheritedEnv) {
     return new ActionEnvironment(SimpleEnvironmentVariables.create(fixedEnv), inheritedEnv);
   }
 
@@ -193,7 +191,7 @@ public ActionEnvironment addFixedVariables(Map<String, String> 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<String> getInheritedEnv() {
+  public ImmutableSet<String> getInheritedEnv() {
     return inheritedEnv;
   }
 
@@ -222,7 +220,7 @@ public Iterable<String> getInheritedEnv() {
    * <p>We pass in a map to mutate to avoid creating and merging intermediate maps.
    */
   public void resolve(Map<String, String> result, Map<String, String> clientEnv) {
-    Preconditions.checkNotNull(clientEnv);
+    checkNotNull(clientEnv);
     result.putAll(fixedEnv.toMap());
     for (String var : inheritedEnv) {
       String value = clientEnv.get(var);

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.
+   *
+   * <p>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;

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<Artifact> artifacts) {
+    fp.addInt(artifacts.size());
+    for (Artifact artifact : artifacts) {
+      addToFingerprint(fp, artifact);
+    }
+  }
+}

src/main/java/com/google/devtools/build/lib/actions/CommandLineItem.java

@@ -75,7 +75,7 @@ interface CapturingMapFn<T> extends MapFn<T> {}
    * 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().
     //

src/main/java/com/google/devtools/build/lib/analysis/actions/CustomCommandLine.java

@@ -122,20 +122,20 @@ public int addToFingerprint(
    *   <li>Simply add all arguments
    * </ul>
    *
-   * <pre>
-   *   Examples:
+   * <pre>{@code
+   * Examples:
    *
-   *   List<String> values = ImmutableList.of("1", "2", "3");
+   * List<String> 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"]
-   * </pre>
+   * commandBuilder.addAll(VectorArg.join(":").each(values))
+   * -> ["1:2:3"]
+   * }</pre>
    */
   @AutoCodec
   public static class VectorArg<T> {
@@ -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}.

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<String> getInstrumentedFilePathStrings() {
-    List<String> 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();

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<Artifact, String> TO_EXEC_PATH = new Function<Artifact, String>() {
-    @Override
-    public String apply(Artifact artifact) {
-      return artifact.getExecPathString();
-    }
-  };
-
   private static final String GUID = "3833f0a3-7ea1-4d9f-b96f-66eff4c922b0";
 
   private final NestedSet<Artifact> 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());
   }
 
   /**

src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java

@@ -334,8 +334,9 @@ public ImmutableList<Pair<String, Path>> 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());
   }
 

src/main/java/com/google/devtools/build/lib/collect/nestedset/NestedSetFingerprintCache.java

@@ -60,7 +60,7 @@ public <T> void addNestedSetToFingerprint(
   private <T> void addNestedSetToFingerprintSlow(
       MapFn<? super T> mapFn, Fingerprint fingerprint, NestedSet<T> nestedSet) {
     for (T object : nestedSet.toList()) {
-      mapFn.expandToCommandLine(object, fingerprint);
+      addToFingerprint(mapFn, fingerprint, object);
     }
   }
 
@@ -92,7 +92,7 @@ private <T> void addToFingerprint(
   @VisibleForTesting
   <T> void addToFingerprint(
       CommandLineItem.MapFn<? super T> mapFn, Fingerprint fingerprint, T object) {
-    mapFn.expandToCommandLine(object, fingerprint);
+    mapFn.expandToCommandLine(object, fingerprint::addString);
   }
 
   private static Map<CommandLineItem.MapFn<?>, DigestMap> createMap() {

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<BigInteger> 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);
     }