From 350e32952bdd639969e2aff7887c5dc34c21bce6 Mon Sep 17 00:00:00 2001 From: kshyanashree <109167932+kshyanashree@users.noreply.github.com> Date: Mon, 20 Feb 2023 05:17:07 -0800 Subject: [PATCH] [6.1.0]Fix symlink file creation overhead (#17488) * 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 * Update ActionMetadataHandler.java * Create OutputPermissions.java --------- Co-authored-by: Chi Wang Co-authored-by: keertk <110264242+keertk@users.noreply.github.com> --- .../lib/skyframe/ActionMetadataHandler.java | 66 +++++++++++++++---- .../google/devtools/build/lib/skyframe/BUILD | 1 + .../skyframe/ActionMetadataHandlerTest.java | 29 ++++++++ 3 files changed, 82 insertions(+), 14 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 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); + } }