On Mon, 8 Jul 2024 16:21:16 GMT, Axel Boldt-Christmas <abold...@openjdk.org>
wrote:
>> When inflating a monitor the `ObjectMonitor*` is written directly over the
>> `markWord` and any overwritten data is displaced into a displaced
>> `markWord`. This is problematic for concurrent GCs which needs extra care or
>> looser semantics to use this displaced data. In Lilliput this data also
>> contains the klass forcing this to be something that the GC has to take into
>> account everywhere.
>>
>> This patch introduces an alternative solution where locking only uses the
>> lock bits of the `markWord` and inflation does not override and displace the
>> `markWord`. This is done by keeping associations between objects and
>> `ObjectMonitor*` in an external hash table. Different caching techniques are
>> used to speedup lookups from compiled code.
>>
>> A diagnostic VM option is introduced called `UseObjectMonitorTable`. It is
>> only supported in combination with the LM_LIGHTWEIGHT locking mode (the
>> default).
>>
>> This patch has been evaluated to be performance neutral when
>> `UseObjectMonitorTable` is turned off (the default).
>>
>> Below is a more detailed explanation of this change and how `LM_LIGHTWEIGHT`
>> and `UseObjectMonitorTable` works.
>>
>> # Cleanups
>>
>> Cleaned up displaced header usage for:
>> * BasicLock
>> * Contains some Zero changes
>> * Renames one exported JVMCI field
>> * ObjectMonitor
>> * Updates comments and tests consistencies
>>
>> # Refactoring
>>
>> `ObjectMonitor::enter` has been refactored an a
>> `ObjectMonitorContentionMark` witness object has been introduced to the
>> signatures. Which signals that the contentions reference counter is being
>> held. More details are given below in the section about deflation.
>>
>> The initial purpose of this was to allow `UseObjectMonitorTable` to interact
>> more seamlessly with the `ObjectMonitor::enter` code.
>>
>> _There is even more `ObjectMonitor` refactoring which can be done here to
>> create a more understandable and enforceable API. There are a handful of
>> invariants / assumptions which are not always explicitly asserted which
>> could be trivially abstracted and verified by the type system by using
>> similar witness objects._
>>
>> # LightweightSynchronizer
>>
>> Working on adapting and incorporating the following section as a comment in
>> the source code
>>
>> ## Fast Locking
>>
>> CAS on locking bits in markWord.
>> 0b00 (Fast Locked) <--> 0b01 (Unlocked)
>>
>> When locking and 0b00 (Fast Locked) is observed, it may be beneficial to
>> avoid inflating by spinning a bit.
>>
>> If 0b10 (Inflated) is observed or there is to...
>
> Axel Boldt-Christmas has updated the pull request incrementally with two
> additional commits since the last revision:
>
> - Add JVMCI symbol exports
> - Revert "More graceful JVMCI VM option interaction"
>
> This reverts commit 2814350370cf142e130fe1d38610c646039f976d.
This is really great work, Axel! I've been reading this code for a while, and
have done one pass looking through the PR with a few comments.
src/hotspot/share/opto/library_call.cpp line 4620:
> 4618: Node *unlocked_val = _gvn.MakeConX(markWord::unlocked_value);
> 4619: Node *chk_unlocked = _gvn.transform(new
> CmpXNode(lmasked_header, unlocked_val));
> 4620: Node *test_not_unlocked = _gvn.transform(new
> BoolNode(chk_unlocked, BoolTest::ne));
I don't really know what this does. Someone from the c2 compiler group should
look at this.
src/hotspot/share/runtime/arguments.cpp line 1830:
> 1828: FLAG_SET_CMDLINE(LockingMode, LM_LIGHTWEIGHT);
> 1829: warning("UseObjectMonitorTable requires LM_LIGHTWEIGHT");
> 1830: }
Maybe we want this to have the opposite sense - turn off UseObjectMonitorTable
if not LM_LIGHTWEIGHT?
src/hotspot/share/runtime/javaThread.inline.hpp line 258:
> 256: }
> 257:
> 258: _om_cache.clear();
This could be shorter, ie: if (UseObjectMonitorTable) _om_cache.clear();
I think the not having an assert was to make the caller unconditional, which is
good.
src/hotspot/share/runtime/lightweightSynchronizer.cpp line 393:
> 391:
> 392: ObjectMonitor* LightweightSynchronizer::get_or_insert_monitor(oop
> object, JavaThread* current, const ObjectSynchronizer::InflateCause cause,
> bool try_read) {
> 393: assert(LockingMode == LM_LIGHTWEIGHT, "must be");
This assert should be assert(UseObjectMonitorTable not LM_LIGHTWEIGHT).
src/hotspot/share/runtime/lightweightSynchronizer.cpp line 732:
> 730:
> 731: markWord mark = object->mark();
> 732: assert(!mark.is_unlocked(), "must be unlocked");
"must be locked" makes more sense.
src/hotspot/share/runtime/lightweightSynchronizer.cpp line 763:
> 761: assert(mark.has_monitor(), "must be");
> 762: // The monitor exists
> 763: ObjectMonitor* monitor = ObjectSynchronizer::read_monitor(current,
> object, mark);
This looks in the table for the monitor in UseObjectMonitorTable, but could it
first check the BasicLock? Or we got here because BasicLock.metadata was not
the ObjectMonitor?
src/hotspot/share/runtime/lightweightSynchronizer.cpp line 773:
> 771: }
> 772:
> 773: ObjectMonitor* LightweightSynchronizer::inflate_locked_or_imse(oop obj,
> const ObjectSynchronizer::InflateCause cause, TRAPS) {
I figured out at one point why we now check IMSE here but now cannot remember.
Can you add a comment why above this function?
-------------
PR Review: https://git.openjdk.org/jdk/pull/20067#pullrequestreview-2167461168
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1671214948
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1671216649
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1671220251
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1671225452
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1671229697
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1671231155
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1671231863
