[6.1.0]Fix symlink file creation overhead

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(

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",

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