8273916: Remove 'special' ranking #5563

coleenp · 2021-09-17T11:50:22Z

This change removes the special ranking and folds it into nosafepoint. You have to look at commit #3 to see this actual part of the change that doesn't include JDK-8273915.
This passes tier1-6 also.

Progress

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

Issue

JDK-8273916: Remove 'special' ranking

Reviewers

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

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5563

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

Using diff file

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

…ing locks are always lower ranked than the safepoint checking locks because they cannot block.

bridgekeeper · 2021-09-17T11:51:11Z

👋 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-09-17T11:54:32Z

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

hotspot
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 · 2021-09-17T11:57:58Z

Webrevs

dholmes-ora

Sorry Coleen but I'm not understanding the mapping process here. I expected to see all special changed to the same thing, eg. nosafepoint, , and all special-N changed to nosafepoint-N, but you have not done that. ???

David

coleenp · 2021-09-20T19:40:19Z

Thanks for looking at this, David. I started to map special(-n) to nosafepoint(-n) but since special is nosafepoint - 9 (?) there were interactions with other nosafepoint locks so they needed rankings relative to other locks.

CompiledMethod_lock(nosafepoint-4) -> CodeCache_lock(nosafepoint-3) -> VtableStubs_lock(nosafepoint-2) -> CompiledIC_lock(nosafepoint)

CodeSweeper_lock(nosafepoint-5) -> CompiledMethod_lock(nosafepoint-4)

ThreadsSMRDelete_lock(nosafepoint-3) -> ConcurrentHashTableResize_lock(nosafepoint-2)

The compiler locks have the deepest nestings.

dholmes-ora · 2021-09-20T22:17:26Z

Sorry I don't follow. Lets examine that first example. We currently have:

CompiledMethod_lock (special-1) ->
CodeCache_lock (special) ->
VtableStubs_lock (leaf-2 == special + 11) ->
CompiledIC_lock(leaf+2 == special + 15)

If we change special to nosafepoint then we would have:

CompiledMethod_lock (nosafepoint-1) ->
CodeCache_lock (nosafepoint) ->
VtableStubs_lock (leaf-2 == nosafepoint + 4) ->
CompiledIC_lock(leaf+2 == nosafepoint + 8 == safepoint - 8)

so I don't see why we would have CodeCache_lock instead be nosafepoint-3 ?

If all you have done is change special to nosafepoint then all the existing relative rankings remain for things like leaf and leaf-2. The only adjustment you have to make is for a leaf+2 to instead be expressed as safepoint-8.

David

coleenp · 2021-09-20T22:30:30Z

All the lock rankings are negative values from locks they hold. The nosafepoint ranking and ranking below that are safepoint_check_never locks. So nosafepoint is the top of the lock hierarchy for safepoint_check_never locks. That's how we start with CompiledIC_lock at 'nosafepoint' rank.

The lock values will not be exactly the same value as they were when they were 'special'. They'll be ordered relative to their ranking in the nosafepoint to nosafepoint-n range.

I did skip a value though (nosafepoint-1).

dholmes-ora · 2021-09-20T23:32:12Z

So there is more to this than just removing the "special" ranking - you've also changed some locks that are safepoint_never, that used to have ranks above what is now nosafepoint, so that they instead have ranks below nosafepoint - is that right? As long a all relative rankings of locks that can be taken together is maintained, then that is okay - but it is very hard to see that just by looking at the changes.

coleenp · 2021-09-21T12:03:00Z

I just merged with my previous commit and hopefully this change makes a lot more sense now.

pchilano

Hi Coleen,

Changes look good to me. Not straightforward to verify by code inspection but seems you also already figured out dependent locks with all the testing.

Thanks,
Patricio

src/hotspot/share/runtime/mutexLocker.cpp

coleenp · 2021-09-21T23:43:11Z

Thanks Patricio for the code review!

dholmes-ora

Hi Coleen,

Thanks again for the offlist discussion. Sorry it took me so long to "get it".

Cheers,
David

openjdk · 2021-09-23T01:36:36Z

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

8273916: Remove 'special' ranking

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

6031388: 8273714: jdk/jfr/api/consumer/TestRecordedFrame.java still times out after JDK-8273047
8821b00: 8205137: Remove Applet support from SwingSet2
57fe11c: 8274120: [JVMCI] CompileBroker should resolve parameter types for JVMCI compiles
81d4164: 8272759: (fc) java/nio/channels/FileChannel/Transfer2GPlus.java failed in timeout
da38ced: 8271602: Add Math.ceilDiv() family parallel to Math.floorDiv() family
d39aad9: 8273924: ArrayIndexOutOfBoundsException thrown in java.util.JapaneseImperialCalendar.add()
c9de806: 8274039: codestrings gtest fails when hsdis is present
33df388: 8274003: ProcessHandleImpl.Info toString has an if check which is always true
0a36163: 8272600: (test) Use native "sleep" in Basic.java
c6df3c9: 8274071: Clean up java.lang.ref comments and documentation
... and 20 more: https://git.openjdk.java.net/jdk/compare/111d5e1a9324cb5e8d98627f6329d17fcbc9c13d...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 · 2021-09-23T11:19:39Z

Thank you @dholmes-ora for the discussions and review!
/integrate

openjdk · 2021-09-23T11:21:14Z

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

4f3b626: 8271567: AArch64: AES Galois CounterMode (GCM) interleaved implementation using vector instructions
8799856: 8274075: Fix miscellaneous typos in java.base
8b833bb: 8274048: IGV: Replace usages of Collections.sort with List.sort call
a74c099: 8252842: Extend jmap to support parallel heap dump
2166ed1: 8273894: ConcurrentModificationException raised every time ReferralsCache drops referral
1c6fa11: 8273979: move some os time related functions to os_posix for POSIX platforms
45adc92: 8273578: javax/swing/JMenu/4515762/bug4515762.java fails on macOS 12
0fbbe4c: 8274033: Some tier-4 CDS EpsilonGC tests throw OOM
9d3379b: 8267356: AArch64: Vector API SVE codegen support
6031388: 8273714: jdk/jfr/api/consumer/TestRecordedFrame.java still times out after JDK-8273047
... and 29 more: https://git.openjdk.java.net/jdk/compare/111d5e1a9324cb5e8d98627f6329d17fcbc9c13d...master

Your commit was automatically rebased without conflicts.

openjdk · 2021-09-23T11:21:26Z

@coleenp Pushed as commit d098751.

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

coleenp added 3 commits September 16, 2021 12:44

Partition safepoint checking and nonchecking lock ranks. The noncheck…

d14f8e1

…ing locks are always lower ranked than the safepoint checking locks because they cannot block.

Partition safepoint checking and nonchecking lock ranks. The noncheck…

9d4fbe9

…ing locks are always lower ranked than the safepoint checking locks because they cannot block.

Remove "special" rank.

1798827

openjdk bot added the rfr Pull request is ready for review label Sep 17, 2021

openjdk bot added serviceability serviceability-dev@openjdk.org hotspot hotspot-dev@openjdk.org labels Sep 17, 2021

dholmes-ora reviewed Sep 20, 2021

View reviewed changes

Add comment about ThreadSMRDelete_lock

81c5fef

coleenp added 2 commits September 21, 2021 07:59

Merge branch 'master' into remove-special

ecc0a99

Remove blank line.

5867b9a

pchilano approved these changes Sep 21, 2021

View reviewed changes

src/hotspot/share/runtime/mutexLocker.cpp Show resolved Hide resolved

Remove special comment.

6440a8c

dholmes-ora approved these changes Sep 23, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 23, 2021

openjdk bot closed this Sep 23, 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 Sep 23, 2021

coleenp deleted the remove-special branch September 23, 2021 11:22

8273916: Remove 'special' ranking #5563

8273916: Remove 'special' ranking #5563

Uh oh!

Conversation

coleenp commented Sep 17, 2021 • 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 Sep 17, 2021

Uh oh!

openjdk bot commented Sep 17, 2021

Uh oh!

mlbridge bot commented Sep 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

dholmes-ora left a comment

Choose a reason for hiding this comment

Uh oh!

coleenp commented Sep 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dholmes-ora commented Sep 20, 2021

Uh oh!

coleenp commented Sep 20, 2021

Uh oh!

dholmes-ora commented Sep 20, 2021

Uh oh!

coleenp commented Sep 21, 2021

Uh oh!

pchilano left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coleenp commented Sep 21, 2021

Uh oh!

dholmes-ora left a comment

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Sep 23, 2021

Uh oh!

coleenp commented Sep 23, 2021

Uh oh!

openjdk bot commented Sep 23, 2021

Uh oh!

openjdk bot commented Sep 23, 2021

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

coleenp commented Sep 17, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Sep 17, 2021 •

edited

Loading

coleenp commented Sep 20, 2021 •

edited

Loading