Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Node added inside collapsed node is never executed #11341

Conversation

4e6
Copy link
Contributor

@4e6 4e6 commented Oct 16, 2024

Pull Request Description

close #11251

Changelog:

  • add: CachePreferences configuration of the expressions that are marked for caching
  • update: Invalidate the self keywords in the parent frames on function edit
  • update: persistance config

Important Notes

The demo shows that:

  1. The new node created in the collapsed function is highlighted (GUI receives the expression update)
  2. When the collapsed function is edited, the nodes in the main method are updated correctly
enso-11251-collapsed.mp4

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    TypeScript,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • Unit tests have been written where possible.

@4e6 4e6 added the CI: No changelog needed Do not require a changelog entry for this PR. label Oct 16, 2024
@4e6 4e6 self-assigned this Oct 16, 2024
@4e6 4e6 marked this pull request as ready for review October 17, 2024 16:19
|main =
| x = Main.inc 3
| y = Main.inc 7
| IO.println x+y
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this example was not working before this PR?

Why it is working now?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was not working because the self argument was not invalidated after the method was edited. With the cached self we observed some stale nodes during the runtime, and the function call may return incorrect results (the result before the edit was done). Why it was the behavior is another question and I was not able to find the answer.

* @param kind the node kind
*/
public void set(UUID id, Kind kind) {
preferences.put(id, kind);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am afraid of a data structure read from persistance that can be modified. At certain moment such a structure must become immutable. We cannot have a mutable structures flying around in the system. There are already plenty of such structures and we don't want to introduce new ones.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am afraid of a data structure read from persistance that can be modified.

From what I can see, all the compiler metadata is mutable currently. I introduced the CachePreferences to avoid copying and simply pass it to runtime after the compilation.

If we want to introduce an immutable object for persistence, we would need to convert mutable prefs to immutable to pass it to the persistence. And then convert immutable prefs to mutable again when we pass it to the runtime.

Copy link
Collaborator

@hubertp hubertp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -403,6 +403,12 @@ class EnsureCompiledJob(
CacheInvalidation.StackSelector.All,
invalidateStaleCommand,
Set(CacheInvalidation.IndexSelector.All)
),
CacheInvalidation(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So that's the crucial point of this PR, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

@@ -177,13 +166,13 @@ case object CachePreferenceAnalysis extends IRPass {
callArgument: CallArgument,
weights: WeightInfo
): CallArgument = {
callArgument.value.getExternalId.foreach(weights.update(_, Weight.Always))
callArgument.value.getExternalId
.foreach(weights.update(_, CachePreferences.Kind.SELF_ARGUMENT))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@4e6 4e6 added the CI: Ready to merge This PR is eligible for automatic merge label Oct 18, 2024
@mergify mergify bot merged commit 7522acf into develop Oct 18, 2024
41 of 42 checks passed
@mergify mergify bot deleted the wip/db/11251-node-added-inside-collapsed-node-is-never-executed branch October 18, 2024 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: No changelog needed Do not require a changelog entry for this PR. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Node added inside collapsed node is never executed.
3 participants