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

Mon Feb 3 19:56:03 UTC 2020

On 2/3/20 2:30 PM, coleen.phillimore at oracle.com wrote:
>
> http://cr.openjdk.java.net/~dcubed/8235795-webrev/2-for-jdk15.inc/src/hotspot/share/runtime/objectMonitor.inline.hpp.udiff.html 
>
>
> Can you add a little comment why you need Atomic::load() and store() 
> above these functions?
>
> // The _om_next field can be concurrently read and modified, so 
> disable optimizations that elide loading and storing this value.

I'll add something like above the new functions.

> I don't need to see another webrev.

Okay.

> Looks good to me.

Thanks for the fast re-review!

Dan

>
> Coleen
>
> On 2/2/20 9:53 AM, Daniel D. Daugherty wrote:
>> Greetings,
>>
>> I've made changes based on Coleen's comments on CR1. I'm looking for a
>> re-review by both David H. and Coleen. Of course, anyone else is welcome
>> to chime in on this review thread.
>>
>>     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/2-for-jdk15.inc/
>>
>> Here's the full webrev URL:
>>
>> http://cr.openjdk.java.net/~dcubed/8235795-webrev/2-for-jdk15.full/
>>
>> Here's what changed between CR1 and CR2:
>>
>>   - add ObjectMonitor::next_om(), set_next_om(), and try_set_next_om()
>>     and update direct uses of _next_om field to use them
>>   - ObjectMonitor::_next_om field is now private
>>   - rename ListGlobals -> ObjectMonitorListGlobals, rename LVars ->
>>     om_list_globals, and prefix each ObjectMonitorListGlobals field 
>> with '_'
>>   - delete static set_next() function
>>   - clarify comments
>>   - delete stale comments about mux*()
>>
>> These changes have been tested in a Mach5 Tier[1-3] run with no
>> failures. 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).
>>
>> Thanks, in advance, for comments, questions or suggestions.
>>
>> 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
>>>>
>>>
>>
>