RFR(L) 8153224 Monitor deflation prolong safepoints

Daniel D. Daugherty daniel.daugherty at oracle.com
Fri Apr 5 16:10:15 UTC 2019 

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