RFR Backport 8165808: Add release barriers when allocating objects with concurrent collection

[Yangfei (Felix)]

Hi,

> -----Original Message-----
> From: Andrew Haley [mailto:aph at redhat.com]
> Sent: Thursday, August 27, 2020 12:12 AM
> To: Yangfei (Felix) <felix.yang at huawei.com>; jdk8u-dev at openjdk.java.net
> Subject: Re: RFR Backport 8165808: Add release barriers when allocating
> objects with concurrent collection

Cut ...

> >>
> >> By the way, there are many places where arrays are allocated. Do you
> >> know why only this particular one needs a release barrier?
> >
> > Not sure if I understand the question correctly.
> > I think the common entry points are CollectedHeap::obj_allocate &
> CollectedHeap::array_allocate.  These functions finally calls
> CollectedHeap::post_allocation_setup_common.
> > Many places will call these functions to create objects.  For example,
> > InstanceKlass::allocate_objArray -> CollectedHeap::array_allocate ->
> > CollectedHeap::post_allocation_setup_array ->
> > CollectedHeap::post_allocation_setup_common
> > Since we do a release store in
> CollectedHeap::post_allocation_setup_common, so all the places where
> objects are created will benefit.
> 
> There is code at MacroAssembler::eden_allocate and
> PhaseMacroExpand::expand_allocate_common which does object allocation.
> If CollectedHeap::obj_allocate needs release barriers, why don't those places
> need them too? Is there another patch which adds such release barriers that
> also needs to be backported?

That's a good question.
I checked the latest jdk/jdk repo and looks like the barrier is not there too. 
One of the code path looks like: TemplateTable::_new -> MacroAssembler::eden_allocate

Consider the fast path in TemplateTable::_new, we do a plain store for klass:

3622     // initialize object header only.
3623     __ bind(initialize_header);
3624     if (UseBiasedLocking) {
3625       __ ldr(rscratch1, Address(r4, Klass::prototype_header_offset()));
3626     } else {
3627       __ mov(rscratch1, (intptr_t)markWord::prototype().value());
3628     }
3629     __ str(rscratch1, Address(r0, oopDesc::mark_offset_in_bytes()));
3630     __ store_klass_gap(r0, zr);  // zero klass gap for compressed oops
3631     __ store_klass(r0, r4);      // store klass last                <==============

Looking at MacroAssembler::store_klass, the same question has been raised early before.

3780 void MacroAssembler::store_klass(Register dst, Register src) {
3781   // FIXME: Should this be a store release?  concurrent gcs assumes
3782   // klass length is valid if klass field is not null.
3783   if (UseCompressedClassPointers) {
3784     encode_klass_not_null(src);
3785     strw(src, Address(dst, oopDesc::klass_offset_in_bytes()));
3786   } else {
3787     str(src, Address(dst, oopDesc::klass_offset_in_bytes()));
3788   }
3789 }

But the slow path in TemplateTable::_new is different.  The call chain looks like:
    InterpreterRuntime::_new -> InstanceKlass::allocate_instance -> CollectedHeap::obj_allocate -> MemAllocator::allocate -> ObjAllocator::initialize -> MemAllocator::finish -> oopDesc::release_set_klass

I don't see a reason why the barrier is not necessary for the fast path.
There is a similar issue for PhaseMacroExpand::expand_allocate_common: fast path and slow path are different on the storing of klass.
Also C1 uses a plain store for klass too in C1_MacroAssembler::initialize_header.
I'm more inclined to think these code paths are missed by the original patch, but that need confirmation.

Thanks,
Felix