RFR(L) 8153224 Monitor deflation prolong safepoints

[Daniel D. Daugherty]

On 4/5/19 8:07 AM, Robbin Ehn wrote:
> Hi Dan,
>
> (Martin there is question for you last in this email)
>
> After first pass I did not find any real issues.

Thanks for taking another pass thru the webrev...

> Considering what you had to work with, it looks good!

The base Java monitor code is showing its age... :-)


> #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?

Yeah... that's a little paranoid... will take care of that.


> 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?

Also a little paranoid... will take care of that...

> I think it's easier to read the code if some on the most obvious 
> asserts are removed. Maybe comments instead.

I'll take a pass at the end of the CR1 round and look at each of the new
asserts. If them seem to paranoid, I'll drop them


> #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");

I'd rather not make that change as part of this project. I'm likely
to do another cleanup subtask related to the _count field discussion
from the design review. I could see looking at TRAPS then...


> #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?

So what code are you saying we can switch to for this project?

Dan


>
> 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.