WIP: Replacing Static Dataflow Analysis with Reactive Caches #13948
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
Initial implementation that replaces cache invalidation logic and static dataflow analysis with Reactive caches. A lot of code is commented out, as either it needs to be re-done or is simply obsolete in the new implementation. Depends on #13907.
10 tasks
hubertp
added
the
CI: Clean build required
The remaining problems relate to the fact that we are caching all expressions. That is conflict with the current `enterables` logic for entering functions from a local call stack. The latter will need a more involving rewrite.
engine/runtime-instrument-common/src/main/java/org/enso/interpreter/instrument/Observable.java
Outdated
Show resolved
Hide resolved
engine/runtime-compiler/src/main/java/org/enso/compiler/pass/analyse/FramePointer.java
Outdated
Show resolved
Hide resolved
engine/polyglot-api/src/main/java/org/enso/polyglot/debugger/IdExecutionService.java
Outdated
Show resolved
Hide resolved
5 tasks
In order to be able to track dependencies between nodes, also in the event when no external `UUID` is available, had to allow to have two types of Observables. Similarly, `UUID`s that are external and internal are now easily distinguishable thanks to `RuntimeID` wrapper. Currently some tracking logic is inside nodes themselves. That's obviously wrong - it should all be in the instrumentation. Follow up change will move the logic. The current approach is mostly for achieving a minimal viable proof that the approach will work.
Mostly cosmetic changes due to the fact that we now invalidate less. But two prominent changes are included: - invalidation logic when Recompute requests comes - Fixed tracking method calls/closure execution The latter uses enter/exit logic in `RuntimeAnalysis` in `InvokeCallableNode` which is correct but undeseriable. The information should be gathered in instrumention instead. Explicit calls are easier to experiment with and will be transfered to instrumentation eventually.
Had to ensure that dependencies are being tracked correctly across function call boundaries. This is problematic due to the fact that local calls are being done with their own RuntimeCaches; any invalidation has to ensure that it is capable of crossing that boundary. Additionally, had to ensure that self argument is properly tracked/invalidated or changes withing the functions would not be translated into Truffle nodes. The logic is still experiemental and elements of RuntimeAnalysis will need to move to instrumentation, for performance reasons.
The exception was being propagated, unintentionally it seems, and showing up in logs.
Spurious method/self arg invalidation leading to large and unnecessary re-compilations.
Unresolved constructors are being resolved in synthetic constructs. Sadly, those constructs inside closures, share original IDs and therefore create false dependencies between nodes. As we still need the original IDs for instrumentation and expression updates, introduced a flag that disables runtime tracking for expressions.
Not yet complete but already found a few corner cases not covered in the new design.
Extra diagnostics were lost when computation failed at an early CompletionStage.
Panics aren't cached but visualizations can still be run on them. Had to add another method to Observable to make it happen.
Have to keep track of arguments applied to the visualization and map them to synthetic assignnment constructs in visualization code to get the right identifiers.
If a new external identifier is added within an expression that is already cached, the latter should be invalidated. Fixes a number of broken visualization tests.
When a text edit is performed on a code that is being called in the visualization, a full re-evaluation is needed in order to generate updated Truffle nodes. Decided not to support that case, as it is very unlikely to be needed in the foresable future. Text change will invalidate necessary observers, as one would expect, but it will not trigger re-evaluation. Instead, one has to explicitly make `ModifyVisualization` request. This seems like a good compromise as evaluations of visualization expressions are very costly (also due to locking).
Workarounds for Stackoverflow issues + IdMap updates causing full program invalidations. This change breaks 1 test in RuntimeVisualizationTests that will be addressed separately.
With runtime tracking we are able to precisely discover which cached values need to be invalidated. Unfortunately excessive use of external UUIDs (and therefore caching) by GUI revealed a flaw in that logic wrt to local variable access. Whenever a local variable is being accessed it is reading a previously written value from a frame. But if the assignment has been cached, then a specific slot in the frame is not written during successive execution. This means that adding new expressions that make use of previously cached local variables would report uninitialized value errors. This change workarounds this by **never** caching assignments. Assignments should be fast if the RHS is already cached, and GUI appears to assign external UUIDS to RHS always, it seems. That way any reading from a frame's slot will succeed. Also reverted to the old tree building in Changeset as it appeared to be sufficient for our needs.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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
Initial implementation that replaces cache invalidation logic and static dataflow analysis with Reactive caches.
A lot of code is commented out, as either it needs to be re-done or is simply obsolete in the new implementation.
There will be still a lot of changes but basic functionality of loading/executing projects and executing visualizations works.
Invalidation of only the necessary nodes works, by replacing static dataflow analysis with runtime analysis that collects dependencies between UUIDs.
Depends on #13907. Addresses #10525, renders #13219 obsolete.
Important Notes
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.
or the Snowflake database integration, a run of the Extra Tests has been scheduled.