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 index f6e45f06189dcb..a9385f9f81260a 100644 --- 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 index 18c7e3f8d80085..43fde9cc68ae10 100644 --- 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 index bdf714b4fc0abd..8d58e33016e7b1 100644 --- 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); + } }