[6.1.0]Fix symlink file creation overhead (bazelbuild#17488)

ShreeM01 · coeuvre · keertk · web-flow · commit 350e32952bdd · 2023-02-20T14:17:07.000+01:00
* Fix symlink file creation overhead The cost of symlink action scales with the size of input because Bazel re-calculates the digest of the output by following the symlink in `actuallyCompleteAction` (bazelbuild#14125). However, the re-calculation is redundant because the digest was already computed by Bazel when checking the outputs of the generating action. Bazel should be smart enough to reuse the result. There is a global cache in Bazel for digest computation. Symlink action didn't make use of the cache because it uses the path of symlink as key to look up the cache. This PR changes to use the path of input file (i.e. target path) to query the cache to avoid recalculation. For a large target (700MB), the time for symlink action is reduced from 2000ms to 1ms. Closes bazelbuild#17478. PiperOrigin-RevId: 509524641 Change-Id: Id3c9dc07d68758770c092f6307e2433dad40ba10 * Update ActionMetadataHandler.java * Create OutputPermissions.java --------- Co-authored-by: Chi Wang <chiwang@google.com> Co-authored-by: keertk <110264242+keertk@users.noreply.github.com>
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java
@@ -18,6 +18,7 @@
 import static com.google.common.base.Preconditions.checkState;
 import static java.util.concurrent.TimeUnit.MINUTES;
 
+import com.google.auto.value.AutoValue;
 import com.google.common.base.MoreObjects;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
@@ -505,7 +506,7 @@ private FileArtifactValue constructFileArtifactValue(
       throws IOException {
     checkState(!artifact.isTreeArtifact(), "%s is a tree artifact", artifact);
 
-    FileArtifactValue value =
+    var statAndValue =
         fileArtifactValueFromArtifact(
             artifact,
             artifactPathResolver,
@@ -515,6 +516,7 @@ private FileArtifactValue constructFileArtifactValue(
             // Prevent constant metadata artifacts from notifying the timestamp granularity monitor
             // and potentially delaying the build for no reason.
             artifact.isConstantMetadata() ? null : tsgm);
+    var value = statAndValue.fileArtifactValue();
 
     // Ensure that we don't have both an injected digest and a digest from the filesystem.
     byte[] fileDigest = value.getDigest();
@@ -561,8 +563,17 @@ private FileArtifactValue constructFileArtifactValue(
     if (injectedDigest == null && type.isFile()) {
       // We don't have an injected digest and there is no digest in the file value (which attempts a
       // fast digest). Manually compute the digest instead.
-      injectedDigest =
-          DigestUtils.manuallyComputeDigest(artifactPathResolver.toPath(artifact), value.getSize());
+      Path path = statAndValue.pathNoFollow();
+      if (statAndValue.statNoFollow() != null
+          && statAndValue.statNoFollow().isSymbolicLink()
+          && statAndValue.realPath() != null) {
+        // If the file is a symlink, we compute the digest using the target path so that it's
+        // possible to hit the digest cache - we probably already computed the digest for the
+        // target during previous action execution.
+        path = statAndValue.realPath();
+      }
+
+      injectedDigest = DigestUtils.manuallyComputeDigest(path, value.getSize());
     }
     return FileArtifactValue.createFromInjectedDigest(value, injectedDigest);
   }
@@ -582,15 +593,37 @@ static FileArtifactValue fileArtifactValueFromArtifact(
       @Nullable TimestampGranularityMonitor tsgm)
       throws IOException {
     return fileArtifactValueFromArtifact(
-        artifact,
-        ArtifactPathResolver.IDENTITY,
-        statNoFollow,
-        /*digestWillBeInjected=*/ false,
-        xattrProvider,
-        tsgm);
+            artifact,
+            ArtifactPathResolver.IDENTITY,
+            statNoFollow,
+            /* digestWillBeInjected= */ false,
+            xattrProvider,
+            tsgm).fileArtifactValue();
+  }
+  
+  @AutoValue
+  abstract static class FileArtifactStatAndValue {
+    public static FileArtifactStatAndValue create(
+        Path pathNoFollow,
+        @Nullable Path realPath,
+        @Nullable FileStatusWithDigest statNoFollow,
+        FileArtifactValue fileArtifactValue) {
+      return new AutoValue_ActionMetadataHandler_FileArtifactStatAndValue(
+          pathNoFollow, realPath, statNoFollow, fileArtifactValue);
+    }
+
+    public abstract Path pathNoFollow();
+
+    @Nullable
+    public abstract Path realPath();
+
+    @Nullable
+    public abstract FileStatusWithDigest statNoFollow();
+
+    public abstract FileArtifactValue fileArtifactValue();
   }
 
-  private static FileArtifactValue fileArtifactValueFromArtifact(
+  private static FileArtifactStatAndValue fileArtifactValueFromArtifact(
       Artifact artifact,
       ArtifactPathResolver artifactPathResolver,
       @Nullable FileStatusWithDigest statNoFollow,
@@ -604,7 +637,8 @@ private static FileArtifactValue fileArtifactValueFromArtifact(
     // If we expect a symlink, we can readlink it directly and handle errors appropriately - there
     // is no need for the stat below.
     if (artifact.isSymlink()) {
-      return FileArtifactValue.createForUnresolvedSymlink(pathNoFollow);
+      var fileArtifactValue = FileArtifactValue.createForUnresolvedSymlink(pathNoFollow);
+      return FileArtifactStatAndValue.create(pathNoFollow, /* realPath= */ null, statNoFollow, fileArtifactValue);
     }
 
     RootedPath rootedPathNoFollow =
@@ -621,8 +655,11 @@ private static FileArtifactValue fileArtifactValueFromArtifact(
     }
 
     if (statNoFollow == null || !statNoFollow.isSymbolicLink()) {
-      return fileArtifactValueFromStat(
-          rootedPathNoFollow, statNoFollow, digestWillBeInjected, xattrProvider, tsgm);
+      var fileArtifactValue =
+          fileArtifactValueFromStat(
+              rootedPathNoFollow, statNoFollow, digestWillBeInjected, xattrProvider, tsgm);
+      return FileArtifactStatAndValue.create(
+          pathNoFollow, /* realPath= */ null, statNoFollow, fileArtifactValue);
     }
 
     // We use FileStatus#isSymbolicLink over Path#isSymbolicLink to avoid the unnecessary stat
@@ -642,8 +679,9 @@ private static FileArtifactValue fileArtifactValueFromArtifact(
     // and is a source file (since changes to those are checked separately).
     FileStatus realStat = realRootedPath.asPath().statIfFound(Symlinks.NOFOLLOW);
     FileStatusWithDigest realStatWithDigest = FileStatusWithDigestAdapter.maybeAdapt(realStat);
-    return fileArtifactValueFromStat(
+    var fileArtifactValue = fileArtifactValueFromStat(
         realRootedPath, realStatWithDigest, digestWillBeInjected, xattrProvider, tsgm);
+    return FileArtifactStatAndValue.create(pathNoFollow, realPath, statNoFollow, fileArtifactValue);
   }
 
   private static FileArtifactValue fileArtifactValueFromStat(
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
@@ -515,6 +515,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/util/io",
         "//src/main/java/com/google/devtools/build/lib/vfs",
         "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
+        "//third_party:auto_value",
         "//third_party:flogger",
         "//third_party:guava",
         "//third_party:jsr305",
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandlerTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandlerTest.java
@@ -41,6 +41,7 @@
 import com.google.devtools.build.lib.testutil.Scratch;
 import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor;
 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.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.lib.vfs.Root;
@@ -714,4 +715,32 @@ public void fileArtifactValueFromArtifactCompatibleWithGetMetadata_notChanged()
     assertThat(fileArtifactValueFromArtifactResult.couldBeModifiedSince(getMetadataResult))
         .isFalse();
   }
+
+  @Test
+  public void fileArtifactValueForSymlink_readFromCache() throws Exception {
+    DigestUtils.configureCache(1);
+    Artifact target =
+        ActionsTestUtil.createArtifactWithRootRelativePath(
+            outputRoot, PathFragment.create("bin/target"));
+    scratch.file(target.getPath().getPathString(), "contents");
+    Artifact symlink =
+        ActionsTestUtil.createArtifactWithRootRelativePath(
+            outputRoot, PathFragment.create("bin/symlink"));
+    scratch
+        .getFileSystem()
+        .getPath(symlink.getPath().getPathString())
+        .createSymbolicLink(scratch.getFileSystem().getPath(target.getPath().getPathString()));
+    ActionMetadataHandler handler =
+        createHandler(
+            new ActionInputMap(0),
+            /* forInputDiscovery= */ false,
+            /* outputs= */ ImmutableSet.of(target, symlink));
+    var targetMetadata = handler.getMetadata(target);
+    assertThat(DigestUtils.getCacheStats().hitCount()).isEqualTo(0);
+
+    var symlinkMetadata = handler.getMetadata(symlink);
+
+    assertThat(symlinkMetadata).isEqualTo(targetMetadata);
+    assertThat(DigestUtils.getCacheStats().hitCount()).isEqualTo(1);
+  }
 }
