RFR: 8198949: Modularize arraycopy stub routine GC barriers

Erik Österlund erik.osterlund at oracle.com
Mon Mar 12 16:02:45 UTC 2018


Hi Per,

Thank you for reviewing this.

New full webrev:
http://cr.openjdk.java.net/~eosterlund/8198949/webrev.01/

Incremental:
http://cr.openjdk.java.net/~eosterlund/8198949/webrev.00_01/

On 2018-03-12 15:13, Per Liden wrote:
> 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.

Fixed.

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

Fixed.

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

I tried a variation of this. Instead of 
BarrierSetCodeGen::create<G1BarrierSetCodeGen>(), I use 
BarrierSet::make_code_gen<G1BarrierSetCodeGen>(). The reason is that we 
might not even have a concrete code gen class available if we are 
running on Zero, but then we have a forward declaration instead. The 
BarrierSet make function checks accordingly whether the code generator 
should be instantiated or not (depending on whether we are compiling 
with Zero or not).

I hope this is kind of what you had in mind.

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

Oops. Fixed.

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

Fixed.

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

Fixed.

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

Thank you for the review.

Thanks,
/Erik

> 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



More information about the hotspot-dev mailing list