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

Daniel D. Daugherty daniel.daugherty at oracle.com
Fri Jan 31 13:44:33 UTC 2020


Hi David,

On 1/30/20 6:52 PM, David Holmes wrote:
> On 31/01/2020 12:06 am, Daniel D. Daugherty wrote:
>> 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...
>
> Sorry Dan, as I said trying to keep the code paths clear is tricky.

Agreed.


> IIUC om_release operates on a per-thread om-in-use list and removes an 
> OM from that list. There can only ever be one active call to 
> om_release for a given thread and thus a given om-in-use list. Some 
> other code (it doesn't matter exactly what) may be able to walk that 
> same om-in-use list but it doesn't mutate the list. The only other 
> mutation of a given om-in-use list is by deflation at a safepoint, and 
> by definition that can't be happening concurrently with this use of 
> om_release. Is that correct?

All of that is correct.

Do you have a suggestion for how to improve the comment?
(Keep in mind that the comment changes with the pending
Async Monitor Deflation changes in 8153224...)

Dan


>
> Thanks,
> David
>
>>
>> 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