8336410: runtime/NMT/TotalMallocMmapDiffTest.java Total malloc diff is incorrect. Expected malloc diff range

[jdksjolen]

Hi,

We've seen intermittent failures in this test because of the change being out of the pre-defined range. The largest "out of bounds" we've observed is 1.5MiB, so increasing the fudge factor to allow for 25MiBs more/less should be sufficient for us to not see this error for a while. This kind of adjustment is an unfortunate necessity when developing these kinds of tests.

---

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-8336410: runtime/NMT/TotalMallocMmapDiffTest.java Total malloc diff is incorrect. Expected malloc diff range (Bug - P4)

Reviewers

• Gerard Ziemski (@gerard-ziemski - Committer)
• Thomas Stuefe (@tstuefe - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 20326

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

Using diff file

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

Webrev

Link to Webrev Comment

[bridgekeeper]

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

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

8336410: runtime/NMT/TotalMallocMmapDiffTest.java Total malloc diff is incorrect. Expected malloc diff range

Reviewed-by: gziemski, stuefe

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

• 7e21d4c: 8337268: Redundant Math.ceil in StyleSheet.ListPainter#drawShape
• ab27090: 8337225: Demote maxStack and maxLocals from CodeModel to CodeAttribute
• bd36b6a: 8337285: Examine java.text.DecimalFormat API for api/implXxx tag usage
• a86244f: 6381729: Javadoc for generic constructor doesn't document type parameter
• c4e6255: 8332738: Debug agent can deadlock on callbackLock when using StackFrame.PopFrames
• c23d37e: 8337300: java/lang/Process/WaitForDuration.java leaves child process behind
• ee365d7: 8336342: Fix known X11 library locations in sysroot
• cd52ad8: 8337267: [REDO] G1: Refactor G1RebuildRSAndScrubTask
• 9124a94: 8337165: Test jdk/jfr/event/gc/stacktrace/TestG1YoungAllocationPendingStackTrace.java failed: IndexOutOfBoundsException: Index 64 out of bounds for length 64
• db168d9: 8336966: Alpine Linux x86_64 compilation error: sendfile64
• ... and 28 more: https://git.openjdk.org/jdk/compare/3baff2af6a30cc6cb2e0d4391db1cf7e61c33f64...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]

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

• hotspot-runtime

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.

[mlbridge]

Webrevs

• 01: Full - Incremental (e0795616)
• 00: Full (37b12d06)

[gerard-ziemski]

Should we consider splitting the fudge factor into 2 - one for lower bound, the other for upper one?

I can see how the lower bound should be tighter, whereas the upper one might be harder to define and we might need to keep upping?

[tstuefe]

Ping

Should we consider splitting the fudge factor into 2 - one for lower bound, the other for upper one?

I can see how the lower bound should be tighter, whereas the upper one might be harder to define and we might need to keep upping?

That sounds reasonable.

In addition, consider running the test with C1 only, to remove C2 and its arenas out of the equation.

[jdksjolen]

Both good ideas, will fix on Monday :).

[jdksjolen]

@tstuefe , @gerard-ziemski , ping for approval!

[gerard-ziemski]

LGTM, thanks for fixing this!

[tstuefe]

ok

[jdksjolen]

Thanks!

/integrate

[openjdk]

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

• c095c0e: 8336489: Track scoped accesses in JVMCI compiled code
• 7e925d7: 8337283: configure.log is truncated when build dir is on different filesystem
• 2e093b0: 8337779: test/jdk/jdk/jfr/jvm/TestHiddenWait.java is a bit fragile
• d3e7b0c: 8336462: ConcurrentSkipListSet Javadoc incorrectly warns about size method complexity
• be34730: 8335257: Refactor code to create Initialized Assertion Predicates into separate class
• fbe8a81: 8333144: docker tests do not work when ubsan is configured
• dabdae6: 8334599: Improve code from JDK-8302671
• 8bd3cd5: 8337716: ByteBuffer hashCode implementations are inconsistent
• 367e0a6: 8334755: Asymptotically faster implementation of square root algorithm
• 34edc73: 8337396: Cleanup usage of ExternalAddess
• ... and 86 more: https://git.openjdk.org/jdk/compare/3baff2af6a30cc6cb2e0d4391db1cf7e61c33f64...master

Your commit was automatically rebased without conflicts.

[openjdk]

@jdksjolen Pushed as commit 97afbd9.

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