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.

Sign up for GitHub

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

Jump to bottom

Encapsulating State access by moving nodes to runtime.state package #12314

Merged
merged 15 commits into from
Feb 22, 2025
Merged

Encapsulating State access by moving nodes to runtime.state package #12314

merged 15 commits into from
Feb 22, 2025

Conversation

JaroslavTulach
Copy link
Member

@JaroslavTulach JaroslavTulach commented Feb 18, 2025 •
edited
Loading

Pull Request Description

Refactoring to fix #12294.

Checklist

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

  • All code follows the
    Scala,
    Java,
  • Unit tests have been written where possible.
  • Engine benchmark run seems fine

@JaroslavTulach JaroslavTulach self-assigned this Feb 18, 2025
@enso-bot enso-bot bot mentioned this pull request Feb 19, 2025
Closed
@JaroslavTulach JaroslavTulach added the CI: No changelog needed Do not require a changelog entry for this PR. label Feb 19, 2025
jdunkerley
jdunkerley approved these changes Feb 19, 2025
View reviewed changes
Copy link
Member

@jdunkerley jdunkerley left a comment

Choose a reason for hiding this comment

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

Unblocking from libs side.

@JaroslavTulach JaroslavTulach force-pushed the wip/jtulach/StateNodes12294 branch from 5c4a006 to d379995 Compare February 19, 2025 15:16
Akirathan
Akirathan approved these changes Feb 21, 2025
View reviewed changes
Copy link
Member

@Akirathan Akirathan left a comment

Choose a reason for hiding this comment

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

I see a lot of changes like https://github.com/enso-org/enso/pull/12314/files#diff-ce5f7521078c4c4b93e626e91c74c4fd045073780af555def6cada79203eb58cR692, that is, instead of passing context.emptyState(), we are passing context.currentState(). That seems fine. But is it tested anywhere?

engine/runtime/src/main/java/org/enso/interpreter/runtime/EnsoContext.java
@@ -108,7 +109,9 @@ public final class EnsoContext {
private final LockManager lockManager;
private final AtomicLong clock = new AtomicLong();

private final Shape rootStateShape = Shape.newBuilder().layout(State.Container.class).build();
@CompilationFinal(dimensions = 1)
private Object[] extraValues = new Object[0];
Copy link
Member

Choose a reason for hiding this comment

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

Apart for rootStateShape, isn't there anything else that we can move into this extraValues?

Copy link
Member Author

Choose a reason for hiding this comment

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

LockManager, NotificationHandler, DistributionManager, ResourceManager, etc. - all of this stuff can be moved out of EnsoContext now with the help of extraValues. E.g. the EnsoManager doesn't need to be a hub that references everything during javac compilation. All these extra services can be loosely connected to it with the help of this concept.

Thanks for noticing and I encourage everyone to use this extra values concept in subsequent PRs!

@Akirathan
Copy link
Member

#12326 is soon to be merged. So I am replacing -libs-API-change label with the more specific -libs-API-change-Base label.

@Akirathan Akirathan added -libs-API-change-Base Marks a PR that changes the public API of Standard.Base and removed -libs-API-change labels Feb 21, 2025
@JaroslavTulach
Copy link
Member Author

I see a lot of changes ... context.emptyState()... passing context.currentState(). That seems fine.

We don't have emptyState() anymore. The state is filled and cleaned as the execution progresses by RunStateNode.

But is it tested anywhere?

This is the first and only test related to that I know about. I guess we add more if something gets broken.

@JaroslavTulach JaroslavTulach added the CI: Ready to merge This PR is eligible for automatic merge label Feb 22, 2025
@mergify mergify bot merged commit 6709d47 into develop Feb 22, 2025
73 checks passed
@mergify mergify bot deleted the wip/jtulach/StateNodes12294 branch February 22, 2025 05:16
@JaroslavTulach JaroslavTulach mentioned this pull request Feb 26, 2025
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@4e6 4e6 4e6 left review comments

@jdunkerley jdunkerley jdunkerley approved these changes

@Akirathan Akirathan Akirathan approved these changes

@GregoryTravis GregoryTravis GregoryTravis approved these changes

@hubertp hubertp Awaiting requested review from hubertp hubertp is a code owner

@radeusgd radeusgd Awaiting requested review from radeusgd radeusgd is a code owner

@AdRiley AdRiley Awaiting requested review from AdRiley AdRiley is a code owner

@marthasharkey marthasharkey Awaiting requested review from marthasharkey marthasharkey is a code owner

Assignees

@JaroslavTulach JaroslavTulach

Labels
-libs-API-change-Base Marks a PR that changes the public API of Standard.Base 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Further rework State manipulation Nodes
5 participants
@JaroslavTulach @Akirathan @4e6 @jdunkerley @GregoryTravis