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

8273300: Check Mutex ranking during a safepoint #5467

Closed
wants to merge 8 commits into from

Conversation

coleenp
Copy link
Contributor

@coleenp coleenp commented Sep 10, 2021

This change checks lock ranking during a safepoint. For some reason, safepoint checking was excluded, probably from the days where Safepoint_lock and Threads_lock were used.
Because of checking during a safepoint, some locks had to get lower ranks. The CR has the details of which locks these were. The Service_lock complicates things because it's held during oops_do, which may take out other G1 locks.
This was built and tested with Shenandoah. Thanks to @zhengyu123 for the changes in Shenandoah.
Tests run tier1-8.


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/5467/head:pull/5467
$ git checkout pull/5467

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5467

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Sep 10, 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 10, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Sep 10, 2021

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

  • hotspot
  • 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.

@openjdk openjdk bot added hotspot shenandoah labels Sep 10, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Sep 10, 2021

Webrevs

fisk
fisk approved these changes Sep 10, 2021
Copy link
Contributor

@fisk fisk left a comment

Looks good.

@openjdk
Copy link

@openjdk openjdk bot commented Sep 10, 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:

8273300: Check Mutex ranking during a safepoint

Reviewed-by: eosterlund, 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 18 new commits pushed to the master branch:

  • 7b2beb6: 8273823: Problemlist gc/stringdedup tests timing out on ZGC
  • 8290424: 8272771: frame::pd_ps() is not implemented on any platform
  • a3ca770: 8272815: jpackage --type rpm produces an error: Invalid or unsupported type: [null]
  • 8132bfd: 8273559: Shenandoah: Shenandoah should support multi-threaded heap dump
  • f531b5c: 8273514: java/util/DoubleStreamSums/CompensatedSums.java failure
  • 4c673df: 8273656: Improve java.lang.invoke.MethodType.parameterList() and its usage
  • 8fbcc82: 8273494: Zero: Put libjvm.so into "zero" folder, not "server"
  • 92c30c9: 8273599: Remove cross_threshold method usage around GC
  • 02af541: 8273605: VM Exit does not abort concurrent mark
  • febcc72: 8273366: [testbug] javax/swing/UIDefaults/6302464/bug6302464.java fails on macOS12
  • ... and 8 more: https://git.openjdk.java.net/jdk/compare/fe89dd3b0d47807c7dbfe24d17f6aca152fc8908...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 10, 2021
@coleenp
Copy link
Contributor Author

@coleenp coleenp commented Sep 10, 2021

Thanks Erik!

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Hi Coleen,

The core change seems clean and simple, but the fanout of the ranking changes is far less clear and some of this looks like it could/should be separated out - see comments below.

If I see a lock with a rank service-1, then should I infer that lock can be acquired while the service (or notification) lock is held? And that if lock A is service-1 and lock B is service-2, then B can be acquired while holding A?

Thanks,
David

src/hotspot/share/memory/universe.cpp Outdated Show resolved Hide resolved
src/hotspot/share/memory/universe.cpp Show resolved Hide resolved
@coleenp
Copy link
Contributor Author

@coleenp coleenp commented Sep 13, 2021

If I see a lock with a rank service-1, then should I infer that lock can be acquired while the service (or notification) lock is held? And that if lock A is service-1 and lock B is service-2, then B can be acquired while holding A?

Yes. The Service_lock is held while all these other locks are taken. The subtraction comes from the highest ranked lock with a name, in this case 'service'.

@coleenp
Copy link
Contributor Author

@coleenp coleenp commented Sep 14, 2021

I just remerged from master and a couple of copyright updates leaked in from my script (which I shouldn't have used).

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Hi Coleen,

It wasn't clear if you thought you had already fixed the incorrect copyright notices so I flagged the two I saw.

I still have some queries about this - see comment below - but nothing that would stop it from being pushed as-is.

Thanks,
David

src/hotspot/cpu/zero/vm_version_zero.cpp Outdated Show resolved Hide resolved
src/hotspot/share/memory/universe.cpp Show resolved Hide resolved
src/hotspot/share/runtime/mutex.hpp Show resolved Hide resolved
src/hotspot/share/utilities/growableArray.hpp Outdated Show resolved Hide resolved
@coleenp
Copy link
Contributor Author

@coleenp coleenp commented Sep 16, 2021

Thanks Patricio for the code review. Thanks David for all the comments.
/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Sep 16, 2021

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

  • c86e24d: 8271954: C2: assert(false) failed: Bad graph detected in build_loop_late
  • 14dc517: 8273372: Remove scavenge trace message in psPromotionManager
  • 241ac89: 8273606: Zero: SPARC64 build fails with si_band type mismatch
  • 181292d: 8273801: Handle VMTYPE for "core" VM variant
  • 09ecb11: 8273806: compiler/cpuflags/TestSSE4Disabled.java should test for CPU feature explicitly
  • 99cfc16: 8273805: gc/g1/TestGCLogMessages.java test should handle non-JFR configs
  • 1c5de8b: 8273807: Zero: Drop incorrect test block from compiler/startup/NumCompilerThreadsCheck.java
  • 46af82e: 8273804: Platform.isTieredSupported should handle the no-compiler case
  • 2d13fb2: 8273803: Zero: Handle "zero" variant in CommandLineOptionTest.java
  • d4546b6: 8273526: Extend the OSContainer API pids controller with pids.current
  • ... and 20 more: https://git.openjdk.java.net/jdk/compare/fe89dd3b0d47807c7dbfe24d17f6aca152fc8908...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Sep 16, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Sep 16, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Sep 16, 2021

@coleenp Pushed as commit 5e4d09c.

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

@coleenp coleenp deleted the checkrank branch Sep 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot integrated shenandoah
4 participants