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

Tue Jul 25 17:18:45 UTC 2017

On 7/25/17 11:36 AM, Erik Österlund wrote:
> Hi Coleen,
>
> I agree that if this is accessed purely in JIT compiled code that we 
> control, and the value is stable, i.e. goes from NULL to not NULL, and 
> then never changes, then this seems safe without acquire in this 
> specific case.
> Having said that, the corresponding java_lang_Class::array_klass() 
> function loads the klass with a non-volatile load, rather than a 
> load_acquire. Now, it seems like the only use of ths function is in 
> InstanceKlass::oop_print_on(), so I guess it is not crucial 
> performance wise. It would be great if the 
> java_lang_Class::array_klass() was safe to use in our code base so 
> that new users that accidentally use it for more than printing will 
> not be surprised by this piece of code and how it inconsistent to 
> other hotspot code violates our synchronization practices of linking 
> release_store with load_acquire, in favor of a non-volatile load.
>
> Would you agree it would be beneficial to use load_acquire at least in 
> this not so hot path in the C++ code? Then we have indisputably 
> correct synchronization in the C++ code, can dodge the whole consume 
> discussion (we do what every other acquire/release pair does in 
> hotspot - business as usual), and leave the optimized case to our own 
> JIT compilers that we trust retain the data dependency. I would be 
> happy with that.

Yes, I checked this in but I should have done this.  I mistakenly 
thought the only accesses for this field were through compiled code and 
thought there wasn't a java_lang_Class::array_klass() accessor. But 
there is but it's only used for printing.

I'll file a bug to make the array_klass() use load acquire for 
cleanliness and send it out in a bit.  Sorry to jump the gun on pushing.

thanks,
Coleen

>
> Thanks,
> /Erik
>
> On 2017-07-24 21:39, coleen.phillimore at oracle.com wrote:
>>
>> 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.
>>>
>>
>