RFR: 8166862: CMS needs klass_or_null_acquire

[Kim Barrett]

> On Oct 5, 2016, at 12:10 PM, White, Derek <Derek.White at cavium.com> wrote:
> -----Original Message-----
> From: hotspot-dev [mailto:hotspot-dev-bounces at openjdk.java.net] On Behalf Of Kim Barrett
> Sent: Tuesday, October 04, 2016 7:12 PM
> To: David Holmes <david.holmes at oracle.com>
> Cc: hotspot-dev Source Developers <hotspot-dev at openjdk.java.net>
> Subject: Re: RFR: 8166862: CMS needs klass_or_null_acquire
>  
> > On Oct 4, 2016, at 5:04 AM, David Holmes <david.holmes at oracle.com> wrote:
> > 
> > Hi Kim,
> > 
> > On 2/10/2016 7:29 AM, Kim Barrett wrote:
> >> Please review this change to the CMS collector to replace uses of 
> >> klass_or_null with klass_or_null_acquire where needed.
> >> 
> >> [Reviewing on hotspot-dev; only GC code is involved, but some non-GC 
> >> folks have expressed interest.]
> >> 
> >> All non-assert uses of klass_or_null are being replaced with the 
> >> acquire variant.  In most cases, the size is being accessed soon 
> >> after, and the acquire is needed to ensure order of klass and 
> >> possible inline size access.  In a few cases, the need for acquire is 
> >> less immediately obvious:
> >> 
> >> - CompatibleFreeListSpace::block_is_obj - Size access is not 
> >> lexically  apparent, but callers access the size or other data 
> >> ordered with the  klass.
> >> 
> >> - MarkFromRootsClosure::do_bit and ParMarkFromRootsClosure::do_bit:
> >>  These perform other operations that need to be properly ordered wrto  
> >> the klass access.
> > 
> > Can you point me to the release-store that is being paired with these load-acquires please?
>  
> It’s the release_set_klass recently added here:
> http://hg.openjdk.java.net/jdk9/hs/hotspot/rev/fd16b627ebc5
>  
> FYI
> I'm still looking, but I think this is true:
>  
> I think a release-store also needs to be in the interpreter and jitted code, for processors that need it (e.g. aarch64 and ppc). For example in MacroAssembler::store_klass.
>  
> Those processors have a storestore after object initialization to ensure that Java threads don't see uninitialized fields, but don't have a store-release on setting the class to prevent CMS from tripping over a mis-ordered header initialization. The nominal store order may also be wrong (e.g. "length" before "klass"). See C1_MacroAssembler::initialize_header()
>  
> If this sounds right, I can file a bug…

Those are all young-gen allocations.  The issues that lead to
require_set_klass and klass_or_null_acquire pairings only arise with
old-gen allocations.  All mutator initiated old-gen allocations go
through the post_allocation_setup_common path.  (That's also the slow
path for young-gen allocations, which don't need the require fence,
but conditionalizing the fence didn't seem worthwhile.)

There is a separate problem that fences are needed when a new object
escapes to some other thread.  It must be a well-formed object,
including having its klass set, before escaping.  So a fence is
between setting the klass and escape.  I think those exist, though
Dean has been chasing a bug for a while that might be a missing one.

The difference is that these concurrent collectors can form a
reference to a blob of memory that is in the process of being turned
into a new object, e.g. before its klass has been set.  That's where
klass_or_null comes into play.  G1 almost doesn't do that; see
"RFR: 8166607: G1 needs klass_or_null_acquire" and JDK-8166995.