RFR(L): 8235795: replace monitor list mux{Acquire,Release}(&gListLock) with spin locks
Daniel D. Daugherty
daniel.daugherty at oracle.com
Mon Feb 3 15:18:48 UTC 2020
Thanks for closing the loop on this part.
Dan
On 2/2/20 6:30 PM, David Holmes wrote:
> 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