Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[6.3.0] Remove option to disable FJP. #18791

Merged
merged 2 commits into from
Jun 28, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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,
Expand All @@ -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(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,6 @@ public void preloadTransitiveTargets(
.setKeepGoing(keepGoing)
.setNumThreads(parallelThreads)
.setEventHandler(eventHandler)
.setUseForkJoinPool(true)
.build();
EvaluationResult<SkyValue> result =
memoizingEvaluatorSupplier.get().evaluate(valueNames, evaluationContext);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -1643,7 +1642,6 @@ EvaluationResult<SkyValue> targetPatterns(
.setKeepGoing(keepGoing)
.setNumThreads(numThreads)
.setEventHandler(eventHandler)
.setUseForkJoinPool(true)
.build();
return memoizingEvaluator.evaluate(patternSkyKeys, evaluationContext);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -44,7 +43,6 @@ protected EvaluationContext(
@Nullable Supplier<ExecutorService> executorServiceSupplier,
boolean keepGoing,
ExtendedEventHandler eventHandler,
boolean useForkJoinPool,
boolean isExecutionPhase,
int cpuHeavySkyKeysThreadPoolSize,
int executionPhaseThreadPoolSize,
Expand All @@ -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;
Expand All @@ -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.
Expand Down Expand Up @@ -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;
Expand All @@ -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 =
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -242,7 +227,6 @@ public EvaluationContext build() {
executorServiceSupplier,
keepGoing,
eventHandler,
useForkJoinPool,
isExecutionPhase,
cpuHeavySkyKeysThreadPoolSize,
executionJobsThreadPoolSize,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -221,9 +221,7 @@ private static EvaluationContext ensureExecutorService(EvaluationContext evaluat
.setExecutorServiceSupplier(
() ->
AbstractQueueVisitor.createExecutorService(
evaluationContext.getParallelism(),
"skyframe-evaluator",
evaluationContext.getUseForkJoinPool()))
evaluationContext.getParallelism(), "skyframe-evaluator"))
.build();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down