Disable visualizations for subexpressions #11949
Conversation
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.
hubertp
requested review from
4e6,
JaroslavTulach and
Akirathan
as code owners
hubertp
added
the
CI: No changelog needed
| @@ -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)) | |||
|
Is this change meant to be temporary? It possibly violates assumptions made by the GUI team, since we assumed that we can attach visualizations to any arbitrary expression AST. Though I'm not exactly sure that it would actually breaks anything in practice right now. Could you write down some rules what is allowed/expected to work, so we can reference that in the future? |
Yes, we will bring it back eventually. Right now it causes serious perf issues and we don't seem to use the arbitrary part so no reason to keep it around for the moment. |
mergify
bot
deleted the
wip/hubert/11882-disable-subexpressions-visualizations
branch
Pull Request Description
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:
Responses after the change:
Results in the same (final) data:
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.