Skip to content

Conversation

@plummercj
Copy link
Contributor

@plummercj plummercj commented Mar 21, 2023

There are two GC related issues with this test that are being addressed. The test was limiting the heap size to 6m so if there is still a leak, it will be detected quickly. This proved to be too small of a size when using ZGC. For the most part changing the size to 7m fixed this issue. However, I was still seeing frequent issues with ZGC on macOS. This is explained by JDK-8304449, which noticed (rarely) OOME on macos even when not using ZGC. From JDK-8304449:

"macOS has a thread behavior that is not seen on linux and windows that is causing more memory usage, which sometimes leads to this unexpected OOME. The debuggee side of the test constantly creates threads that do little more than a short sleep. It has a counter of "live" threads, and won't let that go over 500. On the debugger side it is just tracking ThreadStartEvents and ThreadDeathEvents. It keep tracks of threads (ThreadReferences) for which a ThreadStartEvent had been received but a ThreadDeathEvent has not. On linux and windows the count of outstanding threads is generally in the 200-400 range, sometimes briefly going over 500. However, on macOS it is closer to 2400. This means a lot more ThreadReferences being tracked, which means more memory usage, so sometimes you see an OOME on macOS as a result. "

The threads collection mainly existed just so its size could be used to log the number of outstanding ThreadDeathEvents. I got rid of the threads collection and instead am just tracking the number of ThreadStartEvents and ThreadDeathEvents, and computing the difference to get the number of outstanding ThreadDeathEvents.


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

Issues

  • JDK-8304436: com/sun/jdi/ThreadMemoryLeakTest.java fails with "OutOfMemoryError: Java heap space" with ZGC
  • JDK-8304449: com/sun/jdi/ThreadMemoryLeakTest.java times out

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/13130/head:pull/13130
$ git checkout pull/13130

Update a local copy of the PR:
$ git checkout pull/13130
$ git pull https://git.openjdk.org/jdk.git pull/13130/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 13130

View PR using the GUI difftool:
$ git pr show -t 13130

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/13130.diff

@plummercj plummercj closed this Mar 21, 2023
@plummercj plummercj reopened this Mar 21, 2023
@plummercj
Copy link
Contributor Author

/issue add JDK-8304449

@bridgekeeper
Copy link

bridgekeeper bot commented Mar 21, 2023

👋 Welcome back cjplummer! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot changed the title 8304436 8304436: com/sun/jdi/ThreadMemoryLeakTest.java fails with "OutOfMemoryError: Java heap space" with ZGC Mar 21, 2023
@openjdk openjdk bot added the rfr Pull request is ready for review label Mar 21, 2023
@openjdk
Copy link

openjdk bot commented Mar 21, 2023

@plummercj
Adding additional issue to issue list: 8304449: com/sun/jdi/ThreadMemoryLeakTest.java times out.

@openjdk
Copy link

openjdk bot commented Mar 21, 2023

@plummercj The following label will be automatically applied to this pull request:

  • serviceability

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the serviceability serviceability-dev@openjdk.org label Mar 21, 2023
@mlbridge
Copy link

mlbridge bot commented Mar 21, 2023

Webrevs

@openjdk
Copy link

openjdk bot commented Mar 27, 2023

@plummercj This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8304436: com/sun/jdi/ThreadMemoryLeakTest.java fails with "OutOfMemoryError: Java heap space" with ZGC
8304449: com/sun/jdi/ThreadMemoryLeakTest.java times out

Reviewed-by: lmesnik, dcubed

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 103 new commits pushed to the master branch:

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Mar 27, 2023
@plummercj
Copy link
Contributor Author

Can I get one more review please? Thanks!

Copy link
Member

@dcubed-ojdk dcubed-ojdk left a comment

Choose a reason for hiding this comment

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

Thumbs up. Thanks for the explanations in the PR and in the
bug reports.

What kind of testing has been done on this fix?

@plummercj
Copy link
Contributor Author

What kind of testing has been done on this fix?

I ran this tests 25 times on each supported platform, both with and with ZGC.

@dcubed-ojdk
Copy link
Member

Was the com/sun/jdi/ThreadMemoryLeakTest.java test executed by itself in those
runs or did you run the entire test task or at least com/sun/jdi so that parallelism
was also a factor?

@plummercj
Copy link
Contributor Author

Just the one test.

@dcubed-ojdk
Copy link
Member

Okay. Let's go with it and I'll keep an eye on the CI (like usual)...

@plummercj
Copy link
Contributor Author

/integrate

@openjdk
Copy link

openjdk bot commented Mar 29, 2023

Going to push as commit 9643f65.
Since your change was applied there have been 103 commits pushed to the master branch:

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Mar 29, 2023
@openjdk openjdk bot closed this Mar 29, 2023
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Mar 29, 2023
@openjdk
Copy link

openjdk bot commented Mar 29, 2023

@plummercj Pushed as commit 9643f65.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

integrated Pull request has been integrated serviceability serviceability-dev@openjdk.org

Development

Successfully merging this pull request may close these issues.

3 participants