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