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

[David Holmes]

On 5/11/2019 11:34 am, Daniel D. Daugherty wrote:
> I can do that. I assume delete them in the Async Monitor Deflation
> code and delete them in threadSMR.cpp. For the threadSMR.cpp, I'll
> roll that change into my latest baseline cleanup bug:
> 
>      JDK-8230876 baseline cleanups from Async Monitor Deflation v2.07
>      https://bugs.openjdk.java.net/browse/JDK-8230876

Sure.

Thanks,
David
-----

> Erik, please chime in here... Thanks!
> 
> Dan
> 
> 
> On 11/4/19 6:44 PM, David Holmes wrote:
>> Hi Dan,
>>
>> Just delete the comments.
>>
>> Thanks,
>> David
>>
>> On 5/11/2019 7:25 am, Daniel D. Daugherty wrote:
>>> Hi David and Erik,
>>>
>>> Thanks for chiming in here Erik...
>>>
>>> This set of comments is not addressed in the CR8/v2.08/11-for-jdk14
>>> code review request that I just sent out.
>>>
>>> I've read this response twice and I'm not quite sure what to do with it
>>> relative to David's CR comment. I'll repeat those here:
>>>
>>>  >  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!)
>>>
>>> Should I delete this comment? Or should it be changed? If changed, then
>>> what text do you recommend here?
>>>
>>>
>>>  > 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.
>>>
>>> Should I delete this comment? Or should it be changed? If changed, then
>>> what text do you recommend here?
>>>
>>> Dan
>>>
>>>
>>> On 11/4/19 8:09 AM, erik.osterlund at oracle.com wrote:
>>>> Hi,
>>>>
>>>> TL/DR: David is right; the commentary is weird and does not capture 
>>>> what the real constraints are.
>>>>
>>>> As the comment implied before "8222034: Thread-SMR functions should 
>>>> be updated to remove work around", the PPC port used to have 
>>>> incorrect memory ordering, and the code guarded against that. 
>>>> inc/dec used to be memory_order_relaxed and add/sub used to be 
>>>> memory_order_acq_rel on PPC, despite the shared contract promising 
>>>> memory_order_conservative.
>>>>
>>>> The implication for the nested counter in the Thread SMR project was 
>>>> that I wanted to use the inc/dec API but knew it was not gonna work 
>>>> as expected on PPC because we really needed *at least* 
>>>> memory_order_acq_rel when decrementing (and 
>>>> memory_order_conservative when incrementing, which was simulated in 
>>>> a CAS loop... yuck), but would find ourselves getting 
>>>> memory_order_relaxed. Rather than treating it as a bug in the PPC 
>>>> atomics implementation, and having the code be broken while we 
>>>> waited for a fix, I changed the use to sub when decrementing (which 
>>>> gave me the required memory_order_acq_rel ordering I needed), and 
>>>> the horrible CAS loop when incrementing, as a workaround, and 
>>>> alerted Martin Doerr that this would needed to be sorted out in the 
>>>> PPC code. Since then, the PPC code did indeed get cleaned up so that 
>>>> inc/dec stopped being relaxed-only and worked as advertised.
>>>>
>>>> After that, the "8222034: Thread-SMR functions should be updated to 
>>>> remove work around" change removed the workaround that was no longer 
>>>> required from the code, and put back the desired inc/dec calls 
>>>> (which now used an overly conservative memory_order_conservative 
>>>> ordering, which is suboptimal, in particular for decrements, but 
>>>> importantly not incorrect). Since the nested case would almost never 
>>>> run and is possibly the coldest code path in the VM, I did not care 
>>>> to comment in that review thread about optimizing it by explicitly 
>>>> passing in a weaker ordering. However, I should have commented on 
>>>> the comment that was changed, which does indeed look a bit confused. 
>>>> David is right that the stuff about volatile has nothing to do with 
>>>> why this is correct. The correctness required memory_order_acq_rel 
>>>> for decrements, but the implementation provided more, which is fine.
>>>>
>>>> The actual reason why I wanted memory_order_conservative for 
>>>> correctness when incrementing and memory_order_acq_rel when 
>>>> decrementing, was to prevent accesses inside of the critical section 
>>>> (in particular - reading Thread*s from the acquired ThreadsList), 
>>>> from floating outside of the reference increment and decrement that 
>>>> marks reading the list as safe to access without the underlying list 
>>>> blowing up. In practice, it might have been possible to relax it a 
>>>> bit by relying on side effects of other unrelated parts of the 
>>>> protocol to have spurious fencing... but I did not want to get the 
>>>> protocol tangled in that way because it would be difficult to reason 
>>>> about.
>>>>
>>>> Hope this explanation clears up that confusion.
>>>>
>>>> Thanks,
>>>> /Erik
>>>>
>>>> On 11/2/19 2:15 PM, Daniel D. Daugherty wrote:
>>>>> 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.
>>>>>
>>>>
>>>
>