From 970e822f151d0bcc791698b1e12cc89308934dda Mon Sep 17 00:00:00 2001 From: Googler Date: Thu, 12 Jan 2023 02:51:39 -0800 Subject: [PATCH] Remove option to disable FJP. FJP has been enabled for a quite a while virtually everywhere. PiperOrigin-RevId: 501512884 Change-Id: I7c05252463cb53357db5328467a4ddb46ae17ea5 --- .../lib/bazel/rules/BazelRulesModule.java | 9 +++++++++ .../lib/buildtool/BuildRequestOptions.java | 9 --------- .../lib/concurrent/AbstractQueueVisitor.java | 19 +------------------ .../QueryTransitivePackagePreloader.java | 1 - .../build/lib/skyframe/SkyframeExecutor.java | 2 -- .../skyframe/AbstractParallelEvaluator.java | 19 +++++-------------- .../build/skyframe/EvaluationContext.java | 16 ---------------- .../skyframe/InMemoryMemoizingEvaluator.java | 4 +--- .../QueryTransitivePackagePreloaderTest.java | 2 -- 9 files changed, 16 insertions(+), 65 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRulesModule.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRulesModule.java index 0fd6672a0b67c7..b19883326b8ed5 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRulesModule.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRulesModule.java @@ -483,6 +483,15 @@ public static final class AllCommandGraveyardOptions extends OptionsBase { effectTags = {OptionEffectTag.NO_OP}, help = "No-op") public boolean incompatibleDisableThirdPartyLicenseChecking; + + @Option( + name = "experimental_use_fork_join_pool", + defaultValue = "true", + documentationCategory = OptionDocumentationCategory.UNDOCUMENTED, + metadataTags = OptionMetadataTag.DEPRECATED, + effectTags = {OptionEffectTag.NO_OP}, + help = "No-op.") + public boolean useForkJoinPool; } @Override diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequestOptions.java b/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequestOptions.java index 5b9b24f3e88b56..e4e42403309d37 100644 --- a/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequestOptions.java +++ b/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequestOptions.java @@ -463,15 +463,6 @@ public boolean useTopLevelTargetsForSymlinks() { + "https://github.com/bazelbuild/bazel/issues/8651") public boolean incompatibleSkipGenfilesSymlink; - @Option( - name = "experimental_use_fork_join_pool", - defaultValue = "false", - documentationCategory = OptionDocumentationCategory.UNDOCUMENTED, - metadataTags = OptionMetadataTag.EXPERIMENTAL, - effectTags = {OptionEffectTag.EXECUTION}, - help = "If this flag is set, use a fork join pool in the abstract queue visitor.") - public boolean useForkJoinPool; - @Option( name = "experimental_replay_action_out_err", defaultValue = "false", diff --git a/src/main/java/com/google/devtools/build/lib/concurrent/AbstractQueueVisitor.java b/src/main/java/com/google/devtools/build/lib/concurrent/AbstractQueueVisitor.java index 91593c9d753dcf..c82475e1511af4 100644 --- a/src/main/java/com/google/devtools/build/lib/concurrent/AbstractQueueVisitor.java +++ b/src/main/java/com/google/devtools/build/lib/concurrent/AbstractQueueVisitor.java @@ -29,7 +29,6 @@ import java.util.concurrent.CountDownLatch; import java.util.concurrent.ExecutorService; import java.util.concurrent.ForkJoinPool; -import java.util.concurrent.PriorityBlockingQueue; import java.util.concurrent.RejectedExecutionException; import java.util.concurrent.ThreadPoolExecutor; import java.util.concurrent.TimeUnit; @@ -147,9 +146,6 @@ private static ExecutorService createExecutorService( BlockingQueue workQueue, String poolName) { - if ("1".equals(System.getProperty("experimental_use_fork_join_pool"))) { - return new NamedForkJoinPool(poolName, parallelism); - } return new ThreadPoolExecutor( /*corePoolSize=*/ parallelism, /*maximumPoolSize=*/ parallelism, @@ -161,21 +157,8 @@ private static ExecutorService createExecutorService( .build()); } - public static ExecutorService createExecutorService( - int parallelism, String poolName, boolean useForkJoinPool) { - if (useForkJoinPool) { - return new NamedForkJoinPool(poolName, parallelism); - } - return createExecutorService(parallelism, poolName); - } - public static ExecutorService createExecutorService(int parallelism, String poolName) { - return createExecutorService( - parallelism, - /*keepAliveTime=*/ 1, - TimeUnit.SECONDS, - new PriorityBlockingQueue<>(), - poolName); + return new NamedForkJoinPool(poolName, parallelism); } public static AbstractQueueVisitor createWithExecutorService( diff --git a/src/main/java/com/google/devtools/build/lib/query2/common/QueryTransitivePackagePreloader.java b/src/main/java/com/google/devtools/build/lib/query2/common/QueryTransitivePackagePreloader.java index fb7dc43e65318f..df5df13d8eedd1 100644 --- a/src/main/java/com/google/devtools/build/lib/query2/common/QueryTransitivePackagePreloader.java +++ b/src/main/java/com/google/devtools/build/lib/query2/common/QueryTransitivePackagePreloader.java @@ -146,7 +146,6 @@ public void preloadTransitiveTargets( .setKeepGoing(keepGoing) .setNumThreads(parallelThreads) .setEventHandler(eventHandler) - .setUseForkJoinPool(true) .build(); EvaluationResult result = memoizingEvaluatorSupplier.get().evaluate(valueNames, evaluationContext); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java index cab49ab361b123..3fd975ffe6f81a 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java @@ -1506,7 +1506,6 @@ public EvaluationResult buildArtifacts( newEvaluationContextBuilder() .setKeepGoing(options.getOptions(KeepGoingOption.class).keepGoing) .setNumThreads(options.getOptions(BuildRequestOptions.class).jobs) - .setUseForkJoinPool(options.getOptions(BuildRequestOptions.class).useForkJoinPool) .setEventHandler(reporter) .setExecutionPhase() .build(); @@ -1643,7 +1642,6 @@ EvaluationResult targetPatterns( .setKeepGoing(keepGoing) .setNumThreads(numThreads) .setEventHandler(eventHandler) - .setUseForkJoinPool(true) .build(); return memoizingEvaluator.evaluate(patternSkyKeys, evaluationContext); } diff --git a/src/main/java/com/google/devtools/build/skyframe/AbstractParallelEvaluator.java b/src/main/java/com/google/devtools/build/skyframe/AbstractParallelEvaluator.java index b89e732645e6d0..c4cf0c6f2ae4cf 100644 --- a/src/main/java/com/google/devtools/build/skyframe/AbstractParallelEvaluator.java +++ b/src/main/java/com/google/devtools/build/skyframe/AbstractParallelEvaluator.java @@ -179,11 +179,8 @@ private static Supplier getQuiescingExecutorSupplier( MultiExecutorQueueVisitor.createWithExecutorServices( executorService.get(), AbstractQueueVisitor.createExecutorService( - /*parallelism=*/ cpuHeavySkyKeysThreadPoolSize, - "skyframe-evaluator-cpu-heavy", - // FJP performs much better on machines with many cores. - /*useForkJoinPool=*/ true), - /*failFastOnException=*/ true, + /* parallelism= */ cpuHeavySkyKeysThreadPoolSize, "skyframe-evaluator-cpu-heavy"), + /* failFastOnException= */ true, NodeEntryVisitor.NODE_ENTRY_VISITOR_ERROR_CLASSIFIER); } // We only consider the experimental case of merged Skyframe phases WITH a separate pool for @@ -192,16 +189,10 @@ private static Supplier getQuiescingExecutorSupplier( MultiExecutorQueueVisitor.createWithExecutorServices( executorService.get(), AbstractQueueVisitor.createExecutorService( - /*parallelism=*/ cpuHeavySkyKeysThreadPoolSize, - "skyframe-evaluator-cpu-heavy", - // FJP performs much better on machines with many cores. - /*useForkJoinPool=*/ true), + /* parallelism= */ cpuHeavySkyKeysThreadPoolSize, "skyframe-evaluator-cpu-heavy"), AbstractQueueVisitor.createExecutorService( - /*parallelism=*/ executionJobsThreadPoolSize, - "skyframe-evaluator-execution", - // FJP performs much better on machines with many cores. - /*useForkJoinPool=*/ true), - /*failFastOnException=*/ true, + /* parallelism= */ executionJobsThreadPoolSize, "skyframe-evaluator-execution"), + /* failFastOnException= */ true, NodeEntryVisitor.NODE_ENTRY_VISITOR_ERROR_CLASSIFIER); } diff --git a/src/main/java/com/google/devtools/build/skyframe/EvaluationContext.java b/src/main/java/com/google/devtools/build/skyframe/EvaluationContext.java index 39b72110329e60..c01e85da11857c 100644 --- a/src/main/java/com/google/devtools/build/skyframe/EvaluationContext.java +++ b/src/main/java/com/google/devtools/build/skyframe/EvaluationContext.java @@ -33,7 +33,6 @@ public class EvaluationContext { @Nullable private final Supplier executorServiceSupplier; private final boolean keepGoing; private final ExtendedEventHandler eventHandler; - private final boolean useForkJoinPool; private final boolean isExecutionPhase; private final int cpuHeavySkyKeysThreadPoolSize; private final int executionPhaseThreadPoolSize; @@ -44,7 +43,6 @@ protected EvaluationContext( @Nullable Supplier executorServiceSupplier, boolean keepGoing, ExtendedEventHandler eventHandler, - boolean useForkJoinPool, boolean isExecutionPhase, int cpuHeavySkyKeysThreadPoolSize, int executionPhaseThreadPoolSize, @@ -54,7 +52,6 @@ protected EvaluationContext( this.executorServiceSupplier = executorServiceSupplier; this.keepGoing = keepGoing; this.eventHandler = Preconditions.checkNotNull(eventHandler); - this.useForkJoinPool = useForkJoinPool; this.isExecutionPhase = isExecutionPhase; this.cpuHeavySkyKeysThreadPoolSize = cpuHeavySkyKeysThreadPoolSize; this.executionPhaseThreadPoolSize = executionPhaseThreadPoolSize; @@ -77,10 +74,6 @@ public ExtendedEventHandler getEventHandler() { return eventHandler; } - public boolean getUseForkJoinPool() { - return useForkJoinPool; - } - /** * Returns the size of the thread pool for CPU-heavy tasks set by * --experimental_skyframe_cpu_heavy_skykeys_thread_pool_size. @@ -157,7 +150,6 @@ public static class Builder { protected Supplier executorServiceSupplier; protected boolean keepGoing; protected ExtendedEventHandler eventHandler; - protected boolean useForkJoinPool; protected int cpuHeavySkyKeysThreadPoolSize; protected int executionJobsThreadPoolSize = 0; protected boolean isExecutionPhase = false; @@ -173,7 +165,6 @@ protected Builder copyFrom(EvaluationContext evaluationContext) { this.keepGoing = evaluationContext.keepGoing; this.eventHandler = evaluationContext.eventHandler; this.isExecutionPhase = evaluationContext.isExecutionPhase; - this.useForkJoinPool = evaluationContext.useForkJoinPool; this.executionJobsThreadPoolSize = evaluationContext.executionPhaseThreadPoolSize; this.cpuHeavySkyKeysThreadPoolSize = evaluationContext.cpuHeavySkyKeysThreadPoolSize; this.unnecessaryTemporaryStateDropperReceiver = @@ -205,12 +196,6 @@ public Builder setEventHandler(ExtendedEventHandler eventHandler) { return this; } - @CanIgnoreReturnValue - public Builder setUseForkJoinPool(boolean useForkJoinPool) { - this.useForkJoinPool = useForkJoinPool; - return this; - } - @CanIgnoreReturnValue public Builder setCPUHeavySkyKeysThreadPoolSize(int cpuHeavySkyKeysThreadPoolSize) { this.cpuHeavySkyKeysThreadPoolSize = cpuHeavySkyKeysThreadPoolSize; @@ -242,7 +227,6 @@ public EvaluationContext build() { executorServiceSupplier, keepGoing, eventHandler, - useForkJoinPool, isExecutionPhase, cpuHeavySkyKeysThreadPoolSize, executionJobsThreadPoolSize, diff --git a/src/main/java/com/google/devtools/build/skyframe/InMemoryMemoizingEvaluator.java b/src/main/java/com/google/devtools/build/skyframe/InMemoryMemoizingEvaluator.java index a8068fafedf05f..4c0a01fe7c2774 100644 --- a/src/main/java/com/google/devtools/build/skyframe/InMemoryMemoizingEvaluator.java +++ b/src/main/java/com/google/devtools/build/skyframe/InMemoryMemoizingEvaluator.java @@ -221,9 +221,7 @@ private static EvaluationContext ensureExecutorService(EvaluationContext evaluat .setExecutorServiceSupplier( () -> AbstractQueueVisitor.createExecutorService( - evaluationContext.getParallelism(), - "skyframe-evaluator", - evaluationContext.getUseForkJoinPool())) + evaluationContext.getParallelism(), "skyframe-evaluator")) .build(); } diff --git a/src/test/java/com/google/devtools/build/lib/query2/common/QueryTransitivePackagePreloaderTest.java b/src/test/java/com/google/devtools/build/lib/query2/common/QueryTransitivePackagePreloaderTest.java index 0c33206ea47e7f..787c11a0ab3aa3 100644 --- a/src/test/java/com/google/devtools/build/lib/query2/common/QueryTransitivePackagePreloaderTest.java +++ b/src/test/java/com/google/devtools/build/lib/query2/common/QueryTransitivePackagePreloaderTest.java @@ -92,8 +92,6 @@ public void setUpMocks() { when(contextBuilder.setKeepGoing(ArgumentMatchers.anyBoolean())).thenReturn(contextBuilder); when(contextBuilder.setNumThreads(ArgumentMatchers.anyInt())).thenReturn(contextBuilder); when(contextBuilder.setEventHandler(ArgumentMatchers.any())).thenReturn(contextBuilder); - when(contextBuilder.setUseForkJoinPool(ArgumentMatchers.anyBoolean())) - .thenReturn(contextBuilder); when(contextBuilder.build()).thenReturn(context); }