8325303: Replace markWord.is_neutral() with markWord.is_unlocked()

[dholmes-ora]

This is a small tidy up to try and remove confusion between checking is_neutral (a general state normally associated with a displaced markword in a "pristine" state) and is_unlocked (a specific state within the locking protocol). The underlying bit-pattern is the same and so these have been used somewhat synonymously/interchangeably.

A few comment tweaks too.

Testing: tiers 1-3 (sanity)

Thanks.

---

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-8325303: Replace markWord.is_neutral() with markWord.is_unlocked() (Enhancement - P4)

Reviewers

• Stefan Karlsson (@stefank - Reviewer)
• Daniel D. Daugherty (@dcubed-ojdk - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 17741

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

Using diff file

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

Webrev

Link to Webrev Comment

[bridgekeeper]

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

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

[openjdk]

@dholmes-ora this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout 8325303-is_neutral
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

[openjdk]

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

8325303: Replace markWord.is_neutral() with markWord.is_unlocked()

Reviewed-by: stefank, dcubed

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

• 83eba86: 8329332: Remove CompiledMethod and CodeBlobLayout classes
• 28216aa: 8328366: Thread.setContextClassloader from thread in FJP commonPool task no longer works after JDK-8327501
• 4276d5c: 8329637: Apparent typo in java.security file property jdk.tls.keyLimits
• 6382a12: 8326666: Remove the Java Management Extension (JMX) Subject Delegation feature
• d90e5b5: 8329546: Assume sized integral types are available
• f3db279: 8327410: Add hostname option for UL file names
• 21867c9: 8313332: Simplify lazy jmethodID cache in InstanceKlass
• b9da140: 8329594: G1: Consistent Titles to Thread Work Items.
• a169c06: 8329580: Parallel: Remove VerifyObjectStartArray
• 8efd7aa: 8328786: [AIX] move some important warnings/errors from trcVerbose to UL

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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.

[mlbridge]

Webrevs

• 00: Full (e16c488d)

[stefank]

This seems reasonable to me. Are there any use-cases of is_neutral() left? Could you explain why we use is_neutral() there and not is_locked()?

[dcubed-ojdk]

Thumbs up.

I still found nine uses of is_neutral() in src/hotspot/share/runtime/synchronizer.cpp.
I suspect you left these alone because they are all associated with displaced mark
words, AKA: dmw/dmh/mark/temp/test. There are just too freaking names for the
same concept of the "displaced mark word". I thought I had cleaned those up years
ago, but obviously not everywhere or they changed again.

[dholmes-ora]

Thanks for the reviews @stefank and @dcubed-ojdk .

Are there any use-cases of is_neutral() left? Could you explain why we use is_neutral() there and not is_locked()?

As Dan indicated (thanks Dan) yes there remain uses of is_neutral associated with inspection of the displaced markword. The displaced markword is (mostly) used when the associated object is locked, but the displaced markword itself contains the unlocked bit pattern. So I decided to keep the is_neutral terminology in those cases to avoid potential avoid confusion.

As this doesn't seem to be a sticking point I will proceed with integration.

Thanks again.

/integrate

[openjdk]

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

• 27cfcef: 8329651: TestLibGraal.java crashes with assert(_stack_base != nullptr)
• e1183ac: 8329703: Remove unused apple.jpeg file from SwingSet2 demo
• 12ad09a: 8322042: HeapDumper should perform merge on the current thread instead of VMThread
• d80d478: 8328649: Disallow enclosing instances for local classes in constructor prologues
• 83eba86: 8329332: Remove CompiledMethod and CodeBlobLayout classes
• 28216aa: 8328366: Thread.setContextClassloader from thread in FJP commonPool task no longer works after JDK-8327501
• 4276d5c: 8329637: Apparent typo in java.security file property jdk.tls.keyLimits
• 6382a12: 8326666: Remove the Java Management Extension (JMX) Subject Delegation feature
• d90e5b5: 8329546: Assume sized integral types are available
• f3db279: 8327410: Add hostname option for UL file names
• ... and 4 more: https://git.openjdk.org/jdk/compare/f26e4308992d989d71e7fbfaa3feb95f0ea17c06...master

Your commit was automatically rebased without conflicts.

[openjdk]

@dholmes-ora Pushed as commit 34f7974.

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

[stefank]

Thanks for the reviews @stefank and @dcubed-ojdk .

Are there any use-cases of is_neutral() left? Could you explain why we use is_neutral() there and not is_locked()?

As Dan indicated (thanks Dan) yes there remain uses of is_neutral associated with inspection of the displaced markword. The displaced markword is (mostly) used when the associated object is locked, but the displaced markword itself contains the unlocked bit pattern. So I decided to keep the is_neutral terminology in those cases to avoid potential avoid confusion.

As this doesn't seem to be a sticking point I will proceed with integration.

Thanks again.

/integrate

In BasicLock::move_to you renamed is_neutral to is_locked should that have stayed as is_neutral?

-   if (displaced_header().is_neutral()) {
+   if (displaced_header().is_unlocked()) {

[dholmes-ora]

In BasicLock::move_to you renamed is_neutral to is_locked should that have stayed as is_neutral?

Yep - I will fix. Thanks @stefank