RFR (S) 8182397: Race in field updates when creating ArrayKlasses can lead to crash
coleen.phillimore at oracle.com
coleen.phillimore at oracle.com
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.
> 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.
>> 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
>>> 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.
More information about the hotspot-dev