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