8274004: Change 'nonleaf' rank name #5845

coleenp · 2021-10-06T23:27:17Z

Also fixes: 8273956: Add checking for rank values

This change does 3 things. I could separate them but this has all been tested together and most of the change is mechanical. The first is a simple rename of nonleaf => safepoint. The second change is to add the enum class Rank which only allows subtraction from a base rank, and some additional type checking so that rank arithmetic doesn't overlap with a different rank. Ie if you say nosafepoint-n, that doesn't overlap with oopstorage. The third change is to remove the _safepoint_check_always/never flag. That is now intrinsic in the ranking, as all nosafepoint ranks are lower than all safepoint ranks.

Future changes could add rank names in the middle of nosafepoint and safepoint, like handshake. As of now, the rank subtractions are not unmanageable. There are actually not many nested Mutex.

This is the last of the planned subtasks for Mutex ranking cleanup. If you have other ideas or suggestions, please let me know.

Tested with tier1-8 at one point in time and just retested with tier1-6.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issues

JDK-8274004: Change 'nonleaf' rank name
JDK-8273956: Add checking for rank values

Reviewers

David Holmes (@dholmes-ora - Reviewer) ⚠️ Review applies to 59bfd94
Patricio Chilano Mateo (@pchilano - Committer) ⚠️ Review applies to 59bfd94

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5845/head:pull/5845
$ git checkout pull/5845

Update a local copy of the PR:
$ git checkout pull/5845
$ git pull https://git.openjdk.java.net/jdk pull/5845/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5845

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5845.diff

8273956: Add checking for rank values

bridgekeeper · 2021-10-06T23:27:55Z

👋 Welcome back coleenp! 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 · 2021-10-06T23:32:12Z

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

hotspot
serviceability
shenandoah

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 · 2021-10-06T23:37:18Z

Webrevs

dcubed-ojdk · 2021-10-06T23:42:03Z

The PR description mentions:
8273956: Add checking for rank values
but the PR synopsis is:
8274004: Change 'nonleaf' rank name

mlbridge · 2021-10-06T23:49:39Z

Mailing list message from coleen.phillimore at oracle.com on hotspot-dev:

On 10/6/21 7:45 PM, Daniel D.Daugherty wrote:

On Wed, 6 Oct 2021 23:27:17 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

8273956: Add checking for rank values

This change does 3 things. I could separate them but this has all been tested together and most of the change is mechanical. The first is a simple rename of nonleaf => safepoint. The second change is to add the enum class Rank which only allows subtraction from a base rank, and some additional type checking so that rank arithmetic doesn't overlap with a different rank. Ie if you say nosafepoint-n, that doesn't overlap with oopstorage. The third change is to remove the _safepoint_check_always/never flag. That is now intrinsic in the ranking, as all nosafepoint ranks are lower than all safepoint ranks.

Future changes could add rank names in the middle of nosafepoint and safepoint, like handshake. As of now, the rank subtractions isn't unmanageable. There are actually not many nested Mutex.

This is the last of the planned subtasks for Mutex ranking cleanup. If you have other ideas or suggestions, please let me know.

Tested with tier1-8 at one point in time and just retested with tier1-6.
The PR description mentions:
`8273956: Add checking for rank values`
but the PR synopsis is:
`8274004: Change 'nonleaf' rank name`

It's actually both.? But I should close the 8273956 as a duplicate of
the first and remove that line in the description.
thanks,
Coleen

dcubed-ojdk · 2021-10-06T23:50:31Z

Just do

/issue JDK-8274004
/issue JDK-8273956

and this PR will cover both.

openjdk · 2021-10-06T23:53:06Z

@dcubed-ojdk Only the author (@coleenp) is allowed to issue the /issue command.

openjdk · 2021-10-06T23:53:20Z

@dcubed-ojdk Only the author (@coleenp) is allowed to issue the /issue command.

coleenp · 2021-10-07T00:01:49Z

/issue JDK-8274004
/issue JDK-8273956

Thank you @dcubed-ojdk!

openjdk · 2021-10-07T00:02:42Z

@coleenp This issue is referenced in the PR title - it will now be updated.

openjdk · 2021-10-07T00:02:58Z

@coleenp
Adding additional issue to issue list: 8273956: Add checking for rank values.

dholmes-ora

Hi Coleen,

Overall this looks good! But shouldn't the test:

test/hotspot/gtest/runtime/test_safepoint_locks.cpp

be replaced by one that checks we lock with/without a safepoint check for a safepoint/nonsafepoint lock respectively? (Or does that test already exist elsewhere?).

Also should there not be a gtest to verify your Rank overlap checking?

Thanks,
David

test/hotspot/gtest/runtime/test_mutex.cpp

pchilano

LGTM!

Thanks,
Patricio

coleenp · 2021-10-07T16:07:28Z

Thanks for the review @pchilano and offline discussion about check_for_overlap.

dholmes-ora

Hi Coleen,

A couple of comments on tests, but otherwise okay.

Thanks,
David

dholmes-ora · 2021-10-08T00:48:05Z

test/hotspot/gtest/runtime/test_mutex.cpp

+  monitor_rank_broken->lock_without_safepoint_check();
+  monitor_rank_broken->unlock();


This is dead code right - the assertion failure will stop us getting here.

Yes, it is dead code. I'll remove it and retest to make sure nothing surprising happens.

dholmes-ora · 2021-10-08T00:48:31Z

test/hotspot/gtest/runtime/test_mutex.cpp

+  monitor_rank_broken->lock_without_safepoint_check();
+  monitor_rank_broken->unlock();


Ditto - dead code

dholmes-ora · 2021-10-08T00:52:26Z

test/hotspot/gtest/runtime/test_mutex.cpp

+  JavaThread* THREAD = JavaThread::current();
+  ThreadInVMfromNative invm(THREAD);
+
+  Monitor* monitor_rank_broken = new Monitor(Mutex::safepoint-1, "monitor_rank_broken");


This rank is not actually broken is it - otherwise we won't get to the next line.

right. It's not broken. I'll rename it to 'monitor_rank_ok'.

dholmes-ora · 2021-10-08T00:52:36Z

test/hotspot/gtest/runtime/test_mutex.cpp

+  monitor_rank_also_broken->lock_without_safepoint_check();
+  monitor_rank_also_broken->unlock();


openjdk · 2021-10-08T00:54:26Z

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

8274004: Change 'nonleaf' rank name
8273956: Add checking for rank values

Reviewed-by: dholmes, pchilanomate

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

b60837a: 8272586: emit abstract machine code in hs-err logs
97ea9dd: 8274840: Update OS detection code to recognize Windows 11
8de7763: 8273152: Refactor CDS FileMapHeader loading code
920e707: 8274920: ProblemList 2 VectorAPI tests failing due to "assert(!vbox->is_Phi()) failed"
7de2cf8: 8273910: Redundant condition and assignment in java.net.URI
8ca0846: 8274407: (tz) Update Timezone Data to 2021c
4ab274a: 8274858: Remove unused dictionary_classes_do functions
03a8d34: 8273826: Correct Manifest file name and NPE checks
722d639: 8274838: runtime/cds/appcds/TestSerialGCWithCDS.java fails on Windows
340c715: 8273929: Remove GzipRandomAccess in heap dump test
... and 13 more: https://git.openjdk.java.net/jdk/compare/c10de3538b47c182d7bfeb02f348fac9b2ad0641...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.

coleenp

I removed the dead test code and retested.

coleenp · 2021-10-08T12:09:50Z

test/hotspot/gtest/runtime/test_mutex.cpp

+  monitor_rank_broken->lock_without_safepoint_check();
+  monitor_rank_broken->unlock();


Yes, it is dead code. I'll remove it and retest to make sure nothing surprising happens.

coleenp · 2021-10-08T12:10:32Z

test/hotspot/gtest/runtime/test_mutex.cpp

+  monitor_rank_broken->lock_without_safepoint_check();
+  monitor_rank_broken->unlock();


coleenp · 2021-10-08T12:11:20Z

test/hotspot/gtest/runtime/test_mutex.cpp

+  JavaThread* THREAD = JavaThread::current();
+  ThreadInVMfromNative invm(THREAD);
+
+  Monitor* monitor_rank_broken = new Monitor(Mutex::safepoint-1, "monitor_rank_broken");


right. It's not broken. I'll rename it to 'monitor_rank_ok'.

coleenp · 2021-10-08T12:12:00Z

test/hotspot/gtest/runtime/test_mutex.cpp

+  monitor_rank_also_broken->lock_without_safepoint_check();
+  monitor_rank_also_broken->unlock();


coleenp · 2021-10-08T12:22:05Z

Thanks for the code review David, and all the discussions leading to this change.

coleenp · 2021-10-08T12:22:35Z

/integrate

openjdk · 2021-10-08T12:23:36Z

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

b60837a: 8272586: emit abstract machine code in hs-err logs
97ea9dd: 8274840: Update OS detection code to recognize Windows 11
8de7763: 8273152: Refactor CDS FileMapHeader loading code
920e707: 8274920: ProblemList 2 VectorAPI tests failing due to "assert(!vbox->is_Phi()) failed"
7de2cf8: 8273910: Redundant condition and assignment in java.net.URI
8ca0846: 8274407: (tz) Update Timezone Data to 2021c
4ab274a: 8274858: Remove unused dictionary_classes_do functions
03a8d34: 8273826: Correct Manifest file name and NPE checks
722d639: 8274838: runtime/cds/appcds/TestSerialGCWithCDS.java fails on Windows
340c715: 8273929: Remove GzipRandomAccess in heap dump test
... and 13 more: https://git.openjdk.java.net/jdk/compare/c10de3538b47c182d7bfeb02f348fac9b2ad0641...master

Your commit was automatically rebased without conflicts.

openjdk · 2021-10-08T12:23:49Z

@coleenp Pushed as commit 6364719.

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

8274004: Change 'nonleaf' rank name

3c6a5b6

8273956: Add checking for rank values

openjdk bot added the rfr Pull request is ready for review label Oct 6, 2021

openjdk bot added serviceability serviceability-dev@openjdk.org hotspot hotspot-dev@openjdk.org shenandoah shenandoah-dev@openjdk.org labels Oct 6, 2021

dholmes-ora reviewed Oct 7, 2021

View reviewed changes

coleenp commented Oct 7, 2021

View reviewed changes

test/hotspot/gtest/runtime/test_mutex.cpp Show resolved Hide resolved

coleenp commented Oct 7, 2021

View reviewed changes

test/hotspot/gtest/runtime/test_mutex.cpp Show resolved Hide resolved

Fix overlap error message printing and add a test.

59bfd94

pchilano approved these changes Oct 7, 2021

View reviewed changes

dholmes-ora approved these changes Oct 8, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 8, 2021

Remove dead code in test.

e8df15d

coleenp commented Oct 8, 2021

View reviewed changes

openjdk bot closed this Oct 8, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 8, 2021

coleenp deleted the rank branch October 8, 2021 12:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8274004: Change 'nonleaf' rank name #5845

8274004: Change 'nonleaf' rank name #5845

coleenp commented Oct 6, 2021 •

edited by openjdk bot

bridgekeeper bot commented Oct 6, 2021

openjdk bot commented Oct 6, 2021

mlbridge bot commented Oct 6, 2021 •

edited

dcubed-ojdk commented Oct 6, 2021

mlbridge bot commented Oct 6, 2021

dcubed-ojdk commented Oct 6, 2021 •

edited

openjdk bot commented Oct 6, 2021

openjdk bot commented Oct 6, 2021

coleenp commented Oct 7, 2021

openjdk bot commented Oct 7, 2021

openjdk bot commented Oct 7, 2021

dholmes-ora left a comment

pchilano left a comment

coleenp commented Oct 7, 2021

dholmes-ora left a comment

dholmes-ora Oct 8, 2021

coleenp Oct 8, 2021

dholmes-ora Oct 8, 2021

coleenp Oct 8, 2021

dholmes-ora Oct 8, 2021

coleenp Oct 8, 2021

dholmes-ora Oct 8, 2021

coleenp Oct 8, 2021

openjdk bot commented Oct 8, 2021 •

edited

coleenp left a comment

coleenp Oct 8, 2021

coleenp Oct 8, 2021

coleenp Oct 8, 2021

coleenp Oct 8, 2021

coleenp commented Oct 8, 2021

coleenp commented Oct 8, 2021

openjdk bot commented Oct 8, 2021

openjdk bot commented Oct 8, 2021

		monitor_rank_broken->lock_without_safepoint_check();
		monitor_rank_broken->unlock();

		monitor_rank_also_broken->lock_without_safepoint_check();
		monitor_rank_also_broken->unlock();

8274004: Change 'nonleaf' rank name #5845

8274004: Change 'nonleaf' rank name #5845

Conversation

coleenp commented Oct 6, 2021 • edited by openjdk bot

Progress

Issues

Reviewers

Reviewing

bridgekeeper bot commented Oct 6, 2021

openjdk bot commented Oct 6, 2021

mlbridge bot commented Oct 6, 2021 • edited

Webrevs

dcubed-ojdk commented Oct 6, 2021

mlbridge bot commented Oct 6, 2021

dcubed-ojdk commented Oct 6, 2021 • edited

openjdk bot commented Oct 6, 2021

openjdk bot commented Oct 6, 2021

coleenp commented Oct 7, 2021

openjdk bot commented Oct 7, 2021

openjdk bot commented Oct 7, 2021

dholmes-ora left a comment

Choose a reason for hiding this comment

pchilano left a comment

Choose a reason for hiding this comment

coleenp commented Oct 7, 2021

dholmes-ora left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjdk bot commented Oct 8, 2021 • edited

coleenp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coleenp commented Oct 8, 2021

coleenp commented Oct 8, 2021

openjdk bot commented Oct 8, 2021

openjdk bot commented Oct 8, 2021

coleenp commented Oct 6, 2021 •

edited by openjdk bot

mlbridge bot commented Oct 6, 2021 •

edited

dcubed-ojdk commented Oct 6, 2021 •

edited

openjdk bot commented Oct 8, 2021 •

edited