RFR(L): 8235795: replace monitor list mux{Acquire,Release}(&gListLock) with spin locks
coleen.phillimore at oracle.com
coleen.phillimore at oracle.com
Fri Jan 31 19:36:35 UTC 2020
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. After this change, we can fix 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. inflate()
should have a NSV also because it has a raw oop.
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)?
Thanks for walking me through this code and answering my questions about
how it worked.
Coleen
On 1/30/20 8:22 PM, coleen.phillimore at oracle.com wrote:
>
> Dan,
>
> http://cr.openjdk.java.net/~dcubed/8235795-webrev/1-for-jdk15.full/src/hotspot/share/runtime/objectMonitor.hpp.udiff.html
>
>
> There should be three functions in ObjectMonitor like:
>
> ObjectMonitor* next_om() const { Atomic::load(&_next_om); }
> void set_next_om(ObjectMonitor* value) { Atomic::store(&_next_om,
> value); }
> and one for the CAS.
>
> And _next_om should be private. That way all accesses to this field
> don't need checking (ie. eyeball checking).
>
> http://cr.openjdk.java.net/~dcubed/8235795-webrev/1-for-jdk15.full/src/hotspot/share/runtime/synchronizer.cpp.sdiff.html
>
>
> 129 ObjectMonitor* free_list;
>
>
> These fields should be prepended with _ ie. _free_list to follow the
> coding standard. I would rename LVars to something like _om_globals
> like _om_globals._free_list . I think LVars is an odd name without
> any meaning.
>
>
> 230 // Set the next field in an ObjectMonitor to the specified value.
> 231 static void set_next(ObjectMonitor* om, ObjectMonitor* value) {
> 232 Atomic::store(&om->_next_om, value);
> 233 }
> 234
>
> the callers of this can just call om->set_next_om(value);
> I almost think you don't need om in the names and have the functions
> be next() and set_next()
>
> + if (!try_om_lock(tail)) {
> + continue; // failed to lock tail so try it all again
> + }
>
>
> Why would 'tail' be locked here?
>
> 'list' is some safe sublist that doesn't change or isn't iterated on,
> while being prepended to the list_p, right? I was trying to
> understand the callers, but a comment would be helpful about what
> "list" is and where it's coming from.
>
> + prepend_list_to_common(new_blk + 1, &new_blk[_BLOCKSIZE - 1],
> _BLOCKSIZE - 1,
>
>
> Commentary: this is just awful. (This leads to more questions but
> not about this patch).
>
> + if (is_MonitorBound_exceeded(Atomic::load(&LVars.population) -
> Atomic::load(&LVars.free_count))) {
>
>
> Since these are globals, add them to your is_MonitorBound_exceeded
> functions.
>
> I just hit a big block of code in om_release to look at but have to
> go, but here's the first part.
>
> 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