8362276: NMT tests should have locks for the entire tests #26324

gerard-ziemski · 2025-07-15T16:14:52Z

We restructure the code to make sure we cover the code that uses the global tree instance (but nothing else that uses the same lock internally) as needed:

we move MemoryReserver::reserve() to the top to do as the first thing
we grab the MemTracker::NmtVirtualMemoryLocker nvml
do all the usual test stuff, including checking, which grabs the global instance of the tree
we move remove_all() to the very bottom to do as the last thing

Testing: passes Mach5 tier1-4

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-8362276: NMT tests should have locks for the entire tests (Bug - P2)

Reviewers

Aleksey Shipilev (@shipilev - Reviewer)
Coleen Phillimore (@coleenp - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 26324

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-07-15T16:15:46Z

👋 Welcome back gziemski! 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 · 2025-07-15T16:16:09Z

@gerard-ziemski 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:

8362276: NMT tests should have locks for the entire tests

Reviewed-by: shade, coleenp

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

770d2b4: 8361076: Add benchmark for ImageReader in preparation for Valhalla changes
70c1ff7: 8362171: C2 fails with unexpected node in SuperWord truncation: ModI
6ed8164: 8362250: ARM32: forward_exception_entry missing return address
... and 30 more: https://git.openjdk.org/jdk/compare/b65fdf5af0a5e1cf0d66d7551c6df63e8d07c5fa...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 · 2025-07-15T16:16:52Z

@gerard-ziemski 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 · 2025-07-15T16:23:49Z

Webrevs

01: Full - Incremental (4387bee2)
00: Full (4958be33)

shipilev

Looks good, but synopsis even more confusing than the original one. Suggestion: "8362276: NMT tests should have locks for the entire tests" or something.

shipilev · 2025-07-15T18:36:44Z

Well, GHA thinks this does not work:

# A fatal error has been detected by the Java Runtime Environment:
#
#  Internal Error (/Users/runner/work/jdk/jdk/src/hotspot/share/runtime/mutex.cpp:143), pid=5569, tid=259
#  assert(owner() != self) failed: invariant

gerard-ziemski · 2025-07-15T19:49:56Z

Well, GHA thinks this does not work:

# A fatal error has been detected by the Java Runtime Environment:
#
#  Internal Error (/Users/runner/work/jdk/jdk/src/hotspot/share/runtime/mutex.cpp:143), pid=5569, tid=259
#  assert(owner() != self) failed: invariant

Yes, this is probably why Afshin went with his fix, investigating ....

coleenp · 2025-07-15T22:10:04Z

Looks good, but synopsis even more confusing than the original one. Suggestion: "8362276: NMT tests should have locks for the entire tests" or something.

Yes, please change the name to that. We changed the synopsis of the bug today to remove CLONE (simple fix), so you should change both the PR and the bug title.

shipilev

All right, this looks reasonable.

coleenp · 2025-07-16T11:17:27Z

test/hotspot/gtest/runtime/test_virtualMemoryTracker.cpp

    size_t size  = 0x01000000;
    ReservedSpace rs = MemoryReserver::reserve(size, mtTest);
+
+    RegionsTree* rtree = VirtualMemoryTracker::Instance::tree();


I have to confess to not paying attention to this change, but shouldn't you fetch the tree under the lock also?

I wondered the same, but this just returns an (unchanging) pointer. We are not in this locking game to ensure memory ordering, we only need mutual exclusion here. So IMO it does not matter for correctness whether we pull this under the lock or not.

Would be cleaner to do everything under the lock, but this seems fine as well.

Okay, I see that too. The code just made me wonder but it's okay.

coleenp

This looks good. Thank you for fixing the crash Gerard, and thank you for your attention to this bug Aleksey.

coleenp · 2025-07-16T13:08:59Z

test/hotspot/gtest/runtime/test_virtualMemoryTracker.cpp

    size_t size  = 0x01000000;
    ReservedSpace rs = MemoryReserver::reserve(size, mtTest);
+
+    RegionsTree* rtree = VirtualMemoryTracker::Instance::tree();


Okay, I see that too. The code just made me wonder but it's okay.

gerard-ziemski · 2025-07-16T15:26:59Z

Thank you for the feedback and reviews!

gerard-ziemski · 2025-07-16T15:27:17Z

/integrate

openjdk · 2025-07-16T15:27:33Z

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

770d2b4: 8361076: Add benchmark for ImageReader in preparation for Valhalla changes
70c1ff7: 8362171: C2 fails with unexpected node in SuperWord truncation: ModI
6ed8164: 8362250: ARM32: forward_exception_entry missing return address
... and 30 more: https://git.openjdk.org/jdk/compare/b65fdf5af0a5e1cf0d66d7551c6df63e8d07c5fa...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-07-16T15:27:49Z

@gerard-ziemski Pushed as commit 10ae602.

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

gerard-ziemski · 2025-07-17T18:38:05Z

slash-backport :jdk25

gerard-ziemski · 2025-07-17T18:39:07Z

/backport :jdk25

gerard-ziemski · 2025-07-17T18:39:39Z

/backport:jdk25

openjdk · 2025-07-17T18:40:12Z

@gerard-ziemski Could not automatically backport 10ae6029 to openjdk/jdk due to conflicts in the following files:

test/hotspot/gtest/runtime/test_virtualMemoryTracker.cpp

Please fetch the appropriate branch/commit and manually resolve these conflicts by using the following commands in your personal fork of openjdk/jdk. Note: these commands are just some suggestions and you can use other equivalent commands you know.

# Fetch the up-to-date version of the target branch
$ git fetch --no-tags https://git.openjdk.org/jdk.git jdk25:jdk25

# Check out the target branch and create your own branch to backport
$ git checkout jdk25
$ git checkout -b backport-gerard-ziemski-10ae6029-jdk25

# Fetch the commit you want to backport
$ git fetch --no-tags https://git.openjdk.org/jdk.git 10ae6029444c1381f7b1b3dcb6b6f32a4ae57efa

# Backport the commit
$ git cherry-pick --no-commit 10ae6029444c1381f7b1b3dcb6b6f32a4ae57efa
# Resolve conflicts now

# Commit the files you have modified
$ git add files/with/resolved/conflicts
$ git commit -m 'Backport 10ae6029444c1381f7b1b3dcb6b6f32a4ae57efa'

Once you have resolved the conflicts as explained above continue with creating a pull request towards the openjdk/jdk with the title Backport 10ae6029444c1381f7b1b3dcb6b6f32a4ae57efa.

Below you can find a suggestion for the pull request body:

Hi all,

This pull request contains a backport of commit 10ae6029 from the openjdk/jdk repository.

The commit being backported was authored by Gerard Ziemski on 16 Jul 2025 and was reviewed by Aleksey Shipilev and Coleen Phillimore.

Thanks!

make sure the lock covers remove_all()

4958be3

openjdk bot added the hotspot-runtime [email protected] label Jul 15, 2025

gerard-ziemski marked this pull request as ready for review July 15, 2025 16:19

openjdk bot added the rfr Pull request is ready for review label Jul 15, 2025

shipilev reviewed Jul 15, 2025

View reviewed changes

gerard-ziemski marked this pull request as draft July 15, 2025 19:48

openjdk bot removed the rfr Pull request is ready for review label Jul 15, 2025

restructure code to make sure we use the lock as needed

4387bee

gerard-ziemski changed the title ~~8362276: CLONE (simple fix) - NMT crash in gtest/NMTGtests.java: fatal error: NMT corruption: Block at 0x0000017748307120: header canary broken~~ 8362276: NMT tests should have locks for the entire tests Jul 15, 2025

gerard-ziemski marked this pull request as ready for review July 15, 2025 22:43

openjdk bot added the rfr Pull request is ready for review label Jul 15, 2025

shipilev approved these changes Jul 16, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jul 16, 2025

coleenp reviewed Jul 16, 2025

View reviewed changes

coleenp approved these changes Jul 16, 2025

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Jul 16, 2025

openjdk bot closed this Jul 16, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jul 16, 2025

8362276: NMT tests should have locks for the entire tests #26324

8362276: NMT tests should have locks for the entire tests #26324

Uh oh!

Conversation

gerard-ziemski commented Jul 15, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Jul 15, 2025

Uh oh!

openjdk bot commented Jul 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Jul 15, 2025

Uh oh!

mlbridge bot commented Jul 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

shipilev left a comment

Choose a reason for hiding this comment

Uh oh!

shipilev commented Jul 15, 2025

Uh oh!

gerard-ziemski commented Jul 15, 2025

Uh oh!

coleenp commented Jul 15, 2025

Uh oh!

shipilev left a comment

Choose a reason for hiding this comment

Uh oh!

coleenp Jul 16, 2025

Choose a reason for hiding this comment

Uh oh!

shipilev Jul 16, 2025

Choose a reason for hiding this comment

Uh oh!

coleenp Jul 16, 2025

Choose a reason for hiding this comment

Uh oh!

coleenp left a comment

Choose a reason for hiding this comment

Uh oh!

coleenp Jul 16, 2025

Choose a reason for hiding this comment

Uh oh!

gerard-ziemski commented Jul 16, 2025

Uh oh!

gerard-ziemski commented Jul 16, 2025

Uh oh!

openjdk bot commented Jul 16, 2025

Uh oh!

openjdk bot commented Jul 16, 2025

Uh oh!

gerard-ziemski commented Jul 17, 2025

Uh oh!

gerard-ziemski commented Jul 17, 2025

Uh oh!

gerard-ziemski commented Jul 17, 2025

Uh oh!

openjdk bot commented Jul 17, 2025

Uh oh!

Uh oh!

gerard-ziemski commented Jul 15, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Jul 15, 2025 •

edited

Loading

mlbridge bot commented Jul 15, 2025 •

edited

Loading