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

[Daniel D. Daugherty]

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

So I searched the patch:

$ 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