RFR(L): 8235795: replace monitor list mux{Acquire,Release}(&gListLock) with spin locks

David Holmes david.holmes at oracle.com
Thu Jan 30 23:52:44 UTC 2020 

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. 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?

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