Make OperationInitializer non-static #4919
Conversation
niloc132
requested review from
chipkent,
jmao-denver and
rcaudy
as code owners
There was a problem hiding this comment.
Nightly build passing: https://github.com/niloc132/deephaven-core/actions/runs/7117005191/job/19376753910
engine/context/src/main/java/io/deephaven/engine/context/PoisonedOperationInitializer.java
Show resolved
Hide resolved
...ne/table/src/main/java/io/deephaven/engine/table/impl/OperationInitializationThreadPool.java
Outdated
Show resolved
Hide resolved
|
|
||
| @Override | ||
| public Future<?> submit(Runnable runnable) { | ||
| return executorService.submit(runnable); |
There was a problem hiding this comment.
This is consistent with existing behavior, but I wonder about checking canParallelize before doing this, since it can be false? In practice, all callers do that already, so it should be overkill, but might catch bugs, or just let a caller execute something directly without a canParallelize check.
There was a problem hiding this comment.
An assertion sounds reasonable; enforcing that we check encourages a code pattern that avoids creating the runnable.
There was a problem hiding this comment.
Assertion doesn't work it turns out - SelectColumnLayer appears to deliberately submit more work from on the OITP threads to that thread pool. Crucially it doesn't block on that work being returned, so this should be safe.
I'm going to defer other refactoring here to minimize risk of introducing bugs that I can't guard against like this.
engine/table/src/main/java/io/deephaven/engine/updategraph/impl/PeriodicUpdateGraph.java
Show resolved
Hide resolved
engine/test-utils/src/main/java/io/deephaven/engine/testutil/ControlledUpdateGraph.java
Outdated
Show resolved
Hide resolved
engine/test-utils/src/main/java/io/deephaven/engine/testutil/QueryTableTestBase.java
Outdated
Show resolved
Hide resolved
engine/updategraph/src/main/java/io/deephaven/engine/updategraph/OperationInitializer.java
Outdated
Show resolved
Hide resolved
engine/updategraph/src/main/java/io/deephaven/engine/updategraph/OperationInitializer.java
Outdated
Show resolved
Hide resolved
engine/context/src/main/java/io/deephaven/engine/context/ExecutionContext.java
Outdated
Show resolved
Hide resolved
engine/updategraph/src/main/java/io/deephaven/engine/updategraph/OperationInitializer.java
Outdated
Show resolved
Hide resolved
| @@ -83,20 +84,23 @@ private static void setContext(final ExecutionContext context) { | |||
| private final QueryScope queryScope; | |||
| private final QueryCompiler queryCompiler; | |||
| private final UpdateGraph updateGraph; | |||
| private final OperationInitializer operationInitializer; | |||
There was a problem hiding this comment.
Note to other reviewers:
I think this is a reasonable thing to add to the execution context, and that it's the right interface to add. I'd welcome consensus-building here.
engine/context/src/main/java/io/deephaven/engine/context/ExecutionContext.java
Outdated
Show resolved
Hide resolved
| /** | ||
| * @return Whether the current thread can parallelize operations using this OperationInitialization. | ||
| */ | ||
| boolean canParallelize(); |
There was a problem hiding this comment.
Can we eliminate this method by requiring parallel operations to install a "same thread" initializer to prevent nest parallelism?
There was a problem hiding this comment.
As above with the SelectColumnLayer, this doesnt look feasible at this time.
|
|
engine/table/src/main/java/io/deephaven/engine/table/impl/partitioned/PartitionedTableImpl.java
Show resolved
Hide resolved
|
Labels indicate documentation is required. Issues for documentation have been opened: How-to: https://github.com/deephaven/deephaven.io/issues/3545 |
This patch makes the OperationInitializer part of the ExecutionContext, allowing consumers to specify a preferred instance. ThreadInitializationFactory is also changed to be built in Dagger instead of only by configuration. Combined, these changes make it easier to reason about startup, requiring that dependencies are declared explicitly rather than implicitly referenced and used.
It is likely that some of the work done here should be refactored further into some JobScheduler factory or assisted injection, but OperationInitializer is still referenced directly by InitialFilterExecution, so this would require more refactoring.
Ideally, OperationInitializer.NON_PARALLELIZABLE would be used instead of the OperationInitializerThreadPool's threadlocal, but some services restore the user's own exec context - this is a shortcoming of the patch, and possibly should be resolved before merging.
Downstream users will need to consider if they want to capture the OperationInitializer when they create new exec contexts - generally this will be desired.
Partial #4040