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

Tue Aug 13 22:15:59 UTC 2024

On Tue, 13 Aug 2024 16:30:12 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 one additional commit since the last revision:
> 
>   Whitespace and nits

I finished my first pass crawl thru on these changes. I need to mull
on these changes a bit before I make another pass. I think there's
only one real bug buried in all of my comments.

src/hotspot/share/runtime/objectMonitor.cpp line 377:

> 375: 
> 376:   if (cur == current) {
> 377:     // TODO-FIXME: check for integer overflow!  BUGID 6557169.

Thanks for removing this comment. JDK-6557169 was closed as
"Will Not FIx" in 2017.

src/hotspot/share/runtime/synchronizer.cpp line 970:

> 968:   if (value == 0) value = 0xBAD;
> 969:   assert(value != markWord::no_hash, "invariant");
> 970:   return value;

In the elided part above this line, we have:

  if (value == 0) value = 0xBAD;
  assert(value != markWord::no_hash, "invariant");

so my memory about zero being returnable as a hash value is wrong.

src/hotspot/share/runtime/synchronizer.cpp line 977:

> 975: 
> 976:   markWord mark = obj->mark_acquire();
> 977:   for(;;) {

nit - please insert space before `(`

src/hotspot/share/runtime/synchronizer.cpp line 997:

> 995:   // Since the monitor isn't in the object header, it can simply be installed.
> 996:   if (UseObjectMonitorTable) {
> 997:     return install_hash_code(current, obj);

Perhaps:

  if (UseObjectMonitorTable) {
    // Since the monitor isn't in the object header, the hash can simply be
    // installed in the object header.
    return install_hash_code(current, obj);

src/hotspot/share/runtime/synchronizer.cpp line 1271:

> 1269:       _no_progress_cnt >= NoAsyncDeflationProgressMax) {
> 1270:     double remainder = (100.0 - MonitorUsedDeflationThreshold) / 100.0;
> 1271:     size_t new_ceiling = ceiling / remainder + 1;

Why was the `new_ceiling` calculation changed?
I think the `new_ceiling` value is going to lower than the old ceiling value.

src/hotspot/share/runtime/synchronizer.inline.hpp line 83:

> 81: 
> 82: 
> 83: #endif // SHARE_RUNTIME_SYNCHRONIZER_INLINE_HPP

nit - please delete one of the blank lines.

src/hotspot/share/runtime/vframe.cpp line 252:

> 250:           if (mark.has_monitor()) {
> 251:             ObjectMonitor* mon = ObjectSynchronizer::read_monitor(current, monitor->owner(), mark);
> 252:             if (// if the monitor is null we must be in the process of locking

nit - please add a space after `(`

test/hotspot/gtest/runtime/test_objectMonitor.cpp line 36:

> 34:     EXPECT_EQ(in_bytes(ObjectMonitor::metadata_offset()), 0)
> 35:          << "_metadata at a non 0 offset. metadata_offset = "
> 36:          << in_bytes(ObjectMonitor::metadata_offset());

nit - the indent should be four spaces instead of five spaces.

test/hotspot/gtest/runtime/test_objectMonitor.cpp line 40:

> 38:     EXPECT_GE((size_t) in_bytes(ObjectMonitor::owner_offset()), cache_line_size)
> 39:          << "the _metadata and _owner fields are closer "
> 40:          << "than a cache line which permits false sharing.";

nit - the indent should be four spaces instead of five spaces.

test/hotspot/gtest/runtime/test_objectMonitor.cpp line 44:

> 42:     EXPECT_GE((size_t) in_bytes(ObjectMonitor::recursions_offset() - ObjectMonitor::owner_offset()), cache_line_size)
> 43:          << "the _owner and _recursions fields are closer "
> 44:          << "than a cache line which permits false sharing.";

nit - the indent should be four spaces instead of five spaces.

test/hotspot/jtreg/runtime/Monitor/UseObjectMonitorTableTest.java line 148:

> 146:         static final int MAX_RECURSION_COUNT = 10;
> 147:         static final double RECURSION_CHANCE = .25;
> 148:         final Random random = new Random();

The test should output a seed value so that the user knows what
random seed value was in use if/when the test fails. Also add a way
to specify the seed value from the command line for reproducibility.

test/micro/org/openjdk/bench/vm/lang/LockUnlock.java line 201:

> 199: 
> 200:   /** Perform two synchronized after each other on the same object. */
> 201:     @Benchmark

Please align L200 with L201.

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

Changes requested by dcubed (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/20067#pullrequestreview-2234133776
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715917040
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715955877
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715957258
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715958513
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715965577
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715976433
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715978312
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715981270
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715981568
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715981881
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715986844
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715990141