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); + } +}