On Wed, 26 Aug 2026 15:48:38 GMT, Axel Boldt-Christmas <[email protected]> wrote:
> The interface on oopDesc and the markWord w.r.t. the locking bits have grown > overtime the names do not reflect what they actually do, there are multiple > ways of asking for the same property. > > The properties `is_locked` and `is_unlocked` are misleading. As the answer > true of false does not necessarily reflect the locking state of the object. I > suggest we use a single terminology `is_fast_locked` to mean the locking bits > are locked using lightweight non-monitor locking and `is_neutral` to mean the > locking bits are in the prototype state. > > Using `is_fast_unlocked` could be an alternative to `is_neutral`, but > `is_neutral` captures the state better of being an object which is currently > not taking part in locking. However the name does not make it obvious that it > is referring to the locking state / mark state. Not 100% on this naming, and > how the comments and code which uses these constants in the MacroAssembler > should name and deal with this. > > Also cleaned up the C1 and C2 Valhalla header bits checks which were gated on > the locking bits. There is not more displaced header so conditionally > checking the prototype header in the Klass* is not needed. > > Testing (in progress): > * Tier 1-5 Oracle supported platforms > * GHA > > --------- > - [x] I confirm that I make this contribution in accordance with the [OpenJDK > Interim AI Policy](https://openjdk.org/legal/ai). First, I thought that the rename is_unlocked -> is_neutral made sense, but the more I read the code, the more I find it less readable to have checks for "is_neutral". Maybe if we remove the `is_neutral` checks, that would be a little bit better, but I think I need to mull over this. src/hotspot/share/interpreter/interpreterRuntime.cpp line 803: > 801: // The object could become unlocked through a JNI call, which we have > no other checks for. > 802: // Give a fatal message if CheckJNICalls. Otherwise we ignore it. > 803: if (obj->is_neutral()) { The comment above talks about unlocked, but the code doesn't use that word, so the apparent connection between the comment and the code is lost. You might need to go with some comment changes here. src/hotspot/share/oops/markWord.hpp line 205: > 203: > 204: // lock accessors (note that these assume lock_shift == 0) > 205: bool is_neutral() const { The comment above is not fully accurate anymore. src/hotspot/share/oops/markWord.hpp line 250: > 248: markWord clear_lock_bits() const { return markWord(value() & > ~lock_mask_in_place); } > 249: > 250: // age operations Pre-exiting: This comment is at the wrong location. The age functions are below. src/hotspot/share/oops/oop.hpp line 265: > 263: > 264: // locking operations > 265: inline bool is_neutral() const; Comment needs to be adjusted. src/hotspot/share/opto/graphKit.cpp line 3926: > 3924: Node* mark_adr = basic_plus_adr(obj, oopDesc::mark_offset_in_bytes()); > 3925: Node* mark = make_load(nullptr, mark_adr, TypeX_X, > TypeX_X->basic_type(), MemNode::unordered); > 3926: if (check_lock && !UseCompactObjectHeaders) { I thought we fixed the Valhalla code to check for the object monitor table here. src/hotspot/share/runtime/sharedRuntime.cpp line 2104: > 2102: // The object could become unlocked through a JNI call, which we have > no other checks for. > 2103: // Give a fatal message if CheckJNICalls. Otherwise we ignore it. > 2104: if (obj->is_neutral()) { Doesn't match comment (like the other, similar comment) src/hotspot/share/runtime/synchronizer.cpp line 731: > 729: } > 730: > 731: // Neutral case, header in place Is the "header in place" part helpful for the reader? src/hotspot/share/runtime/synchronizer.cpp line 1789: > 1787: > 1788: markWord mark = object->mark(); > 1789: assert(mark.is_fast_locked() || mark.has_monitor(), "must be"); It's almost like we should have a function named `is_locked()` ... src/hotspot/share/runtime/synchronizer.cpp line 1893: > 1891: > 1892: markWord mark = object->mark_acquire(); > 1893: assert(mark.is_fast_locked() || mark.has_monitor(), "Cannot be > neutral"); Comment and assert doesn't match that well anymore ------------- PR Review: https://git.openjdk.org/jdk/pull/32544#pullrequestreview-5033919335 PR Review Comment: https://git.openjdk.org/jdk/pull/32544#discussion_r3865696642 PR Review Comment: https://git.openjdk.org/jdk/pull/32544#discussion_r3865715139 PR Review Comment: https://git.openjdk.org/jdk/pull/32544#discussion_r3865721498 PR Review Comment: https://git.openjdk.org/jdk/pull/32544#discussion_r3865726679 PR Review Comment: https://git.openjdk.org/jdk/pull/32544#discussion_r3865747666 PR Review Comment: https://git.openjdk.org/jdk/pull/32544#discussion_r3865765621 PR Review Comment: https://git.openjdk.org/jdk/pull/32544#discussion_r3865771282 PR Review Comment: https://git.openjdk.org/jdk/pull/32544#discussion_r3865784515 PR Review Comment: https://git.openjdk.org/jdk/pull/32544#discussion_r3865788499
