Skip to content

Commit

Permalink
Let DirectoryListingStateFunction re-use the PerBuildSyscallCache. T…
Browse files Browse the repository at this point in the history
…he listing

    function performs a readdir() that we will likely have already executed earlier
    for globbing.

    RELNOTES: None.
    PiperOrigin-RevId: 233908833
  • Loading branch information
Luca Di Grazia committed Sep 4, 2022
1 parent b505d58 commit e5bd4b9
Show file tree
Hide file tree
Showing 10 changed files with 57 additions and 44 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@
// limitations under the License.
package com.google.devtools.build.lib.skyframe;

import com.google.devtools.build.lib.skyframe.ExternalFilesHelper.FileType;
import com.google.devtools.build.lib.vfs.RootedPath;
import com.google.devtools.build.lib.vfs.Symlinks;
import com.google.devtools.build.lib.vfs.UnixGlob.FilesystemCalls;
import com.google.devtools.build.skyframe.SkyFunction;
import com.google.devtools.build.skyframe.SkyFunctionException;
Expand Down Expand Up @@ -51,18 +51,12 @@ public DirectoryListingStateValue compute(SkyKey skyKey, Environment env)
RootedPath dirRootedPath = (RootedPath) skyKey.argument();

try {
FileType fileType = externalFilesHelper.maybeHandleExternalFile(dirRootedPath, true, env);
externalFilesHelper.maybeHandleExternalFile(dirRootedPath, env);
if (env.valuesMissing()) {
return null;
}
if (fileType == FileType.EXTERNAL_REPO
|| fileType == FileType.EXTERNAL_IN_MANAGED_DIRECTORY) {
// Do not use syscallCache as files under repositories get generated during the build,
// while syscallCache is used independently from Skyframe and generally assumes
// the file system is frozen at the beginning of the build command.
return DirectoryListingStateValue.create(dirRootedPath);
}
return DirectoryListingStateValue.create(syscallCache.get().readdir(dirRootedPath.asPath()));
return DirectoryListingStateValue.create(
syscallCache.get().readdir(dirRootedPath.asPath(), Symlinks.NOFOLLOW));
} catch (ExternalFilesHelper.NonexistentImmutableExternalFileException e) {
// DirectoryListingStateValue.key assumes the path exists. This exception here is therefore
// indicative of a programming bug.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -293,6 +293,10 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
protected SkyframeProgressReceiver progressReceiver;
private final AtomicReference<CyclesReporter> cyclesReporter = new AtomicReference<>();

protected int modifiedFiles;
protected int outputDirtyFiles;
protected int modifiedFilesDuringPreviousBuild;

@VisibleForTesting boolean lastAnalysisDiscarded = false;

private boolean analysisCacheDiscarded = false;
Expand Down Expand Up @@ -936,7 +940,7 @@ private void discardAnalysisCache(
try (AutoProfiler p = AutoProfiler.logged("discarding analysis cache", logger)) {
lastAnalysisDiscarded = true;
Iterator<? extends Map.Entry<SkyKey, ? extends NodeEntry>> it =
memoizingEvaluator.getGraphEntries().iterator();
memoizingEvaluator.getGraphMap().entrySet().iterator();
while (it.hasNext()) {
Map.Entry<SkyKey, ? extends NodeEntry> keyAndEntry = it.next();
NodeEntry entry = keyAndEntry.getValue();
Expand Down Expand Up @@ -1167,6 +1171,16 @@ public SkyFunctionEnvironmentForTesting getSkyFunctionEnvironmentForTesting(
return new SkyFunctionEnvironmentForTesting(eventHandler, this);
}

/**
* Informs user about number of modified files (source and output files).
*/
// Note, that number of modified files in some cases can be bigger than actual number of
// modified files for targets in current request. Skyframe may check for modification all files
// from previous requests.
protected void informAboutNumberOfModifiedFiles() {
logger.info(String.format("Found %d modified files from last build", modifiedFiles));
}

public EventBus getEventBus() {
return eventBus.get();
}
Expand Down Expand Up @@ -2682,7 +2696,17 @@ public void evaluated(
}
}

public abstract ExecutionFinishedEvent createExecutionFinishedEvent();
public int getOutputDirtyFilesAndClear() {
int result = outputDirtyFiles;
outputDirtyFiles = 0;
return result;
}

public int getModifiedFilesDuringPreviousBuildAndClear() {
int result = modifiedFilesDuringPreviousBuild;
modifiedFilesDuringPreviousBuild = 0;
return result;
}

private <T extends SkyValue> EvaluationResult<T> evaluate(
Iterable<? extends SkyKey> roots,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,14 @@
import com.google.common.collect.ImmutableMap;
import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider;
import com.google.devtools.build.lib.bazel.BazelRepositoryModule;
import com.google.devtools.build.lib.bazel.repository.MavenDownloader;
import com.google.devtools.build.lib.bazel.repository.MavenServerFunction;
import com.google.devtools.build.lib.bazel.repository.cache.RepositoryCache;
import com.google.devtools.build.lib.bazel.repository.downloader.DownloadManager;
import com.google.devtools.build.lib.bazel.repository.downloader.HttpDownloader;
import com.google.devtools.build.lib.bazel.repository.skylark.SkylarkRepositoryFunction;
import com.google.devtools.build.lib.bazel.rules.BazelRulesModule;
import com.google.devtools.build.lib.packages.BuildFileName;
import com.google.devtools.build.lib.packages.PackageFactory.EnvironmentExtension;
import com.google.devtools.build.lib.rules.repository.ManagedDirectoriesKnowledge;
import com.google.devtools.build.lib.rules.repository.RepositoryDelegatorFunction;
import com.google.devtools.build.lib.rules.repository.RepositoryLoaderFunction;
import com.google.devtools.build.lib.skyframe.ActionEnvironmentFunction;
Expand Down Expand Up @@ -93,8 +93,7 @@ private Builder(Root workspaceDir, Path installBase, Path outputBase, AtomicBool
public BazelPackageLoader buildImpl() {
// Set up SkyFunctions and PrecomputedValues needed to make local repositories work correctly.
RepositoryCache repositoryCache = new RepositoryCache();
HttpDownloader httpDownloader = new HttpDownloader();
DownloadManager downloadManager = new DownloadManager(repositoryCache, httpDownloader);
HttpDownloader httpDownloader = new HttpDownloader(repositoryCache);
addExtraSkyFunctions(
ImmutableMap.<SkyFunctionName, SkyFunction>builder()
.put(
Expand All @@ -110,17 +109,17 @@ public BazelPackageLoader buildImpl() {
.put(
SkyFunctions.REPOSITORY_DIRECTORY,
new RepositoryDelegatorFunction(
BazelRepositoryModule.repositoryRules(),
new SkylarkRepositoryFunction(downloadManager),
BazelRepositoryModule.repositoryRules(
httpDownloader, new MavenDownloader(repositoryCache)),
new SkylarkRepositoryFunction(httpDownloader),
isFetch,
ImmutableMap::of,
directories,
ManagedDirectoriesKnowledge.NO_MANAGED_DIRECTORIES))
directories))
.put(SkyFunctions.REPOSITORY, new RepositoryLoaderFunction())
.put(MavenServerFunction.NAME, new MavenServerFunction(directories))
.build());
addExtraPrecomputedValues(
PrecomputedValue.injected(PrecomputedValue.ACTION_ENV, ImmutableMap.of()),
PrecomputedValue.injected(PrecomputedValue.REPO_ENV, ImmutableMap.of()),
PrecomputedValue.injected(
RepositoryDelegatorFunction.REPOSITORY_OVERRIDES,
Suppliers.ofInstance(ImmutableMap.of())),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@
import com.google.devtools.build.lib.rules.repository.RepositoryLoaderFunction;
import com.google.devtools.build.lib.skyframe.ExternalFilesHelper.ExternalFileAction;
import com.google.devtools.build.lib.skyframe.PackageLookupFunction.CrossRepositoryLabelViolationStrategy;
import com.google.devtools.build.lib.syntax.StarlarkSemantics;
import com.google.devtools.build.lib.syntax.SkylarkSemantics;
import com.google.devtools.build.lib.testutil.FoundationTestCase;
import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor;
import com.google.devtools.build.lib.vfs.PathFragment;
Expand Down Expand Up @@ -157,7 +157,7 @@ public final void setUp() throws Exception {
driver = new SequentialBuildDriver(evaluator);
PrecomputedValue.BUILD_ID.set(differencer, UUID.randomUUID());
PrecomputedValue.PATH_PACKAGE_LOCATOR.set(differencer, pkgLocator.get());
PrecomputedValue.STARLARK_SEMANTICS.set(differencer, StarlarkSemantics.DEFAULT_SEMANTICS);
PrecomputedValue.SKYLARK_SEMANTICS.set(differencer, SkylarkSemantics.DEFAULT_SEMANTICS);
RepositoryDelegatorFunction.REPOSITORY_OVERRIDES.set(
differencer, ImmutableMap.<RepositoryName, PathFragment>of());
RepositoryDelegatorFunction.DEPENDENCY_FOR_UNCONDITIONAL_FETCHING.set(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -106,9 +106,7 @@ public final void setUp() throws Exception {
skyFunctions.put(
FileStateValue.FILE_STATE,
new FileStateFunction(
new AtomicReference<TimestampGranularityMonitor>(),
new AtomicReference<>(UnixGlob.DEFAULT_SYSCALLS),
externalFilesHelper));
new AtomicReference<TimestampGranularityMonitor>(), externalFilesHelper));
skyFunctions.put(FileValue.FILE, new FileFunction(pkgLocator));
skyFunctions.put(SkyFunctions.DIRECTORY_LISTING, new DirectoryListingFunction());
skyFunctions.put(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@
import com.google.devtools.build.lib.skyframe.ExternalFilesHelper.ExternalFileAction;
import com.google.devtools.build.lib.skyframe.GlobValue.InvalidGlobPatternException;
import com.google.devtools.build.lib.skyframe.PackageLookupFunction.CrossRepositoryLabelViolationStrategy;
import com.google.devtools.build.lib.syntax.StarlarkSemantics;
import com.google.devtools.build.lib.syntax.SkylarkSemantics;
import com.google.devtools.build.lib.testutil.ManualClock;
import com.google.devtools.build.lib.testutil.TestConstants;
import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor;
Expand Down Expand Up @@ -137,7 +137,7 @@ public final void setUp() throws Exception {
driver = new SequentialBuildDriver(evaluator);
PrecomputedValue.BUILD_ID.set(differencer, UUID.randomUUID());
PrecomputedValue.PATH_PACKAGE_LOCATOR.set(differencer, pkgLocator.get());
PrecomputedValue.STARLARK_SEMANTICS.set(differencer, StarlarkSemantics.DEFAULT_SEMANTICS);
PrecomputedValue.SKYLARK_SEMANTICS.set(differencer, SkylarkSemantics.DEFAULT_SEMANTICS);
RepositoryDelegatorFunction.RESOLVED_FILE_INSTEAD_OF_WORKSPACE.set(
differencer, Optional.<RootedPath>absent());

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
import com.google.devtools.build.lib.rules.repository.RepositoryDelegatorFunction;
import com.google.devtools.build.lib.skyframe.ExternalFilesHelper.ExternalFileAction;
import com.google.devtools.build.lib.skyframe.PackageLookupFunction.CrossRepositoryLabelViolationStrategy;
import com.google.devtools.build.lib.syntax.StarlarkSemantics;
import com.google.devtools.build.lib.syntax.SkylarkSemantics;
import com.google.devtools.build.lib.testutil.FoundationTestCase;
import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor;
import com.google.devtools.build.lib.vfs.FileSystemUtils;
Expand Down Expand Up @@ -129,7 +129,7 @@ public final void setUp() throws Exception {
evaluator = new InMemoryMemoizingEvaluator(skyFunctions, differencer);
driver = new SequentialBuildDriver(evaluator);
PrecomputedValue.PATH_PACKAGE_LOCATOR.set(differencer, pkgLocator.get());
PrecomputedValue.STARLARK_SEMANTICS.set(differencer, StarlarkSemantics.DEFAULT_SEMANTICS);
PrecomputedValue.SKYLARK_SEMANTICS.set(differencer, SkylarkSemantics.DEFAULT_SEMANTICS);
RepositoryDelegatorFunction.RESOLVED_FILE_INSTEAD_OF_WORKSPACE.set(
differencer, Optional.<RootedPath>absent());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@
import com.google.devtools.build.lib.packages.ConstantRuleVisibility;
import com.google.devtools.build.lib.packages.NoSuchPackageException;
import com.google.devtools.build.lib.packages.NoSuchTargetException;
import com.google.devtools.build.lib.packages.StarlarkSemanticsOptions;
import com.google.devtools.build.lib.packages.SkylarkSemanticsOptions;
import com.google.devtools.build.lib.pkgcache.PackageCacheOptions;
import com.google.devtools.build.lib.pkgcache.PathPackageLocator;
import com.google.devtools.build.lib.rules.repository.RepositoryDelegatorFunction;
Expand Down Expand Up @@ -91,7 +91,7 @@ private void preparePackageLoading(Path... roots) {
Arrays.stream(roots).map(Root::fromPath).collect(ImmutableList.toImmutableList()),
BazelSkyframeExecutorConstants.BUILD_FILES_BY_PRIORITY),
packageCacheOptions,
Options.getDefaults(StarlarkSemanticsOptions.class),
Options.getDefaults(SkylarkSemanticsOptions.class),
UUID.randomUUID(),
ImmutableMap.<String, String>of(),
new TimestampGranularityMonitor(BlazeClock.instance()));
Expand Down Expand Up @@ -340,7 +340,7 @@ public void testGlobOrderStableWithLegacyAndSkyframeComponents() throws Exceptio
ImmutableList.of(Root.fromPath(rootDirectory)),
BazelSkyframeExecutorConstants.BUILD_FILES_BY_PRIORITY),
packageCacheOptions,
Options.getDefaults(StarlarkSemanticsOptions.class),
Options.getDefaults(SkylarkSemanticsOptions.class),
UUID.randomUUID(),
ImmutableMap.<String, String>of(),
tsgm);
Expand Down Expand Up @@ -501,7 +501,7 @@ public void testNonExistingSkylarkExtension() throws Exception {
String expectedMsg =
"error loading package 'test/skylark': "
+ "Unable to load file '//test/skylark:bad_extension.bzl': file doesn't exist";
assertThat(errorInfo.getException()).hasMessageThat().isEqualTo(expectedMsg);
assertThat(errorInfo.getException()).hasMessage(expectedMsg);
}

@Test
Expand All @@ -526,8 +526,7 @@ public void testNonExistingSkylarkExtensionFromExtension() throws Exception {
assertThat(result.hasError()).isTrue();
ErrorInfo errorInfo = result.getError(skyKey);
assertThat(errorInfo.getException())
.hasMessageThat()
.isEqualTo(
.hasMessage(
"error loading package 'test/skylark': "
+ "in /workspace/test/skylark/extension.bzl: "
+ "Unable to load file '//test/skylark:bad_extension.bzl': file doesn't exist");
Expand All @@ -554,8 +553,7 @@ public void testSymlinkCycleWithSkylarkExtension() throws Exception {
ErrorInfo errorInfo = result.getError(skyKey);
assertThat(errorInfo.getRootCauseOfException()).isEqualTo(skyKey);
assertThat(errorInfo.getException())
.hasMessageThat()
.isEqualTo(
.hasMessage(
"error loading package 'test/skylark': Encountered error while reading extension "
+ "file 'test/skylark/extension.bzl': Symlink cycle");
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@
import com.google.devtools.build.lib.skyframe.PackageLookupFunction.CrossRepositoryLabelViolationStrategy;
import com.google.devtools.build.lib.skyframe.PackageLookupValue.ErrorReason;
import com.google.devtools.build.lib.skyframe.PackageLookupValue.IncorrectRepositoryReferencePackageLookupValue;
import com.google.devtools.build.lib.syntax.StarlarkSemantics;
import com.google.devtools.build.lib.syntax.SkylarkSemantics;
import com.google.devtools.build.lib.testutil.FoundationTestCase;
import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor;
import com.google.devtools.build.lib.vfs.FileSystemUtils;
Expand Down Expand Up @@ -170,7 +170,7 @@ public final void setUp() throws Exception {
driver = new SequentialBuildDriver(evaluator);
PrecomputedValue.BUILD_ID.set(differencer, UUID.randomUUID());
PrecomputedValue.PATH_PACKAGE_LOCATOR.set(differencer, pkgLocator.get());
PrecomputedValue.STARLARK_SEMANTICS.set(differencer, StarlarkSemantics.DEFAULT_SEMANTICS);
PrecomputedValue.SKYLARK_SEMANTICS.set(differencer, SkylarkSemantics.DEFAULT_SEMANTICS);
RepositoryDelegatorFunction.REPOSITORY_OVERRIDES.set(
differencer, ImmutableMap.<RepositoryName, PathFragment>of());
RepositoryDelegatorFunction.DEPENDENCY_FOR_UNCONDITIONAL_FETCHING.set(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -389,16 +389,16 @@ private void invalidateDirectory(RootedPath path) {
differencer.invalidate(ImmutableList.of(DirectoryListingStateValue.key(path)));
}

private void invalidateDirectory(Artifact directoryArtifact) {
invalidateDirectory(rootedPath(directoryArtifact));
}

private void invalidateOutputArtifact(Artifact output) {
assertThat(output.isSourceArtifact()).isFalse();
differencer.invalidate(
ImmutableList.of(new NonHermeticArtifactSkyKey(ArtifactSkyKey.key(output, true))));
}

private void invalidateDirectory(Artifact directoryArtifact) {
invalidateDirectory(rootedPath(directoryArtifact));
}

private static final class RecordingEvaluationProgressReceiver
extends EvaluationProgressReceiver.NullEvaluationProgressReceiver {
Set<SkyKey> invalidations;
Expand Down

0 comments on commit e5bd4b9

Please sign in to comment.