From c15148af6c0227ea8360bcf180bd0784113472db Mon Sep 17 00:00:00 2001 From: Chi Wang Date: Tue, 14 Feb 2023 07:39:06 -0800 Subject: [PATCH] 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` (#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 #17478. PiperOrigin-RevId: 509524641 Change-Id: Id3c9dc07d68758770c092f6307e2433dad40ba10 --- .../lib/skyframe/ActionMetadataHandler.java | 71 +++++++++++++++---- .../google/devtools/build/lib/skyframe/BUILD | 1 + .../skyframe/ActionMetadataHandlerTest.java | 29 ++++++++ 3 files changed, 86 insertions(+), 15 deletions(-) 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 7bd727db1c78ae..1f0d3a65cb0fcd 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; @@ -512,7 +513,7 @@ private FileArtifactValue constructFileArtifactValue( throws IOException { checkState(!artifact.isTreeArtifact(), "%s is a tree artifact", artifact); - FileArtifactValue value = + var statAndValue = fileArtifactValueFromArtifact( artifact, artifactPathResolver, @@ -522,6 +523,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(); @@ -568,8 +570,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); } @@ -589,15 +600,16 @@ 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(); } - private static FileArtifactValue fileArtifactValueFromArtifact( + private static FileArtifactStatAndValue fileArtifactValueFromArtifact( Artifact artifact, ArtifactPathResolver artifactPathResolver, @Nullable FileStatusWithDigest statNoFollow, @@ -611,7 +623,9 @@ 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 = @@ -628,8 +642,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 @@ -649,8 +666,32 @@ 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( - realRootedPath, realStatWithDigest, digestWillBeInjected, xattrProvider, tsgm); + var fileArtifactValue = + fileArtifactValueFromStat( + realRootedPath, realStatWithDigest, digestWillBeInjected, xattrProvider, tsgm); + return FileArtifactStatAndValue.create(pathNoFollow, realPath, statNoFollow, 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 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 00cf2a3b3fc1db..0edc51d3c52529 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD @@ -520,6 +520,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 f70a7f951e9a78..ebc04c3ba3149c 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.OutputPermissions; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; @@ -756,4 +757,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); + } }