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

More reliable shutdown of the EnsoContext to save resources #6468

Merged
merged 9 commits into from
May 2, 2023
Merged

More reliable shutdown of the EnsoContext to save resources #6468

merged 9 commits into from
May 2, 2023

Conversation

JaroslavTulach
Copy link
Member

@JaroslavTulach JaroslavTulach commented Apr 28, 2023 •
edited
Loading

Pull Request Description

There is 572 threads during RuntimeServerTest execution create. They stay around because the context isn't closed after each test finishes. Adding afterEach cleanup sequence to runtime-with-instruments tests. Improving robustness of overall EnsoContext shutdown.

Checklist

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

  • All code has been tested:
    • Unit tests are still working

@JaroslavTulach JaroslavTulach added the CI: No changelog needed Do not require a changelog entry for this PR. label Apr 28, 2023
@JaroslavTulach JaroslavTulach self-assigned this Apr 28, 2023
JaroslavTulach
JaroslavTulach commented Apr 28, 2023
View reviewed changes
...with-instruments/src/test/scala/org/enso/interpreter/test/instrument/RuntimeServerTest.scala
@@ -331,6 +331,10 @@ class RuntimeServerTest
val Some(Api.Response(_, Api.InitializedNotification())) = context.receive
}

override protected def afterEach(): Unit = {
context.executionContext.context.close()
Copy link
Member Author

Choose a reason for hiding this comment

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

This helps the "engine threads", but the test should also stop Akka, @4e6. Dmitry, if you know how to do that feel free to directly commit to this branch.

Copy link
Contributor

Choose a reason for hiding this comment

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

We're not using Akka in runtime* projects. Only in the language-server and project-manager

Copy link
Contributor

Choose a reason for hiding this comment

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

I was wrong, in fact, runtime tests are using Akka.

@4e6
Copy link
Contributor

4e6 commented Apr 28, 2023

@JaroslavTulach do you have any estimate of how much resources can occupy the test Context? I'm just curious.

@JaroslavTulach
Copy link
Member Author

JaroslavTulach commented Apr 28, 2023 •
edited
Loading

@JaroslavTulach do you have any estimate of how much resources can occupy the test Context? I'm just curious.

I am using VisualVM to Monitor the started test *RuntimeServerTest JVM. There used to be 572 threads at the end of RuntimeServerTest execution:
Original
now there is at most 39 threads:
After the fixes
each thread occupies at least 1MB because it pre-allocates a stack. That's at least 572MB of system resources. In adition to that the JVM is using 700MB of heap (that hasn't changed). E.g. we can save 1/3 of the RSS size of the process when running the new version.

@4e6
Copy link
Contributor

4e6 commented Apr 28, 2023

Not bad 🤔

JaroslavTulach
JaroslavTulach commented Apr 28, 2023
View reviewed changes
...ts/src/test/scala/org/enso/interpreter/test/instrument/RuntimeExecutionEnvironmentTest.scala
@@ -129,6 +129,11 @@ class RuntimeExecutionEnvironmentTest
val Some(Api.Response(_, Api.InitializedNotification())) = context.receive
}

override protected def afterEach(): Unit = {
context.executionContext.context.close()
Copy link
Member Author

Choose a reason for hiding this comment

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

When I added all these afterEach cleanups, running the whole sbt runtime-with-instruments/test started to randomly fail. Turned out I had to also f71557d - e.g. wait for the shutdown of the EnsoContext threads.

@JaroslavTulach JaroslavTulach changed the title Context.close() to save resources during test execution More reliable shutdown of the EnsoContext to save resources Apr 28, 2023
@enso-bot enso-bot bot mentioned this pull request Apr 28, 2023
Closed
@JaroslavTulach JaroslavTulach added the CI: Keep up to date Automatically update this PR to the latest develop. label May 2, 2023
@JaroslavTulach JaroslavTulach added the CI: Ready to merge This PR is eligible for automatic merge label May 2, 2023
@JaroslavTulach JaroslavTulach removed the CI: Keep up to date Automatically update this PR to the latest develop. label May 2, 2023
@mergify mergify bot merged commit c626543 into develop May 2, 2023
@mergify mergify bot deleted the wip/jtulach/CloseContext branch May 2, 2023 11:55
@enso-bot enso-bot bot mentioned this pull request May 3, 2023
Closed
Procrat added a commit that referenced this pull request May 3, 2023
@Procrat
Merge branch 'develop' into wip/procrat/json-viz-last-row-6196
b63966e 
* develop: (34 commits)
  Continued Execution Context work and some little fixes (#6506)
  IDE's logging to a file (#6478)
  Fix application config (#6513)
  Cloud/desktop mode switcher (#6448)
  Fix doubled named arguments bug (#6422)
  Reimplement `enso_project` as a proper builtin (#6352)
  Fix layer ordering between dropdown and breadcrumbs backgrounds.  (#6483)
  Multiflavor layers (#6477)
  DataflowAnalysis preserves dependencies order (#6493)
  Implement `create_database_table` for Database Table (#6467)
  Limit Dead Letter logging (#6482)
  More reliable shutdown of the EnsoContext to save resources (#6468)
  Make execution mode `live` default for CLI (#6496)
  Finishing Vector Editor (#6470)
  Proper handling of multiple list views. (#6461)
  Fix disappearing cached shapes (#6458)
  Add Execution Context control to Text.write (#6459)
  Change defaults for `Connection.tables` and ensure that `Connection.query` recognizes all available tables (#6443)
  Introducing @BuiltinMethod.needsFrame and InlineableNode (#6442)
  Hide profile button behind a feature flag (#6430)
  ...
Procrat added a commit that referenced this pull request May 4, 2023
@Procrat
Merge branch 'develop' into wip/procrat/home-button-6399
9a86a92 
* develop:
  Fix cut-off in text visualisations (#6421)
  Infer correct synthetic name for nested modules (#6525)
  Delete unused websocket dependency (#6535)
  Run typecheck and eslint on `./run lint` (#6314)
  Force pending saves if client closes abruptly (#6514)
  Continued Execution Context work and some little fixes (#6506)
  IDE's logging to a file (#6478)
  Fix application config (#6513)
  Cloud/desktop mode switcher (#6448)
  Fix doubled named arguments bug (#6422)
  Reimplement `enso_project` as a proper builtin (#6352)
  Fix layer ordering between dropdown and breadcrumbs backgrounds.  (#6483)
  Multiflavor layers (#6477)
  DataflowAnalysis preserves dependencies order (#6493)
  Implement `create_database_table` for Database Table (#6467)
  Limit Dead Letter logging (#6482)
  More reliable shutdown of the EnsoContext to save resources (#6468)
  Make execution mode `live` default for CLI (#6496)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hubertp hubertp hubertp approved these changes

@Akirathan Akirathan Akirathan approved these changes

@4e6 4e6 Awaiting requested review from 4e6 4e6 is a code owner

Assignees

@JaroslavTulach JaroslavTulach

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@JaroslavTulach @4e6 @hubertp @Akirathan