[foreign-memaccess+abi] RFR: 8270376: Finalize API for memory copy [v11]

[Maurizio Cimadamore]

On Mon, 9 Aug 2021 16:26:33 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> This patch ties up some loose ends with the MemoryCopy API, and it also prepares the ground for some other related refactorings in this area.
>> 
>> The meat of this patch is represented by the various changes in MemoryCopy, where all methods were renamed to simply `copy`, and the length parameter (now called `elementCount`) is always moved to the end.
>> 
>> I've also simplified naming of parameters, as I think distinguishing between `index` and `offset` is enough (e.g. an array has an index, a segment has an offset).
>> 
>> You will see that, at the very end of the class, three more copy methods have been added, which deal fully in terms of segments. I have also moved the complex layout-based memory segment copy operation (which does the swap) as a static method in this class, as I believe the static form makes the method more regular and usable.
>> 
>> I've made some changes to our uses of `copyFrom` in the linker, to use the new methods in `MemoryCopy` when the copy operation was performing slicing in the source/target segment, which I think makes the code more readable. Of course these changes are completely optional and could be omitted as well.
>
> Maurizio Cimadamore has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - Consolidate arrays functions to take Object (instead of using overloads)
>    Simplify tests
>  - Drop toXYZArray methods (but toByteArray)

Another update: I decided to remove toXYZArray methods (as discussed last week). And I have also consolidated the various primitive-accepting routines such as MemorySegment::ofArray and MemoryAccess::copy to work with `Object` instead of having multiple unrelated overloads. This was suggested by John some time ago - and I raised some performance concerns. I have now verified that the `Object`-accepting versions of the methods are just as fast, regardless of whether code is inlined or not. They are also easier to work with when the code doesn't know the type of the array it's working with, so these changes led to biggie simplifications in the memory copy tests. A javadoc for this iteration is here:

http://cr.openjdk.java.net/~mcimadamore/panama/memorycopy_finalize_javadoc_v2/javadoc/jdk/incubator/foreign/package-summary.html

(I put it in a different location so that it can be compared with the previous one).

It would be nice if @uschindler could test this iteration (sorry for the back and forth!), as this change might have a non trivial impact on performances. If things regress, we can go back to the previous iteration (but in my benchmarks I didn't see evidence of regression).

As John pointed out, the reason I like this version more is that it does without primitive overloads, which I think will look rather stale once Valhalla is around. That is, `MemorySegment` is Valhalla-ready as is - perhaps some adjustments will be required on `MemoryAccess` - but since that's a bag of static methods, it seems less problematic.

-------------

PR: https://git.openjdk.java.net/panama-foreign/pull/568