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