RFR(L): 8235795: replace monitor list mux{Acquire,Release}(&gListLock) with spin locks
coleen.phillimore at oracle.com
coleen.phillimore at oracle.com
Mon Feb 3 19:25:57 UTC 2020
More!
On 1/31/20 9:08 PM, Daniel D. Daugherty wrote:
> 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);
>
Great.
>
>> 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...
I found one remaining use of muxAcquire in this code, so we should see
if that can become PlatformMonitor too, when somebody looks tat JDK-8225631.
>
>
>> 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()/
Yes, I keep seeing and saying malloc to myself.
>
> 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...
>
Moving adding it to the list, in the uncontended object header case,
moves it only a few instructions down. It can't make any difference in
performance.
So the reason I suggested this change was so I wouldn't have to run
through the race scenarios in my head. I just read it and it seems
safe, particularly since the list is on the current thread, so you are
only racing with a concurrent walker not a concurrent thread also doing
addition and deletion. So leaving it is fine for this checkin but I'd
really like the code to be obviated by a simplification.
> 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...
>
If you're already in a safepoint, a NSV won't do anything because the
thread isn't going to go to a safepoint transition. But you don't need
to add one in inflate() if you've already tested without it. It might
be worth doing as a follow-up issue also.
>
>> 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
>
Awesome. There are a lot of public functions here that shouldn't be.
>
>> Thanks for walking me through this code and answering my questions
>> about how it worked.
>
> Thanks for taking the time to review this code!
You're welcome. Now I know it better too, so it's a win-win.
Coleen
>
> 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