From 2b35f979922ef00cc13fa8615ae5ac0956ba4b36 Mon Sep 17 00:00:00 2001 From: Hubert Plociniczak Date: Mon, 30 Dec 2024 11:48:49 +0100 Subject: [PATCH] Disable visualizations for subexpressions 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. --- .../instrument/execution/JobExecutionEngine.scala | 4 +++- .../enso/interpreter/instrument/job/EnsureCompiledJob.scala | 2 ++ .../interpreter/instrument/job/ProgramExecutionSupport.scala | 3 ++- .../interpreter/instrument/job/UpsertVisualizationJob.scala | 5 +++-- .../test/instrument/RuntimeVisualizationsTest.scala | 3 ++- 5 files changed, 12 insertions(+), 5 deletions(-) diff --git a/engine/runtime-instrument-common/src/main/scala/org/enso/interpreter/instrument/execution/JobExecutionEngine.scala b/engine/runtime-instrument-common/src/main/scala/org/enso/interpreter/instrument/execution/JobExecutionEngine.scala index d5b528a11ee8..a375e5fc62f3 100644 --- a/engine/runtime-instrument-common/src/main/scala/org/enso/interpreter/instrument/execution/JobExecutionEngine.scala +++ b/engine/runtime-instrument-common/src/main/scala/org/enso/interpreter/instrument/execution/JobExecutionEngine.scala @@ -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 {}", @@ -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 => diff --git a/engine/runtime-instrument-common/src/main/scala/org/enso/interpreter/instrument/job/EnsureCompiledJob.scala b/engine/runtime-instrument-common/src/main/scala/org/enso/interpreter/instrument/job/EnsureCompiledJob.scala index 7cbcb769e856..ad3486452306 100644 --- a/engine/runtime-instrument-common/src/main/scala/org/enso/interpreter/instrument/job/EnsureCompiledJob.scala +++ b/engine/runtime-instrument-common/src/main/scala/org/enso/interpreter/instrument/job/EnsureCompiledJob.scala @@ -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( diff --git a/engine/runtime-instrument-common/src/main/scala/org/enso/interpreter/instrument/job/ProgramExecutionSupport.scala b/engine/runtime-instrument-common/src/main/scala/org/enso/interpreter/instrument/job/ProgramExecutionSupport.scala index 53d89c8eaa7d..a59b6ebae530 100644 --- a/engine/runtime-instrument-common/src/main/scala/org/enso/interpreter/instrument/job/ProgramExecutionSupport.scala +++ b/engine/runtime-instrument-common/src/main/scala/org/enso/interpreter/instrument/job/ProgramExecutionSupport.scala @@ -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( @@ -110,6 +110,7 @@ object ProgramExecutionSupport { case _ => } } + logger.log(Level.FINEST, s"ON_COMPUTED ${value.getExpressionId}") sendExpressionUpdate(contextId, executionFrame.syncState, value) sendVisualizationUpdates( contextId, diff --git a/engine/runtime-instrument-common/src/main/scala/org/enso/interpreter/instrument/job/UpsertVisualizationJob.scala b/engine/runtime-instrument-common/src/main/scala/org/enso/interpreter/instrument/job/UpsertVisualizationJob.scala index 00045611a94d..e8da273a04b3 100644 --- a/engine/runtime-instrument-common/src/main/scala/org/enso/interpreter/instrument/job/UpsertVisualizationJob.scala +++ b/engine/runtime-instrument-common/src/main/scala/org/enso/interpreter/instrument/job/UpsertVisualizationJob.scala @@ -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)) } } @@ -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 { @@ -511,7 +513,6 @@ object UpsertVisualizationJob { arguments ) setCacheWeights(visualization) - ctx.state.executionHooks.add(InvalidateCaches(expressionId)) ctx.contextManager.upsertVisualization( visualizationConfig.executionContextId, visualization diff --git a/engine/runtime-integration-tests/src/test/scala/org/enso/interpreter/test/instrument/RuntimeVisualizationsTest.scala b/engine/runtime-integration-tests/src/test/scala/org/enso/interpreter/test/instrument/RuntimeVisualizationsTest.scala index 1e8db46f8890..c164a536f09b 100644 --- a/engine/runtime-integration-tests/src/test/scala/org/enso/interpreter/test/instrument/RuntimeVisualizationsTest.scala +++ b/engine/runtime-integration-tests/src/test/scala/org/enso/interpreter/test/instrument/RuntimeVisualizationsTest.scala @@ -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()