RFR: 8199417: Modularize interpreter GC barriers

Wed Apr 11 11:56:45 UTC 2018

Hi Coleen,

Thanks for the review.

/Erik

On 2018-04-11 13:51, coleen.phillimore at oracle.com wrote:
>
>
> On 4/11/18 5:05 AM, Roman Kennke wrote:
>> Hi Erik,
>>> I have no plans to update the wrappers calling BarrierSetAssembler on
>>> PPC, S390 and AArch64 code.
>>>
>>> 1) I simply can't check if my changes work or not on those platforms.
>>> 2) This is an interface between the platform specific code and the GC.
>>> So I feel like the chosen wrapper to call into BarrierSetAssembler 
>>> is up
>>> to the maintainers of those platforms, and it feels like it is not my
>>> call to tell other maintainers what wrapper functions they should use.
>>>
>>> So basically, since I can't test on those platforms, I think it is 
>>> up to
>>> Martin Doerr and Roman Kennke to adopt the wrapper function if they 
>>> like
>>> it. And I think that is easiest to do as separate RFEs.
>> This sounds reasonable.
>
> Okay, I agree.  That sounds reasonable.  I've reviewed the interpreter 
> pieces.
> Coleen
>
>>
>> Thanks, Roman
>>
>>
>>> Thanks,
>>> /Erik
>>>
>>>
>>> On 2018-04-11 00:49, coleen.phillimore at oracle.com wrote:
>>>>
>>>> On 4/9/18 4:05 PM, Erik Österlund wrote:
>>>>> 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.
>>>> http://cr.openjdk.java.net/~eosterlund/8199417/webrev.02/src/hotspot/cpu/ppc/interp_masm_ppc_64.cpp.udiff.html 
>>>>
>>>>
>>>>
>>>> :( you're not going to do all of them since you changed this code
>>>> too?   It would make it easier for us to pattern match for changes to
>>>> other platforms if you did, since we have no way of testing these
>>>> platforms.
>>>>
>>>> The sparc changes look reasonable too.
>>>>
>>>> Coleen
>>>>
>>>>> 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.00_01/
>>>>>> 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/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.
>>>>> Thanks.
>>>>>
>>>>>>>> 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/
>>>>> Fixed.
>>>>>
>>>>>>>> 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.
>>>>> I added some comments describing the idea behind ModRef.
>>>>>
>>>>>> Shouldn't ModRef::oop_store_at be pure virtual?  That would have
>>>>>> helped my understanding too.
>>>>> Sure. Fixed.
>>>>>
>>>>> Thanks,
>>>>> /Erik
>>
>