JDK-8307810: Consistently use LockingMode instead of UseHeavyMonitors

[tstuefe]

JDK-8291555 phased out UseHeavyMonitors in favor of LockingMode=0. We forgot to apply these changes to PPC and S390.

Since UseHeavyMonitors implies LockingMode, but not vice versa, we now have a mismatch if JVM is started with LockingMode=0 but without UseHeavyMonitors. That leads to crashes.

The patch fixes that, and in addition makes sure that if LockingMode=0 is set, we are setting UseHeavyMonitors too.

---

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-8307810: Consistently use LockingMode instead of UseHeavyMonitors

Reviewers

• Daniel D. Daugherty (@dcubed-ojdk - Reviewer)
• Martin Doerr (@TheRealMDoerr - Reviewer)
• David Holmes (@dholmes-ora - Reviewer)
• Amit Kumar (@offamitkumar - Author)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 13900

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

Using diff file

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

Webrev

Link to Webrev Comment

[tstuefe]

@MBaesken @TheRealMDoerr could you test this please on your CI and check if this fixes ppcle and s390? Thanks!

[bridgekeeper]

👋 Welcome back stuefe! 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]

@tstuefe The following label will be automatically applied to this pull request:

• hotspot

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]

Webrevs

• 03: Full - Incremental (dc870294)
• 02: Full - Incremental (af7dab6c)
• 01: Full - Incremental (bb805a86)
• 00: Full (cc8b9fb9)

[MBaesken]

@MBaesken @TheRealMDoerr could you test this please on your CI and check if this fixes ppcle and s390? Thanks!

I put it into our internal test queue .

[MBaesken]

Looks like the build fails now in arguments.cpp on a few platforms.

[offamitkumar]

Hi @tstuefe, Not sure how correct I am, but UseHeavyMonitors is not implemented for s390x, You may see an Issue open for this here. So i guess if you set UseHeavyMonitors to true for s390x, then build will fail.

Looks like the build fails now in arguments.cpp on a few platforms.

@MBaesken does that include s390x ?

[tstuefe]

Okay, I removed the setting-of-UseHeavyMonitors. We deprecated it in favour of LockingMode=0, and it is a develop flag now. I originally wanted to synchronize UseHeavyMonitors with LockingMode, but doing this only for debug makes no sense, and in release builds UseHeavyMonitors is const false.

[MBaesken]

@MBaesken does that include s390x ?

I referred to the github action builds above. I think there was no linux s390x build included in those.

[dcubed-ojdk]

Sorry we missed finding the rest of the UseHeavyMonitors uses
during the work on JDK-8291555.

Switching those uses to the appropriate check of LockingMode is the
right solution. We want UseHeavyMonitors to fade into the sands of
history...

[openjdk]

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

8307810: Consistently use LockingMode instead of UseHeavyMonitors

Reviewed-by: dcubed, mdoerr, dholmes, amitkumar

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

• 39f4e4d: 8307758: RISC-V: Improve bit test code introduced by JDK-8291555
• 3cb606e: 8306965: osThread allocation failures should not abort the VM
• 4795c39: 8281103: Give example for Locale that is English and follows the ISO standards
• 958d4a7: 8307563: make most fields final in JavacTrees
• f250ab2: 8307377: Remove use of tagName from TagletWriterImpl.linkSeeReferenceOutput
• cc9f7ad: 8307765: DynamicArchiveHeader contents are missing in CDS mapfile
• edc4adb: 8307860: [BACKOUT] JDK-8307194 Add make target for optionally building a complete set of all JDK and hotspot libjvm static libraries
• df90df2: 8307857: validate-source fails after JDK-8306758
• 2688364: 8306758: com/sun/jdi/ConnectedVMs.java fails with "Non-zero debuggee exitValue: 143"
• 1964954: 8307194: Add make target for optionally building a complete set of all JDK and hotspot libjvm static libraries
• ... and 3 more: https://git.openjdk.org/jdk/compare/1bca05eace5c4622a1fd596f4ed196495a8801e4...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.

[TheRealMDoerr]

Thank you for fixing this! Works on PPC64 and avoids "assert(mark.is_neutral()) failed" when -XX:LockingMode=0 is selected.
@offamitkumar: I think s390 requires additional changes in MacroAssembler::compiler_fast_lock_object and MacroAssembler::compiler_fast_unlock_object, but that should probably better be done separately.

[dholmes-ora]

Seems fine.

Thanks.

[offamitkumar]

Build/Tests are good on s390x,

Thanks for the changes :-)

[offamitkumar]

@offamitkumar: I think s390 requires additional changes in MacroAssembler::compiler_fast_lock_object and MacroAssembler::compiler_fast_unlock_object, but that should probably better be done separately.

Sure Martin, I'll look into.

Thank you.

[tstuefe]

Thanks @offamitkumar @TheRealMDoerr @dholmes-ora @dcubed-ojdk !

/integrate

[openjdk]

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

• 39f4e4d: 8307758: RISC-V: Improve bit test code introduced by JDK-8291555
• 3cb606e: 8306965: osThread allocation failures should not abort the VM
• 4795c39: 8281103: Give example for Locale that is English and follows the ISO standards
• 958d4a7: 8307563: make most fields final in JavacTrees
• f250ab2: 8307377: Remove use of tagName from TagletWriterImpl.linkSeeReferenceOutput
• cc9f7ad: 8307765: DynamicArchiveHeader contents are missing in CDS mapfile
• edc4adb: 8307860: [BACKOUT] JDK-8307194 Add make target for optionally building a complete set of all JDK and hotspot libjvm static libraries
• df90df2: 8307857: validate-source fails after JDK-8306758
• 2688364: 8306758: com/sun/jdi/ConnectedVMs.java fails with "Non-zero debuggee exitValue: 143"
• 1964954: 8307194: Add make target for optionally building a complete set of all JDK and hotspot libjvm static libraries
• ... and 3 more: https://git.openjdk.org/jdk/compare/1bca05eace5c4622a1fd596f4ed196495a8801e4...master

Your commit was automatically rebased without conflicts.

[openjdk]

@tstuefe Pushed as commit 984fbbb.

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