RFR(L) 8153224 Monitor deflation prolong safepoints

[Doerr, Martin]

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.

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.