RFR(M): 8212243: More gc interface tweaks for arraycopy and some other smaller changes in preparation for Shenandoah

[Erik Österlund]

Hi Roman,

On 2018-10-18 14:07, Roman Kennke wrote:
> Hi Erik, Roland and all,
>
>> Hi Roland,
>>
>> On 2018-10-18 09:43, Roland Westrelin wrote:
>>> Hi Erik,
>>>
>>> Thanks for looking at this.
>>>
>>>> One goal I have had in the barrier set classes is to not introduce the
>>>> concepts of pre/post (write) barriers until the ModRefBarrierSet layer,
>>>> as it is not a concept that is at all shared for our GCs. Therefore, I
>>>> would prefer if this:
>>>>
>>>> virtual void array_copy_post_barrier_at_expansion(ArrayCopyNode* ac,
>>>> Node*& c, Node*& m, PhaseIterGVN& igvn) const { } ...to all be moved
>>>> into ModRefBarrierSetC2. I'm imagining to on BarrierSetC2 only expose
>>>> something like clone_barrier_at_expansion() that gets overridden by
>>>> ModRefBarrierSetC2 to do the check if post_barriers are needed, and then
>>>> call the "array_copy_post_barrier_at_expansion" member function of
>>>> ModRefBarrierSetC2, that looks like it should also be renamed to
>>>> clone_post_barrier_at_expansion.
>>> That wouldn't work. The reason for this change is Shenandoah and
>>> ShenandoahBarrierSetC2 is a subclass of BarrierSetC2. I suppose it's not
>>> really a post barrier eventhough other gcs could use that hook as a way
>>> to put their post barrier. We use it to fix references after the copy is
>>> done.
>> That seems to lead us to the next question... is there a reason why
>> ShenandoahBarrierSetC2 is not a subclass of ModRefBarrierSetC2? Since it
>> e.g. uses a SATB barrier, that seems like it would fit more naturally,
>> and would remove the need for all this. If there are things in
>> ModRefBarrierSetC2 you do not need, then you can simply opt out from
>> those things by overriding the virtual member functions.
> I guess that's historical. We never inherited from any of the existing
> barrier set impls because it was not a natural fit. I don't think this
> is a good time to change that. But what you propose would probably work
> for Shenandoah as well? I.e. do clone_barrier_at_expansion(...) and then
> do the right thing in Shenandoah subclass. Similar to how we insert the
> other (e.g. SATB) pre-/post-barriers similar to how ModRef or even G1
> does it.

I can relate to how you feel like you are running out of time and want 
to get this in to 12. Having said that, I think now is a good time to 
get the interfacing right.
But sure, if you want to put all of this behind the top level function 
on BarrierSetC2, then I suppose I am fine with that, as long as the 
notion of pre/post write barriers is not introduced outside of 
ModRefBarrierSetC2, because it is not meant to.

>>>> virtual Node* array_copy_load_store_barrier(PhaseGVN *phase, bool
>>>> can_reshape, Node* v, MergeMemNode* mem, Node*& ctl) const { return v; }
>>>> ...I would prefer to pass in the BasicType, and perform the T_OBJECT
>>>> filtering inside, for symmetry with other similar hooks. Also it beats
>>>> me that this is strictly speaking a load barrier for loads performed in
>>>> arraycopy. Would it be possible to use something like access_load_at
>>>> instead? That would have been fantastic. Or is there a mis-fit, such as
>>>> not having a GraphKit with memory state? If so, it would be interesting
>>>> to see what it would take to fix that. I believe that hook could be
>>>> useful for ZGC cloning as well. Thanks, /Erik
>>> GraphKit is a parse time only thing. So the existing gc interface
>>> doesn't offer any way to add barriers once parsing is over. This code
>>> runs after parsing in optimization phases. In the valhalla repo, Tobias
>>> made a change so we can create a GraphKit at optimization time at places
>>> where we have jvm state. So bringing that would allow the use of
>>> access_load and friends in the arraycopy code. Except it's not a simple
>>> change so I would rather go with what we have here and rework it down
>>> the road.
>> Right. I placed the parse-time access_load_at frontend in GraphKit,
>> because it was only ever used at parse time.
>> But it would appear that it would have been great to have something like
>> that available after parse time as well. I do understand though that
>> getting a GraphKit in here Tobias style, is not a quick fix, and is out
>> of the question for this RFE.
>>
>> So currently the context information of the access comes in two
>> flavours: C2Access for context information that is shared for all
>> accesses, and C2AtomicAccess with extra information only used by
>> atomics. I imagine we could put a new type of access context information
>> wrapper into that hierarchy to fit this use case.
>>
>> So basically, you could make a new C2PostParseTimeAccess (name?!) that
>> you wrap this information in, and send it through the
>> BarrierSetC2::load_at backend.
>> Similar how there is a GraphKit::access_load_at helper for baking the
>> context object, you can have a helper (somewhere at your convenience)
>> for baking the C2PostParseTimeAccess.
>>
>> How would that feel to you?
> I don't know if that would work and how much work it would be, maybe
> Roland can comment on it?
>
> Otherwise, maybe go with what Roland proposed, instead of requiring us
> to rewrite whole swaths of code (which is quite a risk too)? As a
> compromise, to not get stuck on this stuff forever? We're already fairly
> close to JDK12 cutoff, and we really want to make it.

I do not think what I am proposing is a large rewrite. It's merely 
wrapping the load + load_barrier pattern in an accessor that bakes the 
required context information (type, address, control, memory, 
decorators, etc), passing it through BarrierSetC2, letting backends 
insert barriers as required, in a similar way to how this has been 
handled in the rest of the VM. The infrastructure for doing this is 
there, you just need a new C2Access context object that wraps the 
context needed here, bake it, and send it through the barrier set.

> The alternative
> being to drop all that arraycopy stuff on the floor, and sort it out
> later, which would probably work, but it would be a pity.

That does not really seem like an alternative to me.

Thanks,
/Erik

> Roman
>