Refactorings in preparation for parallel visualization execution #13354
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reduce lock contention by splitting context lock into a read/write one. Context lock has been slowly ignored in various jobs in favour of improved performance. It shouldn't be, as most of the time we only need a read context lock.
By default, no execution is triggered by push context request.
Most of `ExecutionService` now returns Futures that can be easier to compose and execute via threadpools.
When diagnostics are being gathered outside of ThreadManager, execution will blow up due to Truffle restrictions.
Should reduce lock contention on startup.
hubertp
requested review from
jdunkerley,
radeusgd,
GregoryTravis,
AdRiley,
4e6,
JaroslavTulach and
Akirathan
as code owners
hubertp
added
the
CI: No changelog needed
GregoryTravis
approved these changes
Jun 24, 2025
When reporting error diagnostics we need to analyze the error underlying the `ExecutionException`, not the exception itself.
It now says 427, so adding +30 as a general rule. Dunno on why would increase be needed.
JaroslavTulach
approved these changes
Jun 26, 2025
| } | ||
| return false; |
There was a problem hiding this comment.
Nit: the previous version with two return statement in each if branch felt more functional to me.
...e/runtime-instrument-common/src/main/java/org/enso/interpreter/service/ExecutionService.java
Show resolved
Hide resolved
| return context.getThreadManager().submit(c); | ||
| } | ||
|
|
||
| private static <T> T resultOf(Future<T> future) { |
There was a problem hiding this comment.
Removal of resultOf blocking call marks ExecutionService as fully reactive (e.g. non-blocking) API. Good.
| ) | ||
| ) | ||
|
|
||
| visualizationResultFuture.thenApply(visualizationResult => { |
...ument-common/src/main/scala/org/enso/interpreter/instrument/execution/ReentrantLocking.scala
Show resolved
Hide resolved
...ment-common/src/main/scala/org/enso/interpreter/instrument/job/ProgramExecutionSupport.scala
Show resolved
Hide resolved
| public String getExceptionMessage(AbstractTruffleException panic) { | ||
| var future = submitExecution(() -> computeExceptionMessage(panic)); | ||
| return resultOf(future); | ||
| public CompletionStage<String> getExceptionMessage(AbstractTruffleException panic) { |
There was a problem hiding this comment.
This one would very likely work better as:
CompletionStage<String> getExceptionMessage(CompletionStage<AbstractTruffleException> panic)or something a like that being part of the reactive flow. It makes little sense to ex = toCompletableFuture().get() and then enter again the guest code thread by getExceptionMessage(ex).
The same comment applies to typeOfValue and other methods that inspect result of guest code execution.
|
I will address the remaining comments in follow up PRs. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull Request Description
Work on reactive observers for visualizations revealed a number of blockers once one attempts to execute visualizations in parallel. This PR extracts the less controversial parts that became obvious once we opened a pandora's box, as well as a number of startup improvements:
IdMapnecessary for instrumentation at that stage)ExecutionServiceto not blockHostClassLoaderthread-safeImportant Notes
Should be easier to review than #13219 that went through a number of experiments.
Some startup improvements should be visible from this change already as we reduce lock contention.
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,
TypeScript,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.