RFR: 8198949: Modularize arraycopy stub routine GC barriers (original) (raw)

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 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/stubGeneratorXXX.cpp ------------------------------------------- Most of the subGenerator files have a sequence, similar to this: ... BarrierSetCodeGen *bs = Universe::heap()->barrierset()->codegen(); DecoratorSet decorators = ARRAYCOPYDISJOINT; BasicType type = isoop ? TOBJECT : TINT; if (destuninitialized) { decorators |= ASDESTNOTINITIALIZED; } if (aligned) { decorators |= ARRAYCOPYALIGNED; } bs->arraycopyprologue(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 = ARRAYCOPYDISJOINT; if (destuninitialized) { decorators |= ASDESTNOTINITIALIZED; } if (aligned) { decorators |= ARRAYCOPYALIGNED; } BasicType type = isoop ? TOBJECT : TINT; BarrierSetCodeGen *bs = Universe::heap()->barrierset()->codegen(); bs->arraycopyprologue(masm, decorators, type, from, to, count); ...

Fixed.

src/hotspot/share/gc/shared/barrierSet.cpp ------------------------------------------ Instead of BarrierSet::initialize() and BarrierSet::makecodegen(), could we initialize the codegen 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* cardtable) : CardTableModRefBS(cardtable, BarrierSet::FakeRtti(BarrierSet::G1BarrierSet), BarrierSetCodeGen::create(), dcqs(JavaThread::dirtycardqueueset()) {} Where BarrierSetCodeGen::create() 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() which does the same thing for C1, etc. I think this also means that BarrierSet::codegen can be made private instead of protected. How does that sound?

I tried a variation of this. Instead of BarrierSetCodeGen::create(), I use BarrierSet::make_code_gen(). 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/g1BSCodeGensparc.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::writerefarraypreoopentry(oop* dst, sizet length) { assert(length <= (sizet)maxintx, "count too large");_ _G1BarrierSet *bs =_ _barriersetcast(BarrierSet::barrierset()); bs->G1BarrierSet::writerefarraypre(dst, (int)length, false); } maxinx in the assert above can be larger than int, but we later cast length to int when we later call writerefarraypre(), which looks dangerous. I'd suggest that we change writerefarraypre() and writerefarrayprework() to take a sizet instead of an int and remove the assert.

Fixed.

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

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

void G1BarrierSet::writerefarrayprenarrowoopentry(narrowOop* dst, sizet length) { assert(length <= (sizet)maxintx, "count too large");_ _G1BarrierSet *bs =_ _barriersetcast(BarrierSet::barrierset()); bs->G1BarrierSet::writerefarraypre(dst, (int)length, false); } Same comments as above. void G1BarrierSet::writerefarraypostentry(HeapWord* dst, sizet length) { G1BarrierSet *bs = barriersetcast(BarrierSet::barrierset()); bs->G1BarrierSet::writerefarray(dst, (int)length); } writerefarray() takes a sizet 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