RFR(L) 8153224 Monitor deflation prolong safepoints
Daniel D. Daugherty
daniel.daugherty at oracle.com
Tue Apr 9 16:13:51 UTC 2019
Martin,
I added this comment to JDK-8222034. Hopefully Erik O will
chime in there since he wrote the original code...
Dan
On 4/8/19 12:23 PM, Doerr, Martin wrote:
> Hi Dan,
>
> thanks for addressing this issue. I appreciate it.
>
> I wonder if the comments are correct. Does dec_nested_handle_cnt really only need MO_ACQ_REL while inc_nested_handle_cnt needs MO_SEQ_CST?
> I don't see comments explaining what was intended to get ordered.
>
> I guess we can just use memory_order_conservative (default). Shouldn't be performance critical.
>
> Best regards,
> Martin
>
>
> -----Original Message-----
> From: Daniel D. Daugherty <daniel.daugherty at oracle.com>
> Sent: Freitag, 5. April 2019 18:10
> To: Doerr, Martin <martin.doerr at sap.com>; Robbin Ehn <robbin.ehn at oracle.com>; hotspot-runtime-dev at openjdk.java.net; Carsten Varming <varming at gmail.com>; Roman Kennke <rkennke at redhat.com>
> Subject: Re: RFR(L) 8153224 Monitor deflation prolong safepoints
>
> Filed:
>
> JDK-8222034 Thread-SMR functions should be updated to remove work
> around
> https://bugs.openjdk.java.net/browse/JDK-8222034
>
> Martin and Robbin, please check it out and make sure that I captured
> things correctly...
>
> Dan
>
>
>
> On 4/5/19 12:01 PM, Daniel D. Daugherty wrote:
>> On 4/5/19 8:37 AM, Doerr, Martin wrote:
>>> Hi everybody,
>>>
>>>> I think was fixed with:
>>>> 8202080: Introduce ordering semantics for Atomic::add/inc and other
>>>> RMW atomics
>>>> You should get a leading sync and trailing one with the default
>>>> conservative
>>>> model and thus get proper memory ordering.
>>>> Martin, I'm I correct?
>>> Exactly. Thanks for pointing this out. PPC uses the strongest
>>> possible ordering semantics with memory_order_conservative (default
>>> parameter).
>>> I've seen that comment about PPC in "void
>>> ThreadsList::inc_nested_handle_cnt()". This function could get replaced.
>> Okay so we need a new bug to update these two Thread-SMR functions:
>>
>> src/hotspot/share/runtime/threadSMR.cpp:
>>
>> void ThreadsList::dec_nested_handle_cnt() {
>> // The decrement needs to be MO_ACQ_REL. At the moment, the Atomic::dec
>> // backend on PPC does not yet conform to these requirements. Therefore
>> // the decrement is simulated with an Atomic::sub(1, &addr).
>> // Without this MO_ACQ_REL Atomic::dec simulation, the nested SMR
>> mechanism
>> // is not generally safe to use.
>> Atomic::sub(1, &_nested_handle_cnt);
>> }
>>
>> void ThreadsList::inc_nested_handle_cnt() {
>> // The increment needs to be MO_SEQ_CST. At the moment, the Atomic::inc
>> // backend on PPC does not yet conform to these requirements. Therefore
>> // the increment is simulated with a load phi; cas phi + 1; loop.
>> // Without this MO_SEQ_CST Atomic::inc simulation, the nested SMR
>> mechanism
>> // is not generally safe to use.
>> intx sample = OrderAccess::load_acquire(&_nested_handle_cnt);
>> for (;;) {
>> if (Atomic::cmpxchg(sample + 1, &_nested_handle_cnt, sample) ==
>> sample) {
>> return;
>> } else {
>> sample = OrderAccess::load_acquire(&_nested_handle_cnt);
>> }
>> }
>> }
>>
>> I'll file a new bug, loop in Robbin, Erik O and Martin, and make
>> sure we're all in agreement. Once we decide that Thread-SMR's
>> functions look like, I'll adapt my Async Monitor Deflation
>> functions...
>>
>> Dan
>>
>>
>>> Best regards,
>>> Martin
>>>
>>>
>>> -----Original Message-----
>>> From: Robbin Ehn <robbin.ehn at oracle.com>
>>> Sent: Freitag, 5. April 2019 14:07
>>> To: daniel.daugherty at oracle.com;
>>> hotspot-runtime-dev at openjdk.java.net; Carsten Varming
>>> <varming at gmail.com>; Roman Kennke <rkennke at redhat.com>; Doerr, Martin
>>> <martin.doerr at sap.com>
>>> Subject: Re: RFR(L) 8153224 Monitor deflation prolong safepoints
>>>
>>> Hi Dan,
>>>
>>> (Martin there is question for you last in this email)
>>>
>>> After first pass I did not find any real issues.
>>> Considering what you had to work with, it looks good!
>>>
>>> #1
>>> There are some assert which are redundant (to me at least) like:
>>> src/hotspot/share/runtime/objectMonitor.cpp
>>> L445
>>> if (!dmw->is_marked() && dmw->hash() == 0) {
>>> // This dmw is neutral and has not yet started the restoration
>>> // protocol so we mark a copy of the dmw to begin the protocol.
>>> markOop marked_dmw = dmw->set_marked();
>>> assert(marked_dmw->is_marked() && marked_dmw->hash() == 0,
>>> "sanity_check: is_marked=%d, hash=" INTPTR_FORMAT,
>>> marked_dmw->is_marked(), marked_dmw->hash());
>>>
>>> That assert is basically a test that set_marked worked?
>>>
>>> L505
>>> if (Atomic::cmpxchg(Self, &_owner, DEFLATER_MARKER) ==
>>> DEFLATER_MARKER) {
>>> assert(_succ != Self, "invariant");
>>> assert(_owner == Self, "invariant");
>>>
>>> Assert on _owner checks that our cmpxchg is not broken?
>>>
>>> I think it's easier to read the code if some on the most obvious
>>> asserts are
>>> removed. Maybe comments instead.
>>>
>>> #2
>>> Not your doing but I think we should remove TRAPS/Thread * Self and use
>>> JavaThread* instead.
>>> E.g. so we can change:
>>> void ObjectMonitor::EnterI(TRAPS) {
>>> Thread * const Self = THREAD;
>>> assert(Self->is_Java_thread(), "invariant");
>>> assert(((JavaThread *) Self)->thread_state() == _thread_blocked,
>>> "invariant");
>>>
>>> to:
>>>
>>> void ObjectMonitor::EnterI(JavaThread* Self) {
>>> assert(Self->thread_state() == _thread_blocked, "invariant");
>>>
>>> #3
>>> src/hotspot/share/runtime/objectMonitor.inline.hpp
>>> 164 inline void ObjectMonitor::inc_ref_count() {
>>> 165 // The increment needs to be MO_SEQ_CST. At the moment, the
>>> Atomic::inc
>>> 166 // backend on PPC does not yet conform to these
>>> requirements. Therefore
>>> 167 // the increment is simulated with a load phi; cas phi + 1;
>>> loop.
>>> 168 // Without this MO_SEQ_CST Atomic::inc simulation,
>>> AsyncDeflateIdleMonitors
>>> 169 // is not safe.
>>>
>>> I think was fixed with:
>>> 8202080: Introduce ordering semantics for Atomic::add/inc and other
>>> RMW atomics
>>> You should get a leading sync and trailing one with the default
>>> conservative
>>> model and thus get proper memory ordering.
>>> Martin, I'm I correct?
>>>
>>> Thanks, Robbin
>>>
>>> On 3/24/19 2:57 PM, Daniel D. Daugherty wrote:
>>>> Greetings,
>>>>
>>>> Welcome to the OpenJDK review thread for my port of Carsten's work on:
>>>>
>>>> JDK-8153224 Monitor deflation prolong safepoints
>>>> https://bugs.openjdk.java.net/browse/JDK-8153224
>>>>
>>>> Here's a link to the OpenJDK wiki that describes my port:
>>>>
>>>> https://wiki.openjdk.java.net/display/HotSpot/Async+Monitor+Deflation
>>>>
>>>> Here's the webrev URL:
>>>>
>>>> http://cr.openjdk.java.net/~dcubed/8153224-webrev/3-for-jdk13/
>>>>
>>>> Here's a link to Carsten's original webrev:
>>>>
>>>> http://cr.openjdk.java.net/~cvarming/monitor_deflate_conc/0/
>>>>
>>>> Earlier versions of this patch have been through several rounds of
>>>> preliminary review. Many thanks to Carsten, Coleen, Robbin, and
>>>> Roman for their preliminary code review comments. A very special
>>>> thanks to Robbin and Roman for building and testing the patch in
>>>> their own environments (including specJBB2015).
>>>>
>>>> This version of the patch has been thru Mach5 tier[1-8] testing on
>>>> Oracle's usual set of platforms. Earlier versions have been run
>>>> through my stress kit on my Linux-X64 and Solaris-X64 servers
>>>> (product, fastdebug, slowdebug).Earlier versions have run Kitchensink
>>>> for 12 hours on MacOSX, Linux-X64 and Solaris-X64 (product, fastdebug
>>>> and slowdebug). Earlier versions have run my monitor inflation stress
>>>> tests for 12 hours on MacOSX, Linux-X64 and Solaris-X64 (product,
>>>> fastdebug and slowdebug).
>>>>
>>>> All of the testing done on earlier versions will be redone on the
>>>> latest version of the patch.
>>>>
>>>> Thanks, in advance, for any questions, comments or suggestions.
>>>>
>>>> Dan
>>>>
>>>> P.S.
>>>> One subtest in gc/g1/humongousObjects/TestHumongousClassLoader.java
>>>> is currently failing in -Xcomp mode on Win* only. I've been trying
>>>> to characterize/analyze this failure for more than a week now. At
>>>> this point I'm convinced that Async Monitor Deflation is aggravating
>>>> an existing bug. However, I plan to have a better handle on that
>>>> failure before these bits are pushed to the jdk/jdk repo.
>>
More information about the hotspot-runtime-dev
mailing list