RFR(L) 8153224 Monitor deflation prolong safepoints (CR8/v2.08/11-for-jdk14)

[Daniel D. Daugherty]

Hi David,

On 11/12/19 6:12 PM, David Holmes wrote:
> Hi Dan,
>
> On 13/11/2019 8:24 am, Daniel D. Daugherty wrote:
>> Greetings,
>>
>> I'm only going to jump in on a single item here (at this time).
>>
>>
>> On 11/11/19 9:03 AM, David Holmes wrote:
>>> Hi Robbin,
>>>
>>> On 11/11/2019 10:41 pm, Robbin Ehn wrote:
>>>>
>>>> Also in this patch there is already Atomic::store/load on "volatile 
>>>> markWord _header;".
>>>
>>> And I've flagged the inappropriateness of using these with Dan. 
>>> Though I see we already have a couple of pre-existing occurrences 
>>> which have snuck in - again this seems to be a misunderstanding 
>>> about the need for Atomic use in these cases.
>>
>> I read this comment from Robbin and David's reply and my brain said: 
>> What?
>> It might have been just the common three letter acronym, but I 
>> digress... :-)
>
> First let me clarify that my comment quoted above may have been too 
> broad/general. I wasn't recalling specific changes where you added 
> Atomic::load/store but email exchanges where you said words to the 
> effect of "I could replace ... with Atomic::store ..." and I replied 
> that there was no need to use Atomic::load/store.

Actually it was Robbin that said the change was in my patch... and it is...
I just wanted to clarify that it was due to code motion. :-)

I do remember your general comments about Atomic::load/store. Of course,
because of those comments, I was planning to ping you about the use of
Atomic::load/store with the _header field. Mostly because I hadn't done
that with all of the new volatiles I added... So this sub-thread in the
8153224 review provided the perfect opportunity for me to chase that
little to-do item to ground...


>
>> So I searched the patch:
>
> Thanks for digging that out, as I hadn't recalled those details.
>
> Short version: I've agreed with Robbin that we should move to use 
> Atomic::load/store to get compiler-based-atomicity rather than relying 
> on use of "volatile" on variables. But for the purposes of this patch 
> (where Robbin made a number of suggestions on where to use 
> Atomic::load/store) that we try to limit using this new style to 
> inherently new code (ie lock-free list management) rather than 
> retrofitting all existing usages.

Agreed. I have a bit of work to do there to address all of Robbin's
comments. We'll see if I can get it right in the next round...


> Hope that clarifies somewhat.

It does. Thanks! I'll ping you, Robbin and Erik off thread if I need
any clarifications...

Dan


>
> Thanks,
> David
> -----
>
>> $ grep _header 11-for-jdk14.v2.08.full/open.patch | egrep 'store|load'
>>     assert(Atomic::load(&_header).value() != 0, "must be non-zero");
>> +  Atomic::store(markWord::zero(), &_header);
>> -  Atomic::store(markWord::zero(), &_header);
>>
>> Oh... that code... If you look at the 8153224 webrev you'll see that the
>> ObjectMonitor::clear() function was refactored into two parts:
>>
>>      ObjectMonitor::clear()
>>      ObjectMonitor::clear_using_JT()
>>
>> This line:
>>
>>      Atomic::store(markWord::zero(), &_header);
>>
>> is in the original ObjectMonitor::clear() function and it is in
>> the new ObjectMonitor::clear() function, but there's a lot of
>> code motion in between those two functions so... diff took the
>> easy way out and showed this:
>>
>>      +  Atomic::store(markWord::zero(), &_header);
>>
>> as a new line in the shorter, new clear() function
>> and as a deleted line in the longer, old clear() function:
>>
>>      -  Atomic::store(markWord::zero(), &_header);
>>
>>
>> Okay, so where did that come from? I've been tweaking a lot of
>> ObjectMonitor code lately, but I don't think that line is mine...
>>
>> $ hg annot src/hotspot/share/runtime/objectMonitor.inline.hpp | grep 
>> 'Atomic::store(markWord::zero(), &_header);'
>> 56006:   Atomic::store(markWord::zero(), &_header);
>>
>> $ hg log -r 56006
>> changeset:   56006:90ead0febf56
>> user:        stefank
>> date:        Tue Aug 06 10:48:21 2019 +0200
>> summary:     8229258: Rework markOop and markOopDesc into a simpler 
>> mark word value carrier
>>
>> Okay, I remember this bug:
>>
>> https://bugs.openjdk.java.net/browse/JDK-8229258
>>
>> and I even reviewed it... :-)  It looks like the reviewers are:
>>
>>  > Reviewed-by: rkennke, coleenp, kbarrett, dcubed
>>
>>
>> Roman posted this comment during the 8229258 review:
>>
>> On 8/15/19 1:06 PM, Roman Kennke wrote:
>>
>>> Out of curiosity, what's with the changes in 
>>> objectMonitor.inline.hpp to
>>> access the markWord atomically?:
>>>
>>> -inline markOop ObjectMonitor::header() const {
>>> -  return _header;
>>> +inline markWord ObjectMonitor::header() const {
>>> +  return Atomic::load(&_header);
>>>   }
>>>
>>> I guess this is good (equal or stronger than before) but is there a
>>> rationale behind these changes?
>>
>> and Stefan K replied with this:
>>
>> On 8/15/19 3:26 PM, Stefan Karlsson wrote:
>>
>>> Ahh. Right. That was done to solve the problems I were having with 
>>> volatiles. For example:
>>> src/hotspot/share/runtime/objectMonitor.inline.hpp:38:10: error: 
>>> binding reference of type 'const markWord&' to 'const volatile 
>>> markWord' discards qualifiers
>>>    return _header;
>>>
>>> and:
>>> src/hotspot/share/runtime/basicLock.hpp:40:74: error: implicit 
>>> dereference will not access object of type ‘volatile markWord’ in 
>>> statement [-Werror]
>>>   void         set_displaced_header(markWord header) { 
>>> _displaced_header = header; }
>>>
>>> Kim suggested that the fact that these fields were volatile was an 
>>> indication that we should be doing some kind of atomic/ordered 
>>> operation. By replacing these loads and stores with calls to the 
>>> Atomic APIs, and providing the 
>>> PrimitiveConversions::Translate<markWord> specialization, we could 
>>> solve that problem. 
>>
>> So it appears that Stefan has a good rationale for making the
>> Atomic::load() and Atomic::store() changes with the _header field.
>> Since I've added more volatile fields to ObjectMonitor, it would
>> follow that I should make similar changes...
>>
>> However, it's not clear that David agrees with the above change so
>> I'm hesitant to make the similar changes to my patch...
>>
>> How do we resolve this issue?
>>
>> Dan