RFR: 8199417: Modularize interpreter GC barriers

Wed Apr 11 11:51:17 UTC 2018

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
>