From 6581536d8a4db2bf8f5d223d1a4eb7f4dbc4d0c7 Mon Sep 17 00:00:00 2001 From: Colin Alworth Date: Wed, 6 Dec 2023 16:48:47 -0600 Subject: [PATCH] Review feedback --- .../deephaven/engine/context/ExecutionContext.java | 1 - .../engine/context/PoisonedOperationInitializer.java | 5 ----- .../impl/OperationInitializationThreadPool.java | 9 ++------- .../engine/updategraph/impl/PeriodicUpdateGraph.java | 8 ++++---- .../engine/context/TestExecutionContext.java | 4 ++-- .../engine/testutil/ControlledUpdateGraph.java | 7 ++----- .../engine/testutil/QueryTableTestBase.java | 3 +-- .../engine/updategraph/OperationInitializer.java | 12 +----------- 8 files changed, 12 insertions(+), 37 deletions(-) diff --git a/engine/context/src/main/java/io/deephaven/engine/context/ExecutionContext.java b/engine/context/src/main/java/io/deephaven/engine/context/ExecutionContext.java index 8a628eb865f..23dbb825643 100644 --- a/engine/context/src/main/java/io/deephaven/engine/context/ExecutionContext.java +++ b/engine/context/src/main/java/io/deephaven/engine/context/ExecutionContext.java @@ -230,7 +230,6 @@ public static class Builder { private OperationInitializer operationInitializer = PoisonedOperationInitializer.INSTANCE; private Builder() { - // why automatically propagate this, but not other things? // propagate the auth context from the current context this(getContext().authContext); } diff --git a/engine/context/src/main/java/io/deephaven/engine/context/PoisonedOperationInitializer.java b/engine/context/src/main/java/io/deephaven/engine/context/PoisonedOperationInitializer.java index ee1d111215c..49474844755 100644 --- a/engine/context/src/main/java/io/deephaven/engine/context/PoisonedOperationInitializer.java +++ b/engine/context/src/main/java/io/deephaven/engine/context/PoisonedOperationInitializer.java @@ -27,9 +27,4 @@ public Future submit(Runnable runnable) { public int parallelismFactor() { return fail(); } - - @Override - public void start() { - fail(); - } } diff --git a/engine/table/src/main/java/io/deephaven/engine/table/impl/OperationInitializationThreadPool.java b/engine/table/src/main/java/io/deephaven/engine/table/impl/OperationInitializationThreadPool.java index 053897ef337..286d4386d17 100644 --- a/engine/table/src/main/java/io/deephaven/engine/table/impl/OperationInitializationThreadPool.java +++ b/engine/table/src/main/java/io/deephaven/engine/table/impl/OperationInitializationThreadPool.java @@ -56,6 +56,8 @@ public Thread newThread(@NotNull final Runnable r) { }; executorService = new ThreadPoolExecutor( NUM_THREADS, NUM_THREADS, 0L, TimeUnit.MILLISECONDS, new LinkedBlockingQueue<>(), threadFactory); + + executorService.prestartAllCoreThreads(); } @Override @@ -72,11 +74,4 @@ public Future submit(Runnable runnable) { public int parallelismFactor() { return NUM_THREADS; } - - /** - * Start the OperationInitializationThreadPool. In practice, this just pre-starts all threads. - */ - public void start() { - executorService.prestartAllCoreThreads(); - } } diff --git a/engine/table/src/main/java/io/deephaven/engine/updategraph/impl/PeriodicUpdateGraph.java b/engine/table/src/main/java/io/deephaven/engine/updategraph/impl/PeriodicUpdateGraph.java index 0678d47e27a..3632332f275 100644 --- a/engine/table/src/main/java/io/deephaven/engine/updategraph/impl/PeriodicUpdateGraph.java +++ b/engine/table/src/main/java/io/deephaven/engine/updategraph/impl/PeriodicUpdateGraph.java @@ -123,7 +123,7 @@ public PeriodicUpdateGraph( this.allowUnitTestMode = allowUnitTestMode; this.defaultTargetCycleDurationMillis = targetCycleDurationMillis; this.targetCycleDurationMillis = targetCycleDurationMillis; - this.threadInitializationFactory = threadInitializationFactory; + this.threadInitializationFactory = threadInitializationFactory; if (numUpdateThreads <= 0) { this.updateThreads = Runtime.getRuntime().availableProcessors(); @@ -1211,9 +1211,9 @@ public Builder numUpdateThreads(int numUpdateThreads) { } /** - * - * @param threadInitializationFactory - * @return + * Sets a functional interface that adds custom initialization for threads started by this UpdateGraph. + * @param threadInitializationFactory the function to invoke on any runnables that will be used to start threads + * @return this builder */ public Builder threadInitializationFactory(ThreadInitializationFactory threadInitializationFactory) { this.threadInitializationFactory = threadInitializationFactory; diff --git a/engine/test-utils/src/main/java/io/deephaven/engine/context/TestExecutionContext.java b/engine/test-utils/src/main/java/io/deephaven/engine/context/TestExecutionContext.java index 55356484c8f..1ff3a0a3c51 100644 --- a/engine/test-utils/src/main/java/io/deephaven/engine/context/TestExecutionContext.java +++ b/engine/test-utils/src/main/java/io/deephaven/engine/context/TestExecutionContext.java @@ -6,8 +6,8 @@ import io.deephaven.util.thread.ThreadInitializationFactory; public class TestExecutionContext { - private static final ControlledUpdateGraph UPDATE_GRAPH = - new ControlledUpdateGraph("TEST", true, 1000, 25, -1, ThreadInitializationFactory.NO_OP); + private static final ControlledUpdateGraph UPDATE_GRAPH = new ControlledUpdateGraph(); + private static final OperationInitializationThreadPool OPERATION_INITIALIZATION = new OperationInitializationThreadPool(ThreadInitializationFactory.NO_OP); diff --git a/engine/test-utils/src/main/java/io/deephaven/engine/testutil/ControlledUpdateGraph.java b/engine/test-utils/src/main/java/io/deephaven/engine/testutil/ControlledUpdateGraph.java index 1e20bee290f..0ca0d815015 100644 --- a/engine/test-utils/src/main/java/io/deephaven/engine/testutil/ControlledUpdateGraph.java +++ b/engine/test-utils/src/main/java/io/deephaven/engine/testutil/ControlledUpdateGraph.java @@ -5,10 +5,7 @@ // TODO (deephaven-core#3886): Extract test functionality from PeriodicUpdateGraph public class ControlledUpdateGraph extends PeriodicUpdateGraph { - public ControlledUpdateGraph(String name, boolean allowUnitTestMode, long targetCycleDurationMillis, - long minimumCycleDurationToLogNanos, int numUpdateThreads, - ThreadInitializationFactory threadInitializationFactory) { - super(name, allowUnitTestMode, targetCycleDurationMillis, minimumCycleDurationToLogNanos, numUpdateThreads, - threadInitializationFactory); + public ControlledUpdateGraph() { + super("TEST", true, 1000, 25, -1, ThreadInitializationFactory.NO_OP); } } diff --git a/engine/test-utils/src/main/java/io/deephaven/engine/testutil/QueryTableTestBase.java b/engine/test-utils/src/main/java/io/deephaven/engine/testutil/QueryTableTestBase.java index 65a87fa85ac..6121df0b38e 100644 --- a/engine/test-utils/src/main/java/io/deephaven/engine/testutil/QueryTableTestBase.java +++ b/engine/test-utils/src/main/java/io/deephaven/engine/testutil/QueryTableTestBase.java @@ -10,7 +10,6 @@ import io.deephaven.engine.table.impl.ShiftObliviousInstrumentedListenerAdapter; import io.deephaven.engine.table.impl.util.ShiftObliviousUpdateCoalescer; import io.deephaven.engine.testutil.testcase.RefreshingTableTestCase; -import io.deephaven.engine.updategraph.impl.PeriodicUpdateGraph; import io.deephaven.engine.util.TableDiff; import io.deephaven.engine.util.TableTools; import org.apache.commons.lang3.mutable.MutableInt; @@ -119,7 +118,7 @@ public String toString() { public void step(int leftSize, int rightSize, QueryTable leftTable, QueryTable rightTable, ColumnInfo[] leftColumnInfo, ColumnInfo[] rightColumnInfo, EvalNuggetInterface[] en, Random random) { - final PeriodicUpdateGraph updateGraph = ExecutionContext.getContext().getUpdateGraph().cast(); + final ControlledUpdateGraph updateGraph = ExecutionContext.getContext().getUpdateGraph().cast(); updateGraph.runWithinUnitTestCycle(() -> { GenerateTableUpdates.generateShiftAwareTableUpdates(GenerateTableUpdates.DEFAULT_PROFILE, leftSize, random, leftTable, leftColumnInfo); diff --git a/engine/updategraph/src/main/java/io/deephaven/engine/updategraph/OperationInitializer.java b/engine/updategraph/src/main/java/io/deephaven/engine/updategraph/OperationInitializer.java index 1adf8e8d116..84317389a9f 100644 --- a/engine/updategraph/src/main/java/io/deephaven/engine/updategraph/OperationInitializer.java +++ b/engine/updategraph/src/main/java/io/deephaven/engine/updategraph/OperationInitializer.java @@ -21,12 +21,7 @@ public Future submit(Runnable runnable) { @Override public int parallelismFactor() { - return 0; - } - - @Override - public void start() { - // no-op + return 1; } }; @@ -48,9 +43,4 @@ public void start() { * @return */ int parallelismFactor(); - - /** - * - */ - void start(); }