RFR: 8199417: Modularize interpreter GC barriers (original) (raw)

Erik Österlund erik.osterlund at oracle.com
Mon Apr 9 20:05:08 UTC 2018

Hi Kim,

Thank you for looking at this. Since this was well received, I went ahead and implemented something similar on SPARC as well. Also added support for storing roots (rather than asserting that stores are in the heap only), just in case we need it in the future.

Full webrev: http://cr.openjdk.java.net/~eosterlund/8199417/webrev.02/

Incremental webrev: http://cr.openjdk.java.net/~eosterlund/8199417/webrev.01_02/

On 2018-04-06 01:38, Kim Barrett wrote:

On Apr 4, 2018, at 9:38 AM, Erik Österlund <erik.osterlund at oracle.com> wrote:

Hi Kim, Thank you for reviewing this. I have made a new webrev with proposed changes to the x86/x64 code reflecting the concerns you and Coleen have. I thought we should settle for something we like in the x86/x64 code before going ahead and changing the other platforms too much (I don't want to bug Martin and Roman too much before). New full webrev: http://cr.openjdk.java.net/~eosterlund/8199417/webrev.01/ Incremental: http://cr.openjdk.java.net/~eosterlund/8199417/webrev.0001/ This looks better, and you've mostly answered the issues I've raised so far. Some further discussion inline below.

I'm glad to hear that.

Other things have come up and I'm not going to be able to properly get back to this soon; don't hold up on my account, just drop me as a reviewer.

On 2018-04-01 05:12, Kim Barrett wrote: src/hotspot/cpu/x86/gc/g1/g1BarrierSetAssemblerx86.cpp 364 #ifndef LP64 365 InterpreterMacroAssembler *imasm = staticcast<InterpreterMacroAssembler*>(masm); 366 #endif

In the old code, the function received an InterpreterMacroAssembler*. What is there about this context that lets us know the downcast is safe here? Is oopstoreat only ever called with an IMA*? If so, then why isn't that the parameter type. If not, then what? Yes, this is indeed only used by the InterpreterMacroAssembler. But I wanted the interface to use MacroAssembler. Why? 1) It's only 32 bit x86 that relies on this property, and I hope it will go away soon, and the save bcp hack with it. 2) previous loadheapoop is used not only in the InterpreterMacroAssembler, and I wanted the same assembler in loadat and storeat. So for now it is only used in InterpreterMacroAssembler, and I doubt that will change any time soon. I am hoping 32 bit support will be dropped before that, and the hack will go away. For now, I have added a virtual isinterpretermasm() call that I use in an assert to make sure this is not accidentally used in the wrong context until 32 bit support is gone. The comment plus the new assert satisfies my complaint.

Thanks.

src/hotspot/cpu/x86/templateInterpreterGeneratorx86.cpp 698 address TemplateInterpreterGenerator::generateReferencegetentry(void) {

Almost all the code here (and much of the commentary) was/is specific to G1 (or SATB collectors). Shouldn't it all be in the barrierset's loadat? As it is now, if (say) we're using Parallel GC then I think there's a bunch of additional code being generated and executed here that wasn't present before. It is true that interpreter Reference.get() intrinsics with Parallel/CMS/Serial currently run a few instructions more than they need to pay for the abstraction. There is no doubt in my mind that this abstraction is worth the cost. Reference.get in the interpreter is not a hot path, and does not need to optimize every cycle. I changed the G1 comments to more general comments instead. Good point on this not being a hot path. "Preserve the sender sp in case the pre-barrier calls the runtime" s/the pre-barrier/a load barrier/

Fixed.

src/hotspot/cpu/x86/gc/shared/modRefBarrierSetAssemblerx86.cpp 86 void ModRefBarrierSetAssembler::storeat(MacroAssembler* masm, DecoratorSet decorators, BasicType type, 87 Address dst, Register val, Register tmp1, Register tmp2) { 88 if (type == TOBJECT || type == TARRAY) { 89 oopstoreat(masm, decorators, type, dst, val, tmp1, tmp2); 90 } else { 91 BarrierSetAssembler::storeat(masm, decorators, type, dst, val, tmp1, tmp2); 92 } 93 }

Doesn't the conditional conversion from storeat to oopstoreat actually indicate a bug in the caller? That is, calling storeat with a oop (TOBJECT or TARRAY) value seems like a bug, and should be asserted against, rather than translating. And oopstoreat should assert that the value is an oop value. And should there be any verification that the decorators and the type are consistent? But maybe I'm not understanding the protocol around oopstoreat? There being no documentation, it seems entirely possible that I've misunderstood something here. Maybe I'm being confused by similarities in naming with Access:: that don't hold? The confusion roots in that Access and BarrierSetAssembler are not structured exactly the same. Access provides storeat and oopstoreat. The reason for providing both of these, is that I could not safely infer whether the passed in parameters were oops or not without changing oop and narrowOop to always be value objects, which I did not dare to do as the generated code was worse. In BarrierSetAssembler, there is no such restriction. The type for the access is always passed in to storeat/loadat as BasicType. It is the responsibility of ModRefBarrierSetAssembler to filter away non-oop references, and call oopstoreat for relevant accesses for ModRef. This follows the same pattern that the arraycopy stubs used. This allows, e.g. CardTableModRef to only override oopstoreat. This is similar to how ModRefBarrierSet::AccessBarrier<decorators, BarrierSetT>::oopstoreinheap calls the concrete barriersets for help generating that access. I hope this helps understanding the structuring. This is also the reason why G1BarrierSetAssembler::loadat checks whether it is an oop that is loaded or not, because loads are not riding on the ModRef layer, which only knows about oop stores. I see. The naming congurence, along with my having glazed over the fact that oopstoreat is protected rather than public, led me astray. I don't have any better suggestions for naming. I would have found some comments describing the protocols helpful.

I added some comments describing the idea behind ModRef.

Shouldn't ModRef::oopstoreat be pure virtual? That would have helped my understanding too.

Sure. Fixed.

Thanks, /Erik

More information about the hotspot-dev mailing list