RFR: 8199417: Modularize interpreter GC barriers

Wed Apr 11 09:05:04 UTC 2018

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.

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
>>
> 