RFR (S) 8182397: Race in field updates when creating ArrayKlasses can lead to crash

[Erik Osterlund]

Hi Andrew,

> On 21 Jul 2017, at 15:47, Andrew Dinn <adinn at redhat.com> wrote:
> 
> Hi Coleen,
> 
>> On 21/07/17 00:44, coleen.phillimore at oracle.com wrote:
>> Summary: Update array_klass field in component mirror after
>> klass.java_mirror field for concurrent readers in compiled code.
>> 
>> See bug for details.
>> 
>> open webrev at http://cr.openjdk.java.net/~coleenp/8182397.01/webrev
>> bug link https://bugs.openjdk.java.net/browse/JDK-8182397
>> 
>> Ran tier1 testing with -Xcomp and without.  Thank you to Tom for the
>> test and diagnosis.
> The re-ordering of the writes to
> 
>  k->set_java_mirror(mirror());
>  . . .
>  set_array_klass(comp_mirror(), k);
> 
> is all that is needed, assuming that the second write has release
> semantics. I discussed this with Andrew Haley and he confirmed my
> assumption (also Tom's) that on AArch64 there is no need for a load
> acquire on the reader side because of the address dependency between any
> load of the component mirror's array klass k and ensuing load of k's mirror.

Sorry for jumping in to the conversation a bit late.
Since David Holmes is on vacation, I have to take his place in questioning the elision of acquire on the reader side being okay. Because I know he would if he was here.

In hotspot we almost *never* elide the acquire on the reader side, relying on dependent loads. This would be an exception to that rule, and I do not see why this is warranted. In order to utilize the dependent load properties in shared code, we need memory ordering semantics for this. That memory ordering semantics is called consume in C++11, and has arguably turned out to be a terrible idea. Hotspot does not have a consume memory ordering like C++11, that allows utilizing dependent loads, and I do not think we intend to introduce it any time soon unless there are very compelling reasons to do so. In its absence, we should use load_acquire instead, and not micro optimize it away.

AFAIK, compiler implementers are not happy with C++11 consume memory ordering semantics and implement it with acquire instead as there are too many problems with consume.

To elide acquire in an acquire release pair in shared code, very strong motivation is needed. Like for example how constructors publish final object references with release but relies on dependent load in JIT compiled code to elide acquire, because using acquire semantics on all reference loads is too expensive, so it is warranted here.

Therefore, I am wondering if perhaps we want to add an acquire on the load side anyway, so we do not have to introduce consume memory ordering semantics or something similar in the shared code base. Using a normal load without any memory ordering semantics in shared code to act like a load consume rings alarm clocks in my head.

Thanks,
/Erik

> However, I think your implementation of method
> metadata_field_put_volatile() is overkill. You have
> 
> void oopDesc::metadata_field_put_volatile(int offset, Metadata* value) {
>  OrderAccess::release();
>  *metadata_field_addr(offset) = value;
>  OrderAccess::fence();
> }
> 
> That fence() will impose a full memory barrier which is more than is
> actually needed. It should be ok just to use another release.
> 
> void oopDesc::metadata_field_put_volatile(int offset, Metadata* value) {
>  OrderAccess::release();
>  *metadata_field_addr(offset) = value;
>  OrderAccess::release();
> }
> 
> Alternatively, I believe you could implement the assign of using a call
> to OrderAccess::store_release_ptr which would achieve the same outcome
> on AArch64.
> 
> regards,
> 
> 
> Andrew Dinn
> -----------
> Senior Principal Software Engineer
> Red Hat UK Ltd
> Registered in England and Wales under Company Registration No. 03798903
> Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander