Remove option to disable FJP. (#18791)

FJP has been enabled for a quite a while virtually everywhere. PiperOrigin-RevId: 501512884 Change-Id: I7c05252463cb53357db5328467a4ddb46ae17ea5 Co-authored-by: Googler <twerth@google.com>
bazelbuild · Jun 28, 2023 · 2065438 · 2065438
1 parent 946777a
commit 2065438
Show file tree

Hide file tree

Showing 9 changed files with 16 additions and 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
@@ -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
@@ -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
@@ -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<Runnable> 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/...ain/java/com/google/devtools/build/lib/query2/common/QueryTransitivePackagePreloader.java b/...ain/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<SkyValue> 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
@@ -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<SkyValue> 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
@@ -179,11 +179,8 @@ private static Supplier<QuiescingExecutor> 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<QuiescingExecutor> 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
@@ -33,7 +33,6 @@ public class EvaluationContext {
   @Nullable private final Supplier<ExecutorService> 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<ExecutorService> 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<ExecutorService> 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
@@ -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/...java/com/google/devtools/build/lib/query2/common/QueryTransitivePackagePreloaderTest.java b/...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);
   }