RFR(L) 8153224 Monitor deflation prolong safepoints (CR7/v2.07/10-for-jdk14)

[Daniel D. Daugherty]

Erik,

David H. made a comment during this review cycle that should interest you.

The longer version of this comment came up in early reviews of the Async
Monitor Deflation code because I copied the code and the longer comment
from threadSMR.cpp. I updated the comment based on your input and review
and changed the comment and code in threadSMR.cpp and in the Async Monitor
Deflation project code.

The change in threadSMR.cpp was done with this changeset:

$ hg log -v -r 54517
changeset:   54517:c201ca660afd
user:        dcubed
date:        Thu Apr 11 14:14:30 2019 -0400
files:       src/hotspot/share/runtime/threadSMR.cpp
description:
8222034: Thread-SMR functions should be updated to remove work around
Reviewed-by: mdoerr, eosterlund

Here's one of the two diffs to job your memory:

  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);
+  // The decrement only needs to be MO_ACQ_REL since the reference
+  // counter is volatile (and the hazard ptr is already NULL).
+  Atomic::dec(&_nested_handle_cnt);
  }

Below is David's comment about the code comment...

Dan


Trimming down to just that issue...

On 10/29/19 4:20 PM, Daniel D. Daugherty wrote:
> On 10/24/19 7:00 AM, David Holmes wrote:
 >
 > src/hotspot/share/runtime/objectMonitor.inline.hpp
>
>>  199 // The decrement only needs to be MO_ACQ_REL since the reference
>>  200   // counter is volatile.
>>  201   Atomic::dec(&_ref_count);
>>
>> volatile is irrelevant with regards to memory ordering as it is a 
>> compiler annotation. And you haven't specified any memory order value 
>> so the default is conservative ie. implied full fence. (I see the 
>> same incorrect comment is in threadSMR.cpp!)
>
> I got that wording from threadSMR.cpp and Erik O. confirmed my use of 
> that
> wording previously. I'll chase it down with Erik and get back to you.
>
>
>> 208   // The increment needs to be MO_SEQ_CST so that the reference
>>  209   // counter update is seen as soon as possible in a race with the
>>  210   // async deflation protocol.
>>  211   Atomic::inc(&_ref_count);
>>
>> Ditto you haven't specified any ordering - and inc() and dec() will 
>> have the same default.
>
> And again, I'll have to chase this down with Erik O. and get back to you.