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

[David Holmes]

Hi Dan,

On 28/01/2020 6:43 am, 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

I'm a little confused by this. Here are the comments:

1402     // This list walk can only race with another list walker since
1403     // deflation can only happen at a safepoint so we don't have to
1404     // worry about an ObjectMonitor being removed from this list
1405     // while we are walking it.
1406
1407     // Lock the list head to avoid racing with another list walker.

If we are at a safepoint how can we be racing with another list walker? 
It would have to be a non-JavaThread, but then why would it be walking 
the monitor lists?

>    - 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.

All the actual changes seem fine.

Thanks,
David
-----

> 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
>>
>