RFR: 8166862: CMS needs klass_or_null_acquire

[White, Derek]

Thanks for the reminder Kim! (Boy, you walk away from this code for a few days...)

So we have the case where the slow-path "runtime" allocators always do a store_release on the klass (if CMS might run), but never do a StoreStore barrier after initializing the object fields/elements. The interpreter/compilers are responsible for that (typically done by generated code).

That leaves other runtime callers of the CollectedHeap allocators to also make sure that they have the correct barriers.

It's not yet clear that they do. It might help to separate allocators directly called by the interpreter/compiler. Here's a 2-deep fan-out:
	InstanceMirrorKlass::allocate_instance
	InstanceKlass::allocate_instance
	ArrayKlass::allocate_arrayArray
	InstanceKlass::allocate_objArray
 	- oopFactory::new_objArray
	ObjArrayKlass::allocate
 	- Deoptimization::realloc_objects
	- oopFactory ::new_objectArray
	TypeArrayKlass::allocate_common
  	- oopFactory::new_metaDataArray
  	- oopFactory::new_typeArray_nozero (OK)
	JVM_Clone (OK?).

 - Derek

-----Original Message-----
From: Kim Barrett [mailto:kim.barrett at oracle.com] 
Sent: Wednesday, October 05, 2016 2:42 PM
To: White, Derek <Derek.White at cavium.com>
Cc: David Holmes <david.holmes at oracle.com>; hotspot-dev Source Developers <hotspot-dev at openjdk.java.net>
Subject: Re: RFR: 8166862: CMS needs klass_or_null_acquire

> 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.