[foreign-memaccess+abi] RFR: 8291639: Improve the ability to visualize a MemorySegment in human readable forms [v15]

Wed May 31 09:39:30 UTC 2023

On Wed, 31 May 2023 08:51:40 GMT, Per Minborg <pminborg at openjdk.org> wrote:

>> This PR proposes to add a "Record Mapper" that allows `MemorySegment` instances to be projected onto `Record` types given `GroupLayout` instances.
>> 
>> Here is a simple example of how to use the mapper:
>> 
>> 
>>     private static final GroupLayout POINT_LAYOUT = MemoryLayout.structLayout(
>>             JAVA_INT.withName("x"),
>>             JAVA_INT.withName("y"));
>> 
>>     MemorySegment segment =  MemorySegment.ofArray(new int[]{3, 4});
>> 
>>     Point point = POINT_LAYOUT.recordMapper(Point.class)
>>           .apply(segment); // Point[x=3, y=4]
>> 
>> 
>> I think the implementation can be improved later, for example by de-duplicating handling of arrays and maybe add recursive "un-pealing" of multidimensional arrays.
>
> Per Minborg has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 46 commits:
> 
>  - Merge foreign-memaccess+abi
>  - Remove redundant imports
>  - Use invokeExact and raw method handles for composition
>  - Move method to class
>  - Update array section doc
>  - Define offset in the javadocs
>  - Update javadoc
>  - Use explicit call rather than using a method
>  - Add variable Lookup
>  - Break apart nested switch statements
>  - ... and 36 more: https://git.openjdk.org/panama-foreign/compare/38f4bfe7...e0fb8727

Great progress. The new code structure in the mapper implementation is much clearer and the javadoc has improved a lot.

src/java.base/share/classes/java/lang/foreign/GroupLayout.java line 88:

> 86:      * (which is zero for the initial invocation of this method) is defined as follows {@code M(type, layout, offset)};
> 87:      * <p>
> 88:      * Let {@code R} comprise its the constituent components

Maybe simplify: "Let C1, C2 ... Cn" be the components of R (with N non-negative)"

src/java.base/share/classes/java/lang/foreign/GroupLayout.java line 96:

> 94:      * Let {@code L} be a group layout with the elements {@code e_1, e_2, ..., e_M}, where {@code M >= N}.
> 95:      * <p>
> 96:      * Let {@code offsetBetween(GroupLayout layout, MemoryLayout target)} be a function that, via

I would use two letters as the argument of `offsetBetween`. E.g.
"Let offsetOf(L, LL) be a function that, via ... , can compute the offset from the layout L to a sub-layout LL"
Note the change of name. I don't think "between" is a good choice of word.

src/java.base/share/classes/java/lang/foreign/GroupLayout.java line 100:

> 98:      * {@code layout L} to a sub-layout of {@code L}.
> 99:      * <p>
> 100:      * Then, for each {@code c_a, a <= N}, there must be a corresponding distinct

I thought letters `c` were for record components? - but you say "name of c_a" ?

src/java.base/share/classes/java/lang/foreign/GroupLayout.java line 102:

> 100:      * Then, for each {@code c_a, a <= N}, there must be a corresponding distinct
> 101:      * {@code e_b} such that the {@link MemoryLayout#name() name} of {@code c_a}
> 102:      * {@link Object#equals(Object) equals} the {@link RecordComponent#getName() name} of {@code e_b} and:

such that the name ... and the name ... are the same

src/java.base/share/classes/java/lang/foreign/GroupLayout.java line 107:

> 105:      *        <h4>If {@code e_b} is a {@link ValueLayout };</h4>
> 106:      *        then {@code c_a} must be of the exact type of {@code e_b}'s {@link ValueLayout#carrier() carrier()}<br>
> 107:      *        whereby {@code c_a = f_a(MemorySegment ms) = ms.get(e_b, offset + offsetBetween(layout, e_b))}.<br>

It is not defined what "offset" is. I believe that has to be a parameter to the f_a - e.g. the function takes a segment *and* and offset.

src/java.base/share/classes/java/lang/foreign/GroupLayout.java line 119:

> 117:      *        <h4>If {@code e_b} is a {@link SequenceLayout };</h4>
> 118:      *        then {@code c_a} must be an array {@code C[]^D} (an array of depth {@code D}
> 119:      *        and with an array component type {@code C}) that can be mapped to {@code e_b} via a resulting

Can't all this now be expressed more simply using recursion? E.g. given the element layout E and the component type C (which could be another array), we compute, for each element in the sequence layout, the mapping between E, C and the corresponding element offset. Then we return new C[] with all the elements so computed. If you propagate the offsets correctly it should "just work" ?

src/java.base/share/classes/java/lang/foreign/GroupLayout.java line 143:

> 141:      *    </li>
> 142:      *    <li>
> 143:      *        <h4>If {@code e_b} is a {@link PaddingLayout };</h4>

It feels like these two last bullets can be merged into "otherwise throw". Maybe that text can make an example of when that could happen (e.g. padding layout or String carrier).

src/java.base/share/classes/jdk/internal/foreign/LayoutRecordMapper.java line 72:

> 70:     public LayoutRecordMapper(Class<T> type,
> 71:                               GroupLayout layout) {
> 72:         this(type, layout, 0L, 0, PUBLIC_LOOKUP);

I tend to agree with Remi that passing an explicit lookup to the mapper factory is unavoidable here.

src/java.base/share/classes/jdk/internal/foreign/LayoutRecordMapper.java line 98:

> 96:                 .toList();
> 97: 
> 98:         System.out.println("handles for " + type + " are " + handles);

This is a leftover from some debugging code? There's also some others.

src/java.base/share/classes/jdk/internal/foreign/LayoutRecordMapper.java line 179:

> 177:         }
> 178: 
> 179:         MultidimensionalSequenceLayoutInfo info = MultidimensionalSequenceLayoutInfo.of(sl, componentType);

Now that we have offsets in the right places, I feel like this can be simplified using recursion (similarly to what I proposed to do in the javadoc). Although here we need to special case element layouts for which a faster option is available. So, something like:

* if element layout is a value layout V, use segment.toArray(V)
* otherwise, retrieve the objects corresponding to the array elements (using the mapper recursively) and return an array of them (it is likely that this part is more imperative, and harder to express with method handles).

The key difference is that the second step above only concerns about going "one dimension deeper".

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

PR Review: https://git.openjdk.org/panama-foreign/pull/833#pullrequestreview-1452591520
PR Review Comment: https://git.openjdk.org/panama-foreign/pull/833#discussion_r1211342165
PR Review Comment: https://git.openjdk.org/panama-foreign/pull/833#discussion_r1211345680
PR Review Comment: https://git.openjdk.org/panama-foreign/pull/833#discussion_r1211347477
PR Review Comment: https://git.openjdk.org/panama-foreign/pull/833#discussion_r1211348475
PR Review Comment: https://git.openjdk.org/panama-foreign/pull/833#discussion_r1211350481
PR Review Comment: https://git.openjdk.org/panama-foreign/pull/833#discussion_r1211356607
PR Review Comment: https://git.openjdk.org/panama-foreign/pull/833#discussion_r1211358135
PR Review Comment: https://git.openjdk.org/panama-foreign/pull/833#discussion_r1211361033
PR Review Comment: https://git.openjdk.org/panama-foreign/pull/833#discussion_r1211362706
PR Review Comment: https://git.openjdk.org/panama-foreign/pull/833#discussion_r1211374189