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

Cleanup after each test run #8418

Merged
merged 4 commits into from
Nov 29, 2023
Merged

Cleanup after each test run #8418

merged 4 commits into from
Nov 29, 2023

Conversation

hubertp
Copy link
Collaborator

@hubertp hubertp commented Nov 29, 2023

Pull Request Description

Reducing leaks when running our test suite.

Potentially fixes #8408.

Important Notes

Managed to keep thread count and memory in between runs relatively stable.
Initially:
Screenshot from 2023-11-29 11-06-04
Now:
Screenshot from 2023-11-29 15-57-34

The screenshot illustrates for RuntimeVisualizationsTest only. Will need to be applied in other places as well.

Applying the same style to runtime-with-instruments.
Before:
Screenshot from 2023-11-29 16-55-11
After:
Screenshot from 2023-11-29 16-50-07

Checklist

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

  • 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,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.

WIP: reducing leaks when running our test suite.
@hubertp hubertp force-pushed the wip/hubert/8408-macos-fails branch from e53d95c to 2c78aec Compare November 29, 2023 11:41
@hubertp hubertp force-pushed the wip/hubert/8408-macos-fails branch from 2c78aec to a04d09b Compare November 29, 2023 11:44
Shutdown hook keeps a reference to `RuntimeServerEmulator` which
prevents things from being GC'ed.
@hubertp hubertp marked this pull request as ready for review November 29, 2023 15:01
@hubertp hubertp added the CI: No changelog needed Do not require a changelog entry for this PR. label Nov 29, 2023
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.

Congratulations on finding the actual culprit. The memory consumption now seems pretty reasonable as opposed to the previous state.

@hubertp
Copy link
Collaborator Author

hubertp commented Nov 29, 2023

Running runtime/test is apparently also suffering from similar issues:
Screenshot from 2023-11-29 16-39-16

Eliminating shutdown hooks so that objects get GC'ed.
@hubertp
Copy link
Collaborator Author

hubertp commented Nov 29, 2023

OK. Let's merge this as-is and I will have further improvements coming for runtime/test.
This is already significantly improving the build system.

@hubertp hubertp added the CI: Ready to merge This PR is eligible for automatic merge label Nov 29, 2023
Copy link
Member

@JaroslavTulach JaroslavTulach left a comment

Choose a reason for hiding this comment

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

I'd suggest to share the cleanup in a single method rather than having it copied dozen times.

context.executionContext.context.close()
context.runtimeServerEmulator.terminate()
if (context != null) {
context.reset()
Copy link
Member

Choose a reason for hiding this comment

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

This sequence seems to appear at many places. Shouldn't it be shared at a single place?

@mergify mergify bot merged commit b6bdf90 into develop Nov 29, 2023
34 checks passed
@mergify mergify bot deleted the wip/hubert/8408-macos-fails branch November 29, 2023 19:03
@@ -1,4 +1,6 @@
package org.enso.distribution.locking

/** A [[LockManager]] which guarantees to be thread-safe. */
trait ThreadSafeLockManager extends LockManager
trait ThreadSafeLockManager extends LockManager {
def reset(): Unit
Copy link
Member

Choose a reason for hiding this comment

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

I think we should have a comment saying what is the purpose of this function.

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.

MacOS engine tests fail on CI
4 participants