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

Daniel D. Daugherty daniel.daugherty at oracle.com
Thu Jan 30 14:06:05 UTC 2020 

On 1/29/20 11:30 PM, David Holmes wrote:
> Hi Dan,
>
> On 30/01/2020 12:08 pm, Daniel D. Daugherty wrote:
>> Hi David,
>>
>> Thanks for re-reviewing!
>>
>>
>> On 1/29/20 8:58 PM, David Holmes wrote:
>>> 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?
>>
>> om_release() is called by a JavaThread when we aren't at a safepoint.
>>
>> If we have added a debugging audit_and_print_stats() call that is
>> also executed at a non-safepoint, then we could have a race. Or if
>> we have an exit_globals() call that doesn't happen at a safepoint
>> like the one we discovered with the ExitOnFullCodeCache option...
>>
>> Just covering the possibilities...
>
> Okay, but in that case isn't the comment about not having to worry 
> about an OM being removed incorrect? (Sorry it's hard for me to keep 
> track of all the potential code paths here.)

The comment:

- acknowledges that this function can race with a list walker
- doesn't have to worry about deflation (a list modifier) because
   deflation only happens at a safepoint

I'm not sure how to make the comment any more clear here...

Dan


>
> Cheers,
> David
>
>>
>>>
>>>>    - 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 again for the re-review!!
>>
>> Dan
>>
>>
>>>
>>> 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
>>>>>
>>>>
>>

More information about the hotspot-runtime-dev mailing list