RFR: JDK-8198285: More consistent Access API for arraycopy

[Roman Kennke]

Hi Erik

your changes look good to me.

I'll push it through the submit repo. Do I need another review?
Otherwise I'll push it if submit-repo comes back clean.

Thanks, Roman


>> Thanks for the review. See my comments inline below:
>>
>>> Thanks for doing this. A few comments...
>>>
>>> This:
>>> ArrayAccess<>::arraycopy_from_native<jbyte>(...
>>>
>>> ...won't reliably compile without saying "template", like this:
>>> ArrayAccess<>::template arraycopy_from_native<jbyte>(...
>>>
>>> ...because the template member is qualified on another template class.
>>> My suggestion is to remove the explicit <jbyte> and let that be inferred
>>> from the address instead.
>> Ok, I've done that for all the cases where it's possible. The heap->heap
>> cases cannot do that because there's no address pointer to infer from.
>> I've added the 'template' keyword there.
> 
> There is one use for heap primitive arraycopy that I found that was not
> used inside of a template, and hence does not need the template keyword.
> As for the ones using oop_arraycopy, the type information is never used
> and is hence unnecessary to pass in at all (because the backends always
> get the information whether these are oops or narrow oops through my
> cool template machinery). After removing "template" from those cases,
> there are no cases with "template" left.
> 
>>> And things like this:
>>>   bool RuntimeDispatch<decorators, T,
>>> BARRIER_ARRAYCOPY>::arraycopy_init(arrayOop src_obj, size_t
>>> src_offset_in_bytes, const T* src_raw, arrayOop dst_obj, size_t
>>> dst_offset_in_bytes, T* dst_raw, size_t length) {
>>>
>>> ...as a single line, could do with some newlines to make it reasonably
>>> wide.
>> Ok, I've broken those lines down to have src args in one line, dst args
>> in another. This should make it more readable.
> 
> I liked that. Saw that this was not done consistently though. Should
> probably be done everywhere.
> 
>>> Now that we copy both from heap to heap, from heap to native, and native
>>> to heap, we have to be a bit more careful in modref
>>> oop_arraycopy_in_heap. In the covariant case of arraycopy, you
>>> unconditionally apply the pre and post barriers on the destination.
>>> However, if the destination is native
>>> (ArrayAccess::arraycopy_to_native), that does not feel like it will go
>>> particularly well, unless I have missed something.
>> oop arrays are never copied from/to native.
> 
> Okay, then that makes sense. The general accessor (Access::arraycopy)
> should probably be protected so that one has to go through the
> ArrayAccess class for arraycopy that has the legal variants exposed for
> the public API.
> 
>>> Also your new ArrayAccess class inherits from HeapAccess<IN_HEAP_ARRAY |
>>> decorators>, which is nice. But its member functions call
>>> HeapAccess<decorators>::arraycopy, which will miss out on the
>>> IN_HEAP_ARRAY decorator, which might lead to not using precise marking
>>> in barrier backends. If I were you, I would probably typedef a
>>> BaseAccess for HeapAccess<IN_HEAP_ARRAY | decorators> inside of
>>> ArrayAccess, and call that in the member functions.
>> I am not sure how to do this typedef trick, but I added | IN_HEAP_ARRAY
>> to all the calls. It also required to extend the verify check on the
>> decorators to include IN_HEAP_ARRAY.
> 
> Like this inside of the class declaration:
> typedef HeapAccess<IN_HEAP_ARRAY | decorators> AccessT;
> 
>> Differential:
>> http://cr.openjdk.java.net/~rkennke/JDK-8198285/webrev.02.diff/
>> Full:
>> http://cr.openjdk.java.net/~rkennke/JDK-8198285/webrev.02/
>>
>> What do you think?
> 
> Definitely a lot better. I went ahead and incorporated my last feedback
> into an incremental webrev instead. If you agree and like my additional
> template polishing, then feel free to go with that and consider it
> reviewed:
> 
> Incremental webrev:
> http://cr.openjdk.java.net/~eosterlund/8198285/webrev.00_01/
> 
> Full webrev:
> http://cr.openjdk.java.net/~eosterlund/8198285/webrev.01/




-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: OpenPGP digital signature
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20180604/71ece682/signature.asc>