8351002: com/sun/management/OperatingSystemMXBean cpuLoad tests fail intermittently

[kevinjwalls]

These tests have always silently permitted a -1 return value from OperatingSystemMXBean CPU time methods.

They need to be stricter, but occasionally Windows 2019 returns a -1 for the first few calls of these methods. This seems to be a Windows 2019 bug or peculiarity. Other Windows versions are not affected.

GetProcessCpuLoad.java and GetSystemCpuLoad.java need to fail only if the CPU time calls continually return -1. They should permit -1 values, as long as subsequently a value in the valid range is read.

The GetProcessCpuTime test also needs to retry enough times to expect no -1 values, and not just skip. While updating this test: it has a maximum expected value of Long.MAX_VALUE, which it may as well reduce to something that does not look like a binary "all ones except for the high bit" value (without creating an ongoing game where we keep increasing the value to avoid failures in slow runs).

---

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

Issue

• JDK-8351002: com/sun/management/OperatingSystemMXBean cpuLoad tests fail intermittently (Bug - P4)

Reviewers

• Leonid Mesnik (@lmesnik - Reviewer) Review applies to 453faf4d
• Serguei Spitsyn (@sspitsyn - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 24186

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back kevinw! 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]

@kevinjwalls 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:

8351002: com/sun/management/OperatingSystemMXBean cpuLoad tests fail intermittently

Reviewed-by: sspitsyn, lmesnik

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 42 new commits pushed to the master branch:

• dbc620f: 8352299: GenShen: Young cycles that interrupt old cycles cannot be cancelled
• f5a0db4: 8315447: Invalid Type Annotation attached to a method instead of a lambda
• 60544a1: 8164714: Constructor.newInstance creates instance of inner class with null outer class
• c856b34: 8352587: C2 SuperWord: we must avoid Multiversioning for PeelMainPost loops
• 993eae4: 8346948: Update CLDR to Version 47.0
• e98838f: 8352065: [PPC64] C2: Implement PopCountVL, CountLeadingZerosV and CountTrailingZerosV nodes
• 03105fc: 8351601: [JMH] test UnixSocketChannelReadWrite failed for 2 threads config
• fe03e2e: 8351897: Extra closing curly brace typos in Javadoc
• fa0b18b: 8352509: Update jdk.test.lib.SecurityTools jar method to accept List parameter
• 3ac9678: 8351224: Deprecate com.sun.tools.attach.AttachPermission for removal
• ... and 32 more: https://git.openjdk.org/jdk/compare/a8757332667df3fe41a29a7eedb2a7234d23c2a0...master

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]

@kevinjwalls The following labels will be automatically applied to this pull request:

• jmx
• serviceability

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

[mlbridge]

Webrevs

• 03: Full - Incremental (c420e213)
• 02: Full - Incremental (453faf4d)
• 01: Full - Incremental (2cfd63ce)
• 00: Full (21ebab5b)

[lmesnik]

Generally, changes looks good for me. Please address @plummercj comments.

[lmesnik]

The only one generic question: doesn't it makes sense to update JMX to retry several times if Windows 2029 returns -1.0 or undefined to make JDK more reliable?
Are tools like JMC/VisualVM suffer from this issue or they make a lot of request to track data and can live with this.

[kevinjwalls]

The only one generic question: doesn't it makes sense to update JMX to retry several times if Windows 2029 returns -1.0 or undefined to make JDK more reliable? Are tools like JMC/VisualVM suffer from this issue or they make a lot of request to track data and can live with this.

Yes, I was bit worried about adding the overhead of repeating the call, maybe 5 times, with short sleeps, in the MBean CPU monitoring code. It's been this way "forever", and is documented to return a negative value when info is "not available", so it is still delivering what it promises.
In our CI it's only Windows Server 2019 10.0 (amd64) where we see a problem. I don't see actual user reports of it being a problem. Once the call "works" it seems stable, so maybe the test is fast to startup and sees the problem. I think it's better to have the retry on (rare) failure than have the test delay to try and miss the problem, I don't know how reliable that would be without more investigation (which does not seem worthwhile right now). That may also relate to no reports from JMC/JConsole, but I'm just speculating now. 8-)

[lmesnik]

That's fine then.

[sspitsyn]

The fix is a little bit hacky but as a work around should be okay.
LGTM

[kevinjwalls]

Thanks for all the reviews and comments.

The timing check in GetSystemCpuLoad.java is a little hacky. That test has never claimed to try and test that the cpu time is "correct", but now it will at least test the time measured is not crazy. If I got the definition of "crazy" wrong, will be back here...

[kevinjwalls]

/integrate

[openjdk]

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

• e32a0c9: 8352706: httpclient HeadTest does not run on HTTP2
• 45a9750: 8351655: Optimize ObjectMonitor::unlink_after_acquire()
• 2e4d7d1: 8302459: Missing late inline cleanup causes compiler/vectorapi/VectorLogicalOpIdentityTest.java IR failure
• 1a8c8e0: 8352858: Make java.net.JarURLConnection fields final
• a81250c: 8352673: RISC-V: Vector can't be turned on with -XX:+UseRVV
• 0935ba9: 8352481: Enforce the use of lld with clang
• dbc620f: 8352299: GenShen: Young cycles that interrupt old cycles cannot be cancelled
• f5a0db4: 8315447: Invalid Type Annotation attached to a method instead of a lambda
• 60544a1: 8164714: Constructor.newInstance creates instance of inner class with null outer class
• c856b34: 8352587: C2 SuperWord: we must avoid Multiversioning for PeelMainPost loops
• ... and 38 more: https://git.openjdk.org/jdk/compare/a8757332667df3fe41a29a7eedb2a7234d23c2a0...master

Your commit was automatically rebased without conflicts.

[openjdk]

@kevinjwalls Pushed as commit eb6e828.

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