On Tue, 13 Aug 2024 16:30:12 GMT, Axel Boldt-Christmas <[email protected]>
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
