[foreign-jextract] RFR: 8264187: Add a method for creating a slicing MethodHandle

[Jorn Vernee]

On Fri, 26 Mar 2021 14:23:12 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> Hi,
>> 
>> This patch adds a `sliceHandle(PathElement...)` method to MemoryLayout, which can be used to create a method handle that, when given a segment, will return a slice of that segment for the layout element selected by the earlier given layout path.
>> 
>> This for instance makes it easier to iterate over an array of structs by first creating a slice for each element, and then accessing struct fields from there, without having to resolve to manual offset computation. For example:
>> 
>> SequenceLayout seq = MemoryLayout.ofSequence(5,
>>     MemoryLayout.ofStruct(
>>        MemoryLayout.ofValueBits(32, ByteOrder.nativeOrder()).withName("x"),
>>        MemoryLayout.ofValueBits(32, ByteOrder.nativeOrder()).withName("y")
>>     ));
>> 
>> MethodHandle sliceHandle = seq.sliceHandle(sequenceElement());
>> 
>> MemoryLayout structLayout = seq.select(sequenceElement());
>> VarHandle xHandle = structLayout.varHandle(int.class, groupElement("x"));
>> VarHandle yHandle = structLayout.varHandle(int.class, groupElement("y"));
>> 
>> try (ResourceScope scope = ResourceScope.ofConfined()) {
>>     MemorySegment segment = MemorySegment.allocateNative(seq, scope);
>>     long count = seq.elementCount().orElseThrow();
>>     for (int i = 0; i < count; i++) {
>>         // create view segment of single struct element
>>         MemorySegment struct = (MemorySegment) sliceHandle.invokeExact(segment, (long) i);
>>         xHandle.set(struct, 42);
>>         yHandle.set(struct, 84);
>>     }
>> }
>> 
>> Thanks,
>> Jorn
>
> src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemoryLayout.java line 516:
> 
>> 514:      * where {@code x_1}, {@code x_2}, ... {@code x_n} are <em>dynamic</em> values provided as {@code long}
>> 515:      * arguments, whereas {@code c_1}, {@code c_2}, ... {@code c_m} and {@code s_0}, {@code s_1}, ... {@code s_n} are
>> 516:      * <em>static</em> stride constants which are derived from the layout path.
> 
> There is an issue here, which is probably not introduced by you, but also present in other methods: c_1 and s_0 are not all strides; c_0 are offsets, while s_0 are strides.

Okay, will fix this on the other ones as well

> src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemoryLayout.java line 523:
> 
>> 521:      * }</pre></blockquote>
>> 522:      *
>> 523:      * where {@code segment} is the segment to be sliced, and where {@code layout} is the layout selected by the given
> 
> Maybe we can say "as per MemoryLayout::select(path)" ?

Sure

> src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/LayoutPath.java line 76:
> 
>> 74:             MH_ADD_SCALED_OFFSET = lookup.findStatic(LayoutPath.class, "addScaledOffset",
>> 75:                     MethodType.methodType(long.class, long.class, long.class, long.class));
>> 76:             MH_SLICE = lookup.findStatic(LayoutPath.class, "slice",
> 
> Why not just pointing to MS::asSlice?

Yeah, I'll do that. I did it like this at first just in case we wanted to change the slice API in the future, but looking now it seems kinda YAGNI

> test/jdk/java/foreign/TestLayoutPaths.java line 533:
> 
>> 531:         MethodHandle sliceHandle;
>> 532:         try {
>> 533:             sliceHandle = layout.sliceHandle(groupElement("y")); // should work
> 
> Question: why is the failure dynamic? We know exactly, at construction time that the offset is gonna be odd?

That is true in this case, but not in general where the offset could depend on some dynamic indexes.

However, I could change it to eagerly do the offset check in case we don't have any free dimensions.

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

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