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

[Andrew Haley]

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.

-- 
Andrew Haley
Java Platform Lead Engineer
Red Hat UK Ltd. <https://www.redhat.com>
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671