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

[David Holmes]

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.

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

Hope that clarifies somewhat.

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