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

[Coleen Phillimore]

On Wed, 17 Jul 2024 06:35:34 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> Axel Boldt-Christmas has updated the pull request incrementally with 10 additional commits since the last revision:
>> 
>>  - Remove try_read
>>  - Add explicit to single parameter constructors
>>  - Remove superfluous access specifier
>>  - Remove unused include
>>  - Update assert message OMCache::set_monitor
>>  - Fix indentation
>>  - Remove outdated comment LightweightSynchronizer::exit
>>  - Remove logStream include
>>  - Remove strange comment
>>  - Fix javaThread include
>
> src/hotspot/share/runtime/basicLock.hpp line 44:
> 
>> 42:   // a sentinel zero value indicating a recursive stack-lock.
>> 43:   // * For LM_LIGHTWEIGHT
>> 44:   // Used as a cache the ObjectMonitor* used when locking. Must either
> 
> The first sentence doesn't read correctly.

Suggestion:

  // Used as a cache of the ObjectMonitor* used when locking. Must either

> src/hotspot/share/runtime/deoptimization.cpp line 1641:
> 
>> 1639:               assert(fr.is_deoptimized_frame(), "frame must be scheduled for deoptimization");
>> 1640:               if (LockingMode == LM_LEGACY) {
>> 1641:                 mon_info->lock()->set_displaced_header(markWord::unused_mark());
> 
> In the existing code how is this restricted to the LM_LEGACY case?? It appears to be unconditional which suggests you are changing the non-UOMT LM_LIGHTWEIGHT logic. ??

Only legacy locking uses the displaced header, I believe, which isn't clear in this code at all.  This seems like a fix.  We should probably assert that only legacy locking uses this field as a displaced header.

> src/hotspot/share/runtime/lightweightSynchronizer.cpp line 62:
> 
>> 60: class ObjectMonitorWorld : public CHeapObj<MEMFLAGS::mtObjectMonitor> {
>> 61:   struct Config {
>> 62:     using Value = ObjectMonitor*;
> 
> Does this alias really help? We don't state the type that many times and it looks odd to end up with a mix of `Value` and `ObjectMonitor*` in the same code.

This alias is present in the other CHT implementations, alas as a typedef in StringTable and SymbolTable so this follows the pattern and allows cut/paste of the allocate_node, get_hash, and other functions.

> src/hotspot/share/runtime/lightweightSynchronizer.cpp line 102:
> 
>> 100:       assert(*value != nullptr, "must be");
>> 101:       return (*value)->object_is_cleared();
>> 102:     }
> 
> The `is_dead` functions seem oddly placed given they do not relate to the object stored in the wrapper. Why are they here? And what is the difference between `object_is_cleared` and `object_is_dead` (as used by `LookupMonitor`) ?

This is a good question.  When we look up the Monitor, we don't want to find any that the GC has marked dead, so that's why we call object_is_dead.   When we look up with the object to find the Monitor, the object won't be dead (since we're using it to look up).  But we don't want to find one that we've cleared because the Monitor was  deflated?  I don't see where we would clear it though.  We clear the WeakHandle in the destructor after the Monitor has been removed from the table.

> src/hotspot/share/runtime/lightweightSynchronizer.cpp line 105:
> 
>> 103:   };
>> 104: 
>> 105:   class LookupMonitor : public StackObj {
> 
> I'm not understanding why we need this little wrapper class.

It's a two way lookup.  The plain Lookup class is used to lookup the Monitor given the object.  This LookupMonitor class is used to lookup the object given the Monitor.  The CHT takes these wrapper classes.  Maybe we should rename LookupObject to be more clear?

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1688013308
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1688041218
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1688051557
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1688375335
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1688168626