RFR(L): 8235795: replace monitor list mux{Acquire,Release}(&gListLock) with spin locks
David Holmes
david.holmes at oracle.com
Thu Jan 30 04:30:07 UTC 2020
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.)
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