RFR: 8199417: Modularize interpreter GC barriers

Tue Apr 10 22:49:00 UTC 2018

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