Add another `PackageFunctionWithoutGlobDeps` subclass to make non-skyframe globbing separate from skyframe hybrid
With this change, each globbing strategy are expected to have its dedicated `PackageFunction` subclass. So it is not necessary to keep the `PackageFunction#globbingStrategy` class field anymore.
PiperOrigin-RevId: 627484651
Change-Id: I713ff28892c6731b4ede410a7518e6c03bcc0f7a
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 dc2241a..ca10ec4 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
@@ -48,6 +48,7 @@
"LocalRepositoryLookupFunction.java",
"PackageFunction.java",
"PackageFunctionWithMultipleGlobDeps.java",
+ "PackageFunctionWithoutGlobDeps.java",
"PrepareDepsOfPatternFunction.java",
"SequencedSkyframeExecutor.java",
"SequencedSkyframeExecutorFactory.java",
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
index 742a490..b6a201d 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
@@ -112,7 +112,6 @@
private final ActionOnIOExceptionReadingBuildFile actionOnIOExceptionReadingBuildFile;
private final boolean shouldUseRepoDotBazel;
- protected final GlobbingStrategy globbingStrategy;
protected final Function<SkyKey, ThreadStateReceiver> threadStateReceiverFactoryForMetrics;
@@ -178,7 +177,6 @@
@Nullable PackageProgressReceiver packageProgress,
ActionOnIOExceptionReadingBuildFile actionOnIOExceptionReadingBuildFile,
boolean shouldUseRepoDotBazel,
- GlobbingStrategy globbingStrategy,
Function<SkyKey, ThreadStateReceiver> threadStateReceiverFactoryForMetrics,
AtomicReference<Semaphore> cpuBoundSemaphore) {
this.bzlLoadFunctionForInlining = bzlLoadFunctionForInlining;
@@ -189,7 +187,6 @@
this.packageProgress = packageProgress;
this.actionOnIOExceptionReadingBuildFile = actionOnIOExceptionReadingBuildFile;
this.shouldUseRepoDotBazel = shouldUseRepoDotBazel;
- this.globbingStrategy = globbingStrategy;
this.threadStateReceiverFactoryForMetrics = threadStateReceiverFactoryForMetrics;
this.cpuBoundSemaphore = cpuBoundSemaphore;
}
@@ -273,7 +270,10 @@
}
}
- /** Handles package's glob deps symlink issues discovered by Skyframe globbing. */
+ /**
+ * Queries GLOB deps in Skyframe if necessary, and handles package's glob deps symlink issues
+ * discovered by Skyframe globbing.
+ */
@ForOverride
protected abstract void handleGlobDepsAndPropagateFilesystemExceptions(
PackageIdentifier packageIdentifier,
@@ -1335,18 +1335,35 @@
}
public PackageFunction build() {
- return new PackageFunctionWithMultipleGlobDeps(
- packageFactory,
- pkgLocator,
- showLoadingProgress,
- numPackagesSuccessfullyLoaded,
- bzlLoadFunctionForInlining,
- packageProgress,
- actionOnIOExceptionReadingBuildFile,
- shouldUseRepoDotBazel,
- globbingStrategy,
- threadStateReceiverFactoryForMetrics,
- cpuBoundSemaphore);
+ switch (globbingStrategy) {
+ case SKYFRAME_HYBRID -> {
+ return new PackageFunctionWithMultipleGlobDeps(
+ packageFactory,
+ pkgLocator,
+ showLoadingProgress,
+ numPackagesSuccessfullyLoaded,
+ bzlLoadFunctionForInlining,
+ packageProgress,
+ actionOnIOExceptionReadingBuildFile,
+ shouldUseRepoDotBazel,
+ threadStateReceiverFactoryForMetrics,
+ cpuBoundSemaphore);
+ }
+ case NON_SKYFRAME -> {
+ return new PackageFunctionWithoutGlobDeps(
+ packageFactory,
+ pkgLocator,
+ showLoadingProgress,
+ numPackagesSuccessfullyLoaded,
+ bzlLoadFunctionForInlining,
+ packageProgress,
+ actionOnIOExceptionReadingBuildFile,
+ shouldUseRepoDotBazel,
+ threadStateReceiverFactoryForMetrics,
+ cpuBoundSemaphore);
+ }
+ }
+ throw new IllegalStateException();
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunctionWithMultipleGlobDeps.java b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunctionWithMultipleGlobDeps.java
index d767db9..e947f0b 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunctionWithMultipleGlobDeps.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunctionWithMultipleGlobDeps.java
@@ -56,14 +56,13 @@
/**
* Computes the {@link PackageValue} which depends on multiple GLOB nodes.
*
- * <p>Every glob pattern defined in the package's {@code BUILD} file is represented as a single
- * GLOB node in the dependency graph.
+ * <p>Every glob pattern defined in the package's {@code BUILD} file is represented as a single GLOB
+ * node in the dependency graph.
*
- * <p>{@link PackageFunctionWithMultipleGlobDeps} subclass supports both {@code SKYFRAME_HYBRID}
- * and {@code NON_SKYFRAME} globbing strategy. Incremental evaluation adopts {@code SKYFRAME_HYBRID}
- * globbing strategy, and it can just use the unchanged {@link GlobValue} stored in Skyframe when
- * incrementally reloading the package. On the other hand, {@code NON_SKYFRAME} globbing strategy is
- * used for non-incremental evaluations with no GLOB node queried and stored in Skyframe.
+ * <p>{@link PackageFunctionWithMultipleGlobDeps} subclass is created when the globbing strategy is
+ * {@link com.google.devtools.build.lib.skyframe.PackageFunction.GlobbingStrategy#SKYFRAME_HYBRID} .
+ * Incremental evaluation adopts {@code SKYFRAME_HYBRID} globbing strategy in order to use the
+ * unchanged {@link GlobValue} stored in Skyframe when incrementally reloading the package.
*/
final class PackageFunctionWithMultipleGlobDeps extends PackageFunction {
private static final GoogleLogger logger = GoogleLogger.forEnclosingClass();
@@ -77,7 +76,6 @@
@Nullable PackageProgressReceiver packageProgress,
ActionOnIOExceptionReadingBuildFile actionOnIOExceptionReadingBuildFile,
boolean shouldUseRepoDotBazel,
- GlobbingStrategy globbingStrategy,
Function<SkyKey, ThreadStateReceiver> threadStateReceiverFactoryForMetrics,
AtomicReference<Semaphore> cpuBoundSemaphore) {
super(
@@ -89,7 +87,6 @@
packageProgress,
actionOnIOExceptionReadingBuildFile,
shouldUseRepoDotBazel,
- globbingStrategy,
threadStateReceiverFactoryForMetrics,
cpuBoundSemaphore);
}
@@ -134,59 +131,16 @@
}
}
- private static class LoadedPackageWithGlobDeps extends LoadedPackage {
+ private static final class LoadedPackageWithGlobDeps extends LoadedPackage {
private final Set<SkyKey> globDepKeys;
private LoadedPackageWithGlobDeps(
- Package.Builder builder, Set<SkyKey> globDepKeys, long loadTimeNanos) {
+ Package.Builder builder, long loadTimeNanos, Set<SkyKey> globDepKeys) {
super(builder, loadTimeNanos);
this.globDepKeys = globDepKeys;
}
}
- private interface GlobberWithSkyframeGlobDeps extends Globber {
- Set<SkyKey> getGlobDepsRequested();
- }
-
- private static class NonSkyframeGlobberWithNoGlobDeps implements GlobberWithSkyframeGlobDeps {
- private final NonSkyframeGlobber delegate;
-
- private NonSkyframeGlobberWithNoGlobDeps(NonSkyframeGlobber delegate) {
- this.delegate = delegate;
- }
-
- @Override
- public Set<SkyKey> getGlobDepsRequested() {
- return ImmutableSet.of();
- }
-
- @Override
- public Token runAsync(
- List<String> includes,
- List<String> excludes,
- Globber.Operation globberOperation,
- boolean allowEmpty)
- throws BadGlobException, InterruptedException {
- return delegate.runAsync(includes, excludes, globberOperation, allowEmpty);
- }
-
- @Override
- public List<String> fetchUnsorted(Token token)
- throws BadGlobException, IOException, InterruptedException {
- return delegate.fetchUnsorted(token);
- }
-
- @Override
- public void onInterrupt() {
- delegate.onInterrupt();
- }
-
- @Override
- public void onCompletion() {
- delegate.onCompletion();
- }
- }
-
/**
* A {@link Globber} implemented on top of Skyframe that falls back to a {@link
* NonSkyframeGlobber} on a Skyframe cache-miss. This way we don't require a Skyframe restart
@@ -244,7 +198,7 @@
* encountering all glob calls (meaning the real pass can still have the core problem with
* Skyframe restarts).
*/
- private static class SkyframeHybridGlobber implements GlobberWithSkyframeGlobDeps {
+ private static class SkyframeHybridGlobber implements Globber {
private final PackageIdentifier packageId;
private final Root packageRoot;
private final Environment env;
@@ -262,7 +216,6 @@
this.nonSkyframeGlobber = nonSkyframeGlobber;
}
- @Override
public Set<SkyKey> getGlobDepsRequested() {
return ImmutableSet.copyOf(globDepsRequested);
}
@@ -414,11 +367,11 @@
SkyKey globKey, SkyframeLookupResult globValueMap) throws IOException {
try {
return checkNotNull(
- (GlobValue)
- globValueMap.getOrThrow(
- globKey, BuildFileNotFoundException.class, IOException.class),
- "%s should not be missing",
- globKey)
+ (GlobValue)
+ globValueMap.getOrThrow(
+ globKey, BuildFileNotFoundException.class, IOException.class),
+ "%s should not be missing",
+ globKey)
.getMatches();
} catch (BuildFileNotFoundException e) {
throw new SkyframeGlobbingIOException(e);
@@ -434,21 +387,12 @@
}
@Override
- protected GlobberWithSkyframeGlobDeps makeGlobber(
+ protected Globber makeGlobber(
NonSkyframeGlobber nonSkyframeGlobber,
PackageIdentifier packageId,
Root packageRoot,
Environment env) {
- switch (globbingStrategy) {
- case SKYFRAME_HYBRID:
- return new SkyframeHybridGlobber(packageId, packageRoot, env, nonSkyframeGlobber);
- case NON_SKYFRAME:
- // Skyframe globbing is only useful for incremental correctness and performance. The
- // first time Bazel loads a package ever, Skyframe globbing is actually pure overhead
- // (SkyframeHybridGlobber will make full use of NonSkyframeGlobber).
- return new NonSkyframeGlobberWithNoGlobDeps(nonSkyframeGlobber);
- }
- throw new AssertionError(globbingStrategy);
+ return new SkyframeHybridGlobber(packageId, packageRoot, env, nonSkyframeGlobber);
}
@Override
@@ -456,8 +400,8 @@
Package.Builder packageBuilder, @Nullable Globber globber, long loadTimeNanos) {
Set<SkyKey> globDepKeys = ImmutableSet.of();
if (globber != null) {
- globDepKeys = ((GlobberWithSkyframeGlobDeps) globber).getGlobDepsRequested();
+ globDepKeys = ((SkyframeHybridGlobber) globber).getGlobDepsRequested();
}
- return new LoadedPackageWithGlobDeps(packageBuilder, globDepKeys, loadTimeNanos);
+ return new LoadedPackageWithGlobDeps(packageBuilder, loadTimeNanos, globDepKeys);
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunctionWithoutGlobDeps.java b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunctionWithoutGlobDeps.java
new file mode 100644
index 0000000..dc4aae4
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunctionWithoutGlobDeps.java
@@ -0,0 +1,94 @@
+// Copyright 2024 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.google.devtools.build.lib.skyframe;
+
+import com.google.devtools.build.lib.actions.ThreadStateReceiver;
+import com.google.devtools.build.lib.cmdline.PackageIdentifier;
+import com.google.devtools.build.lib.packages.CachingPackageLocator;
+import com.google.devtools.build.lib.packages.Globber;
+import com.google.devtools.build.lib.packages.NonSkyframeGlobber;
+import com.google.devtools.build.lib.packages.Package;
+import com.google.devtools.build.lib.packages.PackageFactory;
+import com.google.devtools.build.lib.vfs.Root;
+import com.google.devtools.build.skyframe.SkyKey;
+import java.util.concurrent.Semaphore;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.concurrent.atomic.AtomicReference;
+import java.util.function.Function;
+import javax.annotation.Nullable;
+
+/**
+ * Computes the {@link PackageValue} without performing Skyframe globbing.
+ *
+ * <p>{@link PackageFunctionWithoutGlobDeps} subclass is created when the globbing strategy is
+ * {@link com.google.devtools.build.lib.skyframe.PackageFunction.GlobbingStrategy#NON_SKYFRAME},
+ * which is used for non-incremental evaluations with no GLOB nodes queried and stored in Skyframe.
+ */
+final class PackageFunctionWithoutGlobDeps extends PackageFunction {
+
+ PackageFunctionWithoutGlobDeps(
+ PackageFactory packageFactory,
+ CachingPackageLocator pkgLocator,
+ AtomicBoolean showLoadingProgress,
+ AtomicInteger numPackagesSuccessfullyLoaded,
+ @Nullable BzlLoadFunction bzlLoadFunctionForInlining,
+ @Nullable PackageProgressReceiver packageProgress,
+ ActionOnIOExceptionReadingBuildFile actionOnIOExceptionReadingBuildFile,
+ boolean shouldUseRepoDotBazel,
+ Function<SkyKey, ThreadStateReceiver> threadStateReceiverFactoryForMetrics,
+ AtomicReference<Semaphore> cpuBoundSemaphore) {
+ super(
+ packageFactory,
+ pkgLocator,
+ showLoadingProgress,
+ numPackagesSuccessfullyLoaded,
+ bzlLoadFunctionForInlining,
+ packageProgress,
+ actionOnIOExceptionReadingBuildFile,
+ shouldUseRepoDotBazel,
+ threadStateReceiverFactoryForMetrics,
+ cpuBoundSemaphore);
+ }
+
+ private static final class LoadedPackageWithoutDeps extends LoadedPackage {
+ LoadedPackageWithoutDeps(Package.Builder builder, long loadTimeNanos) {
+ super(builder, loadTimeNanos);
+ }
+ }
+
+ @Override
+ protected void handleGlobDepsAndPropagateFilesystemExceptions(
+ PackageIdentifier packageIdentifier,
+ LoadedPackage loadedPackage,
+ Environment env,
+ boolean packageWasInError) {
+ // No-op for non-Skyframe globbing.
+ }
+
+ @Override
+ protected Globber makeGlobber(
+ NonSkyframeGlobber nonSkyframeGlobber,
+ PackageIdentifier packageId,
+ Root packageRoot,
+ Environment env) {
+ return nonSkyframeGlobber;
+ }
+
+ @Override
+ protected LoadedPackage newLoadedPackage(
+ Package.Builder packageBuilder, @Nullable Globber globber, long loadTimeNanos) {
+ return new LoadedPackageWithoutDeps(packageBuilder, loadTimeNanos);
+ }
+}
