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

[Roman Kennke]

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.

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

Roman

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20181018/4d4cd4d9/signature.asc>