On Wed, 17 Jul 2024 06:35:34 GMT, David Holmes <dhol...@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
