RFR(L): 8235795: replace monitor list mux{Acquire,Release}(&gListLock) with spin locks
David Holmes
david.holmes at oracle.com
Mon Feb 3 02:30:51 UTC 2020
On 3/02/2020 12:53 am, Daniel D. Daugherty wrote:
> Greetings,
>
> I've made changes based on Coleen's comments on CR1. I'm looking for a
> re-review by both David H. and Coleen. Of course, anyone else is welcome
> to chime in on this review thread.
All looks fine to me.
Thanks,
David
-----
> 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/2-for-jdk15.inc/
>
> Here's the full webrev URL:
>
> http://cr.openjdk.java.net/~dcubed/8235795-webrev/2-for-jdk15.full/
>
> Here's what changed between CR1 and CR2:
>
> - add ObjectMonitor::next_om(), set_next_om(), and try_set_next_om()
> and update direct uses of _next_om field to use them
> - ObjectMonitor::_next_om field is now private
> - rename ListGlobals -> ObjectMonitorListGlobals, rename LVars ->
> om_list_globals, and prefix each ObjectMonitorListGlobals field
> with '_'
> - delete static set_next() function
> - clarify comments
> - delete stale comments about mux*()
>
> These changes have been tested in a Mach5 Tier[1-3] run with no
> failures. 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).
>
> Thanks, in advance, for comments, questions or suggestions.
>
> Dan
>
>
> On 1/27/20 3:43 PM, 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
>> - 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.
>>
>> 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