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

[coleen.phillimore at oracle.com]

Thank you for your comments, Erik and Andrew.   I like the idea of rules 
for innocent coders (ones who aren't versed in the complexities of 
architecture ordering semantics like myself), that say things like "if 
you have a store-release, you should have a load-acquire on the other 
side".  I would follow this if the load were in the C++ code.  Since 
it's in the JIT compiled code, I think taking advantage of the knowledge 
that this is a dependent load (if I understand this correctly) seems 
acceptable and recommended because performance counts.

Thanks,
Coleen

On 7/24/17 1:40 PM, Andrew Haley wrote:
> On 24/07/17 17:44, Erik Osterlund wrote:
>> 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.
> I'm sure we do in plenty of places, it's just undocumented or maybe
> even unknown to the original author.  We certainly do this a lot in
> the code that the JIT compilers generate, as you note below.
>
>> 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.
> I strongly disagree.  Consume is fine now: I think you're dealing with
> out-of-date information.  It took a while to figure out how to specify
> it, that's all.
>
>> 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.
> Right.  So we already depend on that property, so...
>
>> 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.
> I don't think it's that much of a big deal, really.  Putting in an
> acquire fence every time we read a java mirror strikes me as madness
> when consume is all we need on any target.  Even Power PC doesn't need
> a fence instruction there.  At least make sure that targets can turn
> that off: we should not pay the price for something we don't need.
>