Skip to content

Commit

Permalink
[6.1.0]Fix symlink file creation overhead (bazelbuild#17488)
Browse files Browse the repository at this point in the history 
* 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>
  • Loading branch information
@ShreeM01 @coeuvre @keertk
3 people committed Feb 20, 2023
1 parent ee1daaf commit 350e329
Show file tree
Hide file tree
Showing 3 changed files with 82 additions and 14 deletions.
66 changes: 52 additions & 14 deletions src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java
View file
Expand Up @@ -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;
Expand Down Expand Up @@ -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,
Expand All @@ -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();
Expand Down Expand Up @@ -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);
}
Expand All @@ -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,
Expand All @@ -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 =
Expand All @@ -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
Expand All @@ -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(
Expand Down
1 change: 1 addition & 0 deletions src/main/java/com/google/devtools/build/lib/skyframe/BUILD
View file
Expand Up @@ -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",
Expand Down
29 changes: 29 additions & 0 deletions src/test/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandlerTest.java
View file
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
}

0 comments on commit 350e329

Please sign in to comment.