RFR: 8199417: Modularize interpreter GC barriers

[Kim Barrett]

> 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.00_01/

This looks better, and you've mostly answered the issues I've raised
so far.  Some further discussion inline below.

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/g1BarrierSetAssembler_x86.cpp
>> 364 #ifndef _LP64
>> 365   InterpreterMacroAssembler *imasm = static_cast<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 oop_store_at 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 load_heap_oop is used not only in the InterpreterMacroAssembler, and I wanted the same assembler in load_at and store_at.
> 
> 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 is_interpreter_masm() 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.

>> src/hotspot/cpu/x86/templateInterpreterGenerator_x86.cpp
>> 698 address TemplateInterpreterGenerator::generate_Reference_get_entry(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 barrier_set's
>> load_at?  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/

>> src/hotspot/cpu/x86/gc/shared/modRefBarrierSetAssembler_x86.cpp
>> 86 void ModRefBarrierSetAssembler::store_at(MacroAssembler* masm, DecoratorSet decorators, BasicType type,
>> 87                                        Address dst, Register val, Register tmp1, Register tmp2) {
>> 88   if (type == T_OBJECT || type == T_ARRAY) {
>> 89     oop_store_at(masm, decorators, type, dst, val, tmp1, tmp2);
>> 90   } else {
>> 91     BarrierSetAssembler::store_at(masm, decorators, type, dst, val, tmp1, tmp2);
>> 92   }
>> 93 }
>> 
>> Doesn't the conditional conversion from store_at to oop_store_at
>> actually indicate a bug in the caller? That is, calling store_at with
>> a oop (T_OBJECT or T_ARRAY) value seems like a bug, and should be
>> asserted against, rather than translating.
>> 
>> And oop_store_at 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 oop_store_at?
>> 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 store_at and oop_store_at. 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 store_at/load_at as BasicType. It is the responsibility of ModRefBarrierSetAssembler to filter away non-oop references, and call oop_store_at for relevant accesses for ModRef. This follows the same pattern that the arraycopy stubs used. This allows, e.g. CardTableModRef to only override oop_store_at. This is similar to how ModRefBarrierSet::AccessBarrier<decorators, BarrierSetT>::oop_store_in_heap calls the concrete barriersets for help generating that access. I hope this helps understanding the structuring.
> 
> This is also the reason why G1BarrierSetAssembler::load_at 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 oop_store_at 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.

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