[foreign-memaccess+abi] RFR: 8315769: Add support for sliced allocation [v2]

[Jorn Vernee]

On Wed, 6 Sep 2023 11:45:53 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> This PR adds a new method in `SegmentAllocator`:
>> 
>> 
>> default MemorySegment allocateFrom(ValueLayout elementLayout, MemorySegment source,
>>                                        ValueLayout sourceElementLayout, long sourceOffset, long srcElementCount) {
>> 
>> 
>> This method allows clients to allocate a new memory segment and copy the contents of a portion of an existing segment into the newly allocated region of memory. As such it can be used to address the following use cases:
>> 
>> * allocate from a `ByteBuffer`
>> * allocate from another memory segment
>> * allocate from a Java array slice
>> 
>> All these cases were not covered by the existing API points, which meant that developers had to use a more general allocation request (such as `allocate(long, long)`) and then pay a performance cost (because of memory zeroing). In other words, the new method in this PR completes the allocation API, by providing a flexible way to allocate a new segment from an existing source (another segment) with given offset and length.
>> 
>> Given that the new method is more general than the existing array-accepting `allocateFrom`, this PR rewires the existing array-accepting method to be rewritten on top of the new overload (and tweaks the javadoc of such methods accordingly).
>> 
>> One detail to note is that the new method takes _two_ element layouts - one is the layout of the newly allocated segment, whereas the other is the layout of the source segment. Such layouts must have same alignment and same carrier - but they can have different endianness (in which case a bulk copy with swap is performed). This is not too different from the most general `MemorySegment::copy` static overload.
>
> Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix tests

Very nice improvement!

The implementation feels easier to follow as well.

src/java.base/share/classes/java/lang/foreign/SegmentAllocator.java line 264:

> 262:      * source memory segment.
> 263:      * @implSpec the default implementation for this method calls {@code this.allocate(elementLayout, elementCount)}.
> 264:      * @param elementLayout the value to be set on the newly allocated memory block.

This reads a bit strange. 'on' feels out of place to me. I'd suggest following the flavor of the other allocateFrom overloads, e.g.

Suggestion:

     * @param elementLayout the element layout of the array to be allocated.


Given that we have a pair of element layouts, I think it's reasonable to say that the result is an 'array'. (not a Java array though)

src/java.base/share/classes/java/lang/foreign/SegmentAllocator.java line 268:

> 266:      * @param sourceElementLayout the element layout of the source segment.
> 267:      * @param sourceOffset the starting offset, in bytes, of the source segment.
> 268:      * @param srcElementCount the number of elements in the source segment to be copied.

Maybe just `elementCount` here, instead of `srcElementCount`. It doesn't necessarily have any connection to the source? (`elementCount` is also used later in the IOOBE `@throws` clauses)

src/java.base/share/classes/java/lang/foreign/SegmentAllocator.java line 307:

> 305:     default MemorySegment allocateFrom(ValueLayout.OfByte elementLayout, byte... elements) {
> 306:         return allocateFrom(elementLayout, MemorySegment.ofArray(elements),
> 307:                 elementLayout.withOrder(ByteOrder.nativeOrder()), 0, elements.length);

Can't the source layout here just be `JAVA_BYTE`? Since we're copying from a java byte array. (similar for the other overloads).

Suggestion:

                JAVA_BYTE, 0, elements.length);

test/micro/org/openjdk/bench/java/lang/foreign/AllocFromSliceTest.java line 1:

> 1: package org.openjdk.bench.java.lang.foreign;

Missing copyright header

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

Marked as reviewed by jvernee (Committer).

PR Review: https://git.openjdk.org/panama-foreign/pull/878#pullrequestreview-1613223760
PR Review Comment: https://git.openjdk.org/panama-foreign/pull/878#discussion_r1317220428
PR Review Comment: https://git.openjdk.org/panama-foreign/pull/878#discussion_r1317221835
PR Review Comment: https://git.openjdk.org/panama-foreign/pull/878#discussion_r1317212037
PR Review Comment: https://git.openjdk.org/panama-foreign/pull/878#discussion_r1317225051