RFR(L): 8235795: replace monitor list mux{Acquire,Release}(&gListLock) with spin locks
David Holmes
david.holmes at oracle.com
Sun Feb 2 23:30:53 UTC 2020
Hi Dan,
Thanks for confirming the details. The comment is fine as-is.
David
On 31/01/2020 11:44 pm, Daniel D. Daugherty wrote:
> 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