RFR: 8315884: New Object to ObjectMonitor mapping [v3]

Tue Jul 9 21:20:21 UTC 2024

On Mon, 8 Jul 2024 16:21:16 GMT, Axel Boldt-Christmas <aboldtch at 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