RFR: 8198949: Modularize arraycopy stub routine GC barriers

[Per Liden]

Hi Erik,

Nice patch, a few comments below.

General
-------
May I suggest that we name the CodeGen classes 
<barrier_set_nanme>CodeGen, like G1BarrierSetCodeGen instead of 
G1BSCodeGen. The names become a bit longer but I think the relationship 
with the barrier set them becomes more clear.


src/hotspot/cpu/sparc/stubGenerator_XXX.cpp
-------------------------------------------
Most of the subGenerator files have a sequence, similar to this:

...
BarrierSetCodeGen *bs = Universe::heap()->barrier_set()->code_gen();
DecoratorSet decorators = ARRAYCOPY_DISJOINT;
BasicType type = is_oop ? T_OBJECT : T_INT;
if (dest_uninitialized) {
   decorators |= AS_DEST_NOT_INITIALIZED;
}
if (aligned) {
   decorators |= ARRAYCOPY_ALIGNED;
}
bs->arraycopy_prologue(_masm, decorators, type, from, to, count);
...

Could we re-group block to be more like this, where we keep the setup of 
"decorators" grouped together, and move down the others to where they 
are first used? Just to make it a bit easier to read.

...
DecoratorSet decorators = ARRAYCOPY_DISJOINT;
if (dest_uninitialized) {
   decorators |= AS_DEST_NOT_INITIALIZED;
}
if (aligned) {
   decorators |= ARRAYCOPY_ALIGNED;
}

BasicType type = is_oop ? T_OBJECT : T_INT;
BarrierSetCodeGen *bs = Universe::heap()->barrier_set()->code_gen();
bs->arraycopy_prologue(_masm, decorators, type, from, to, count);
...

src/hotspot/share/gc/shared/barrierSet.cpp
------------------------------------------
Instead of BarrierSet::initialize() and BarrierSet::make_code_gen(), 
could we initialize the _code_gen pointer by having the concrete barrier 
set create it in its constructor and pass it up the chain to BarrierSet. 
For G1, it would look something like this:

G1BarrierSet::G1BarrierSet(G1CardTable* card_table) :
    CardTableModRefBS(card_table,
                      BarrierSet::FakeRtti(BarrierSet::G1BarrierSet),
                      BarrierSetCodeGen::create<G1BarrierSetCodeGen>(),
                      _dcqs(JavaThread::dirty_card_queue_set()) {}

Where BarrierSetCodeGen::create<T>() would check the necessary 
conditions if a CodeGen class should be created, and if so, create a T 
and return a BarrierSetCodeGen*. And in the future, we can have a 
BarrierSetCodeGenC1::create<T>() which does the same thing for C1, etc.

I think this also means that BarrierSet::_code_gen can be made private 
instead of protected.

How does that sound?


src/hotspot/cpu/sparc/gc/g1/g1BSCodeGen_sparc.cpp
-------------------------------------------------
It looks like the fast-path, for when the mark queue is in-active, has 
been accidentally dropped here?


src/hotspot/share/gc/g1/g1BarrierSet.cpp
----------------------------------------

void G1BarrierSet::write_ref_array_pre_oop_entry(oop* dst, size_t length) {
   assert(length <= (size_t)max_intx, "count too large");
   G1BarrierSet *bs = 
barrier_set_cast<G1BarrierSet>(BarrierSet::barrier_set());
   bs->G1BarrierSet::write_ref_array_pre(dst, (int)length, false);
}

max_inx in the assert above can be larger than int, but we later cast 
length to int when we later call write_ref_array_pre(), which looks 
dangerous. I'd suggest that we change write_ref_array_pre() and 
write_ref_array_pre_work() to take a size_t instead of an int and remove 
the assert.

Also, the call to:
   bs->G1BarrierSet::write_ref_array_pre(dst, (int)length, false);

could be shortened to:
   bs->write_ref_array_pre(dst, (int)length, false);


void G1BarrierSet::write_ref_array_pre_narrow_oop_entry(narrowOop* dst, 
size_t length) {
   assert(length <= (size_t)max_intx, "count too large");
   G1BarrierSet *bs = 
barrier_set_cast<G1BarrierSet>(BarrierSet::barrier_set());
   bs->G1BarrierSet::write_ref_array_pre(dst, (int)length, false);
}

Same comments as above.


void G1BarrierSet::write_ref_array_post_entry(HeapWord* dst, size_t 
length) {
   G1BarrierSet *bs = 
barrier_set_cast<G1BarrierSet>(BarrierSet::barrier_set());
   bs->G1BarrierSet::write_ref_array(dst, (int)length);
}

write_ref_array() takes a size_t but we cast length to an int, which is 
wrong.


This was only a half review. I haven't looked through the x86-specific 
stuff in detail yet. I'll follow up on that tomorrow.

cheers,
Per

On 03/09/2018 05:58 PM, Erik Österlund wrote:
> Hi,
> 
> The GC barriers for arraycopy stub routines are not as modular as they 
> could be. They currently use switch statements to check which GC barrier 
> set is being used, and call one or another barrier based on that, with 
> registers already allocated in such a way that it can only be used for 
> write barriers.
> 
> My solution to the problem is to introduce a platform-specific GC 
> barrier set code generator. The abstract super class is 
> BarrierSetCodeGen, and you can get it from the active BarrierSet. A 
> virtual call to the BarrierSetCodeGen generates the relevant GC barriers 
> for the arraycopy stub routines.
> 
> The BarrierSetCodeGen inheritance hierarchy exactly matches the 
> corresponding BarrierSet inheritance hierarchy. In other words, every 
> BarrierSet class has a corresponding BarrierSetCodeGen class.
> 
> The various switch statements that generate different GC barriers 
> depending on the enum type of the barrier set have been changed to call 
> a corresponding virtual member function in the BarrierSetCodeGen class 
> instead.
> 
> Thanks to Martin Doerr and Roman Kennke for providing platform specific 
> code for PPC, S390 and AArch64.
> 
> Webrev:
> http://cr.openjdk.java.net/~eosterlund/8198949/webrev.00/
> 
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8198949
> 
> Thanks,
> /Erik