RFR(L) 8153224 Monitor deflation prolong safepoints

[Daniel D. Daugherty]

Just tracking to make sure I made all the changes...


On 4/5/19 11:54 AM, Daniel D. Daugherty wrote:
> On 4/5/19 8:07 AM, Robbin Ehn wrote:
>
>> #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.

Done. install_displaced_markword_in_object() comments and asserts
have been reworked quite a bit.


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

Deleted "assert(_owner == Self, "invariant")".


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

Will make this pass before I send out the next webrev...


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

The code that I copied from Thread-SMR has been fixed via:

     JDK-8222034 Thread-SMR functions should be updated to remove work 
around
https://bugs.openjdk.java.net/browse/JDK-8222034

I've updated ObjectMonitor::dec_ref_count() and inc_ref_count()
to use the simpler Atomic::dec() and Atomic::inc().


Okay, I believe I've made all the changes for this set of comments
and replies...

Dan