RFR(L): 8235795: replace monitor list mux{Acquire,Release}(&gListLock) with spin locks

Fri Jan 31 01:22:08 UTC 2020

Dan,

http://cr.openjdk.java.net/~dcubed/8235795-webrev/1-for-jdk15.full/src/hotspot/share/runtime/objectMonitor.hpp.udiff.html

There should be three functions in ObjectMonitor like:

ObjectMonitor* next_om() const { Atomic::load(&_next_om); }
void set_next_om(ObjectMonitor* value) { Atomic::store(&_next_om, value); }
and one for the CAS.

And _next_om should be private.  That way all accesses to this field 
don't need checking (ie. eyeball checking).

http://cr.openjdk.java.net/~dcubed/8235795-webrev/1-for-jdk15.full/src/hotspot/share/runtime/synchronizer.cpp.sdiff.html

129 ObjectMonitor* free_list;

These fields should be prepended with _ ie. _free_list to follow the 
coding standard.   I would rename LVars to something like _om_globals 
like _om_globals._free_list .  I think LVars is an odd name without any 
meaning.

230 // Set the next field in an ObjectMonitor to the specified value.
231 static void set_next(ObjectMonitor* om, ObjectMonitor* value) {
232 Atomic::store(&om->_next_om, value);
233 }
234

the callers of this can just call om->set_next_om(value);
I almost think you don't need om in the names and have the functions be 
next() and set_next()

+ if (!try_om_lock(tail)) {
+ continue; // failed to lock tail so try it all again
+ }

Why would 'tail' be locked here?

'list' is some safe sublist that doesn't change or isn't iterated on, 
while being prepended to the list_p, right?  I was trying to understand 
the callers, but a comment would be helpful about what "list" is and 
where it's coming from.

+ prepend_list_to_common(new_blk + 1, &new_blk[_BLOCKSIZE - 1], 
_BLOCKSIZE - 1,

Commentary: this is just awful.    (This leads to more questions but not 
about this patch).

+ if (is_MonitorBound_exceeded(Atomic::load(&LVars.population) - 
Atomic::load(&LVars.free_count))) {

Since these are globals, add them to your is_MonitorBound_exceeded 
functions.

I just hit a big block of code in om_release to look at but have to go, 
but here's the first part.

Coleen

On 1/29/20 1:14 PM, Daniel D. Daugherty wrote:
> Ping! Still looking for a second reviewer on this changeset...
>
> Dan
>
>
> On 1/27/20 3:43 PM, Daniel D. Daugherty wrote:
>> Greetings,
>>
>> I'm looking for a second reviewer on this thread. I've gone ahead and
>> made changes based on David H's comments on CR0.
>>
>>     JDK-8235795 replace monitor list mux{Acquire,Release}(&gListLock) 
>> with spin locks
>>     https://bugs.openjdk.java.net/browse/JDK-8235795
>>
>> Copyright years will be updated when the patches are rebased to JDK15.
>>
>> Here's the incremental webrev URL:
>>
>> http://cr.openjdk.java.net/~dcubed/8235795-webrev/1-for-jdk15.inc/
>>
>> Here's the full webrev URL:
>>
>> http://cr.openjdk.java.net/~dcubed/8235795-webrev/1-for-jdk15.full/
>>
>> Here's what changed between CR0 and CR1:
>>
>>   - refactor common code
>>   - refactor atomic load of LVars.population in 
>> monitors_used_above_threshold
>>   - simplify list walking in ObjectSynchronizer::om_release() so we 
>> lock fewer ObjectMonitors
>>   - remove unnecessary locking from 
>> ObjectSynchronizer::deflate_monitor_list()
>>   - add NoSafepointVerifier helpers to main list management functions
>>   - remove unnecessary storestore()
>>   - remove unnecessary comments
>>   - clarify/fix comments.
>>
>> These changes have been tested in a Mach5 Tier[1-3] run with no
>> regressions. They have also been merged with 8235931 and 8236035 and
>> included in a Mach5 Tier[1-8] run with no known regressions (so far
>> since Tier8 is not quite finished).
>>
>> I did a SPECjbb2015 run on these bits with a jdk-14+32 baseline and 
>> 25 runs:
>>
>>     criticalJOPS                 0.25%  (Non-significant)
>>                    66754.32   66923.08
>>                   ± 1209.80  ± 1585.09
>>                              p = 0.674
>>
>>     maxJOPS                     -1.12%  (Non-significant)
>>                    90965.80   89948.80
>>                   ± 1788.39  ± 1989.22
>>                              p = 0.063
>>
>> I did a SPECjbb2015 run on the merge of 8235931, 8236035, and 8235795
>> with a jdk-14+32 baseline and 25 runs:
>>
>>     criticalJOPS                 0.37%  (Non-significant)
>>                    66754.32   67003.92
>>                   ± 1209.80  ± 1662.01
>>                              p = 0.547
>>
>>     maxJOPS                     -0.23%  (Non-significant)
>>                    90965.80   90754.00
>>                   ± 1788.39  ± 1851.64
>>                              p = 0.683
>>
>> All of these results were flagged as "Non-significant" by the perf
>> testing system. Looks like "p" values are still too high.
>>
>> Thanks, in advance, for comments, questions or suggestions.
>>
>> Dan
>>
>>
>> On 12/23/19 4:57 PM, Daniel D. Daugherty wrote:
>>> Greetings,
>>>
>>> I'm extracting another standalone fix from the Async Monitor Deflation
>>> project (JDK-8153224) and sending it out for review (and testing)
>>> separately.
>>>
>>>     JDK-8235795 replace monitor list 
>>> mux{Acquire,Release}(&gListLock) with spin locks
>>>     https://bugs.openjdk.java.net/browse/JDK-8235795
>>>
>>> Here's the webrev URL:
>>>
>>> http://cr.openjdk.java.net/~dcubed/8235795-webrev/0-for-jdk15/
>>>
>>> Folks that have reviewed JDK-8153224 will recognize these changes as
>>> a subset of the monitor list changes from the Async Monitor Deflation
>>> project. It's a subset because the Async Monitor Deflation project
>>> needs additional spin locking due to the async deflation work.
>>>
>>> The OpenJDK wiki for Async Monitor Deflation has several sections
>>> dedicated to the Spin-Lock Monitor List Management changes. This
>>> link will get you to the first section:
>>>
>>> Spin-Lock Monitor List Management In Theory
>>> https://wiki.openjdk.java.net/display/HotSpot/Async+Monitor+Deflation#AsyncMonitorDeflation-Spin-LockMonitorListManagementInTheory 
>>>
>>>
>>> The remaining monitor list sections are:
>>>
>>> Background: ObjectMonitor Movement Between the Lists
>>> https://wiki.openjdk.java.net/display/HotSpot/Async+Monitor+Deflation#AsyncMonitorDeflation-Background:ObjectMonitorMovementBetweentheLists 
>>>
>>>
>>> Spin-Lock Monitor List Management In Reality
>>> https://wiki.openjdk.java.net/display/HotSpot/Async+Monitor+Deflation#AsyncMonitorDeflation-Spin-LockMonitorListManagementInReality 
>>>
>>>
>>> Using The New Spin-Lock Monitor List Functions
>>> https://wiki.openjdk.java.net/display/HotSpot/Async+Monitor+Deflation#AsyncMonitorDeflation-UsingTheNewSpin-LockMonitorListFunctions 
>>>
>>>
>>> Of course, the OpenJDK wiki content is specific to the Async Monitor
>>> Deflation project, but this extract is a very close subset.
>>>
>>> These changes have been tested in various Mach5 Tier[1-7] runs.
>>> I'm also doing SPECjbb2015 runs.
>>>
>>> Thanks, in advance, for comments, questions or suggestions.
>>>
>>> Dan
>>>
>>
>