RFR(L): 8235795: replace monitor list mux{Acquire,Release}(&gListLock) with spin locks
Daniel D. Daugherty
daniel.daugherty at oracle.com
Sat Feb 1 02:08:15 UTC 2020
Hi Coleen,
Thanks for the second set of comments on this review thread.
Replies embedded below...
On 1/31/20 2:36 PM, coleen.phillimore at oracle.com wrote:
> http://cr.openjdk.java.net/~dcubed/8235795-webrev/1-for-jdk15.full/src/hotspot/share/runtime/synchronizer.cpp.sdiff.html
>
>
> 1301 // CONSIDER: use muxTry() instead of muxAcquire().
>
> This comment is out of date.
Nice catch! I removed these two lines:
L1301: // CONSIDER: use muxTry() instead of muxAcquire().
L1302: // If the muxTry() fails then drop immediately into case 3.
I also removed these three lines:
L2060: // Safepoint logging cares about cumulative
per_thread_times and
L2061: // we'll capture most of the cost, but not the muxRelease()
which
L2062: // should be cheap.
which should have been removed with this line:
old L1735: Thread::muxRelease(&gListLock);
> After this change, we can fix JDK-8225631.
Yup. This bug (JDK-8235795) is linked to JDK-8225631 as a blocker.
JDK-8225631 was previously blocked by JDK-8153224, but now that
this work has been extracted from JDK-8153224 (at your request)
progress can be made without waiting for JDK-8153224 (Async
Monitor Deflation)...
Again, thanks for suggesting this extraction...
> 1395 // _next_om is used for both per-thread in-use and free lists so
> 1396 // we have to remove 'm' from the in-use list first (as needed).
> 1397 if (from_per_thread_alloc) {
> 1398 // Need to remove 'm' from om_in_use_list.
> ...
> 1467 }
>
>
> None of this code would be needed if om_malloc() doesn't add the
> monitor to the in-use list until after the header cas succeeds in
> ObjectSynchronizer::inflate(). You could make sure this code path is
> tested with a new develop flag StressMonitorInterference.
s/om_malloc()/om_alloc()/
For additional context, the above code is in om_release()...
Thanks for sending a webrev with a prototype of the above changes.
I'm planning to address this as a separate RFE.
It is definitely an interesting RFE that has the potential to
simplify things (without complications)...
I need to research why Dice thought that optimistically adding a
newly allocated ObjectMonitor to the in-use list had benefit...
I also need to look for any unintended side effects...
I filed this RFE:
JDK-8238370 ObjectMonitor::om_release() could be simplified
https://bugs.openjdk.java.net/browse/JDK-8238370
> inflate() should have a NSV also because it has a raw oop.
I added some NoSafepointVerifier helpers due to David H's review
of CR0. I didn't add one in inflate() because inflate_helper()
can cause inflate() to be called from strange places like
deoptimization... I'll have to do more investigation...
> The function om_malloc() and many of these are 'public' in
> ObjectSynchronizer when they're only called within. As a new RFE, can
> you make them private (or even static private if possible)?
s/om_malloc()/om_alloc()/
Definitely makes sense to make more stuff private and, if possible,
static private. This fix (8235795) includes a bit of that, but
there's more room for improvement.
I filed this RFE:
JDK-8238371 ObjectSynchronizer::om_alloc() does not have to be public
https://bugs.openjdk.java.net/browse/JDK-8238371
> Thanks for walking me through this code and answering my questions
> about how it worked.
Thanks for taking the time to review this code!
Dan
>
> Coleen
>
>>
>> On 1/29/20 1:14 PM, Daniel D. Daugherty wrote:
>>> Ping! Still looking for a second reviewer on this changeset...
>>>
>>> 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