Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8273916: Remove 'special' ranking #5563

Closed
wants to merge 7 commits into from
Closed

Conversation

coleenp
Copy link
Contributor

@coleenp coleenp commented Sep 17, 2021

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

Reviewers

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

coleenp added 3 commits Sep 16, 2021
…ing locks are always lower ranked than the safepoint checking locks because they cannot block.
…ing locks are always lower ranked than the safepoint checking locks because they cannot block.
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Sep 17, 2021

👋 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 openjdk bot added the rfr label Sep 17, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Sep 17, 2021

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

@openjdk openjdk bot added serviceability hotspot labels Sep 17, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Sep 17, 2021

Webrevs

Copy link
Member

@dholmes-ora dholmes-ora left a comment

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
Copy link
Contributor Author

@coleenp coleenp commented Sep 20, 2021

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
Copy link
Member

@dholmes-ora dholmes-ora commented Sep 20, 2021

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
Copy link
Contributor Author

@coleenp coleenp commented Sep 20, 2021

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
Copy link
Member

@dholmes-ora dholmes-ora commented Sep 20, 2021

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
Copy link
Contributor Author

@coleenp coleenp commented Sep 21, 2021

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

Copy link
Contributor

@pchilano pchilano left a comment

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 Show resolved Hide resolved
@coleenp
Copy link
Contributor Author

@coleenp coleenp commented Sep 21, 2021

Thanks Patricio for the code review!

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Hi Coleen,

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

Cheers,
David

@openjdk
Copy link

@openjdk openjdk bot commented Sep 23, 2021

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

@openjdk openjdk bot added the ready label Sep 23, 2021
@coleenp
Copy link
Contributor Author

@coleenp coleenp commented Sep 23, 2021

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

@openjdk
Copy link

@openjdk openjdk bot commented Sep 23, 2021

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 openjdk bot closed this Sep 23, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Sep 23, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Sep 23, 2021

@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 coleenp deleted the remove-special branch Sep 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot integrated serviceability
3 participants