On Mon, 15 Jul 2024 00:50:30 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 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
I have some suggestions that hopefully you can click on if you agree. Also,
some comments.
src/hotspot/share/runtime/lightweightSynchronizer.cpp line 67:
> 65: }
> 66: static void* allocate_node(void* context, size_t size, Value const&
> value) {
> 67: reinterpret_cast<ObjectMonitorWorld*>(context)->inc_table_count();
Suggestion:
reinterpret_cast<ObjectMonitorWorld*>(context)->inc_items_count();
src/hotspot/share/runtime/lightweightSynchronizer.cpp line 71:
> 69: };
> 70: static void free_node(void* context, void* memory, Value const&
> value) {
> 71: reinterpret_cast<ObjectMonitorWorld*>(context)->dec_table_count();
Suggestion:
reinterpret_cast<ObjectMonitorWorld*>(context)->dec_items_count();
src/hotspot/share/runtime/lightweightSynchronizer.cpp line 125:
> 123: };
> 124:
> 125: void inc_table_count() {
Suggestion:
void inc_items_count() {
src/hotspot/share/runtime/lightweightSynchronizer.cpp line 126:
> 124:
> 125: void inc_table_count() {
> 126: Atomic::inc(&_table_count);
Suggestion:
Atomic::inc(&_items_count);
src/hotspot/share/runtime/lightweightSynchronizer.cpp line 129:
> 127: }
> 128:
> 129: void dec_table_count() {
Suggestion:
void dec_items_count() {
src/hotspot/share/runtime/lightweightSynchronizer.cpp line 130:
> 128:
> 129: void dec_table_count() {
> 130: Atomic::inc(&_table_count);
Suggestion:
Atomic::inc(&_items_count);
src/hotspot/share/runtime/lightweightSynchronizer.cpp line 134:
> 132:
> 133: double get_load_factor() {
> 134: return (double)_table_count/(double)_table_size;
Suggestion:
return (double)_items_count/(double)_table_size;
-------------
PR Review: https://git.openjdk.org/jdk/pull/20067#pullrequestreview-2193868194
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1688563846
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1688563501
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1688565196
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1688565561
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1688565947
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1688566411
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1688566752
