Skip to content

Commit

Permalink
Disable visualizations for subexpressions (#11949)
Browse files Browse the repository at this point in the history
The possibility of attaching visualizations to subexpressions meant that we (currently) have to invalidate caches for their parent expression every time a request comes. That was an acceptable cost when an expression is relatively fast to compute but unacceptable when dealing with slow computations that would have to be repeated.

Since currently attaching visualizations is not used at all and we can rely on caching RHS and self, it is _safe_ to disable it. An observable pattern is better suited for visualizations and would mitigate this problem by design, something that we planned for a while in #10525

Should help with long running visualizations in #11882. Partial visualization results should be addressed on GUI side.

# Important Notes
For the example in #11882 we would at least re-read the large file at least twice, which adds 40-60seconds to the overall startup.

Exchanges before the change:
![Screenshot from 2024-12-27 16-52-46](https://github.com/user-attachments/assets/63e7a6db-73f8-48dd-a24a-a44e197e4ee6)

Responses after the change:
![Screenshot from 2024-12-30 12-18-28](https://github.com/user-attachments/assets/08020b1c-58f0-4c0f-b06d-1d904373b946)

Results in the same (final) data:
![Screenshot from 2024-12-30 12-24-02](https://github.com/user-attachments/assets/280f6ef5-6691-4744-b67a-2a0898f55b8b)
  • Loading branch information
hubertp authored Jan 2, 2025
1 parent bc6a8aa commit 1f763f7
Show file tree
Hide file tree
Showing 5 changed files with 12 additions and 5 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ final class JobExecutionEngine(
assertInJvm(timeSinceRequestedToCancel > 0)
val timeToCancel =
forceInterruptTimeout - timeSinceRequestedToCancel
assertInJvm(timeToCancel > 0)
logger.log(
Level.FINEST,
"About to wait {}ms to cancel job {}",
Expand All @@ -107,7 +108,8 @@ final class JobExecutionEngine(
runningJob.future.get(timeToCancel, TimeUnit.MILLISECONDS)
logger.log(
Level.FINEST,
"Job {} finished within the allocated soft-cancel time"
"Job {} finished within the allocated soft-cancel time",
runningJob.id
)
} catch {
case _: TimeoutException =>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -484,6 +484,8 @@ class EnsureCompiledJob(
)
invalidatedVisualizations.foreach { visualization =>
UpsertVisualizationJob.upsertVisualization(visualization)
// Cache invalidation disabled because of #11882
// ctx.state.executionHooks.add(InvalidateCaches(visualization.expressionId))
}
if (invalidatedVisualizations.nonEmpty) {
ctx.executionService.getLogger.log(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -94,9 +94,9 @@ object ProgramExecutionSupport {

val onComputedValueCallback: Consumer[ExpressionValue] = { value =>
if (callStack.isEmpty) {
logger.log(Level.FINEST, s"ON_COMPUTED ${value.getExpressionId}")

if (VisualizationResult.isInterruptedException(value.getValue)) {
logger.log(Level.FINEST, s"ON_INTERRUPTED ${value.getExpressionId}")
value.getValue match {
case e: AbstractTruffleException =>
sendInterruptedExpressionUpdate(
Expand All @@ -110,6 +110,7 @@ object ProgramExecutionSupport {
case _ =>
}
}
logger.log(Level.FINEST, s"ON_COMPUTED ${value.getExpressionId}")
sendExpressionUpdate(contextId, executionFrame.syncState, value)
sendVisualizationUpdates(
contextId,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,8 @@ class UpsertVisualizationJob(
)
None
case None =>
// Caching disabled due to #11882
// ctx.state.executionHooks.add(InvalidateCaches(expressionId))
Some(Executable(config.executionContextId, stack))
}
}
Expand Down Expand Up @@ -160,7 +162,7 @@ class UpsertVisualizationJob(
object UpsertVisualizationJob {

/** Invalidate caches for a particular expression id. */
sealed private case class InvalidateCaches(
sealed case class InvalidateCaches(
expressionId: Api.ExpressionId
)(implicit ctx: RuntimeContext)
extends Runnable {
Expand Down Expand Up @@ -511,7 +513,6 @@ object UpsertVisualizationJob {
arguments
)
setCacheWeights(visualization)
ctx.state.executionHooks.add(InvalidateCaches(expressionId))
ctx.contextManager.upsertVisualization(
visualizationConfig.executionContextId,
visualization
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4419,7 +4419,8 @@ class RuntimeVisualizationsTest extends AnyFlatSpec with Matchers {
new String(data1, StandardCharsets.UTF_8) shouldEqual "C"
}

it should "emit visualization update for the target of a method call (subexpression) with IdMap" in withContext() {
// Attaching visualizations to subexpressions is currently disabled, see #11882
ignore should "emit visualization update for the target of a method call (subexpression) with IdMap" in withContext() {
context =>
val contextId = UUID.randomUUID()
val requestId = UUID.randomUUID()
Expand Down

0 comments on commit 1f763f7

Please sign in to comment.