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

Fri May 26 14:46:22 UTC 2023

On Fri, 26 May 2023 13:06:57 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 29 commits:
> 
>  - Merge master
>  - Fix typos
>  - Clean up toString methods
>  - Add support for MemoryAddress arrays and improve docs
>  - Improve javadoc
>  - Change to IAE if mapper fails
>  - Remove caching
>  - Add tests for initial issue examples
>  - Clean up and optimize
>  - Add support for multidimensional records
>  - ... and 19 more: https://git.openjdk.org/panama-foreign/compare/bee4503b...1d7db589

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

> 86:      * <p>
> 87:      * Let <em>R</em> be the provided record {@code type} with the constituent components
> 88:      * <em>C<sub>0</sub></em>, ..., <em>C<sub>N-1</sub></em>, where <em>N</em> is non-negative.

general stylistic note - you some fancy html here to get close to some math notation. This is nice, but is also inconsistent with what has been done in other parts of the memory layout API - see e.g. MemoryLayout::var handle, which just uses `A_0`, `A_1` etc.

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

> 91:      * , where <em>M</em> {@code >=} <em>N</em>.
> 92:      * <p>
> 93:      * Then, for each <em>C<sub>a</sub></em> (<em>a</em> {@code <} <em>N</em>), there must be a corresponding distinct

As discussed offline, I think the documentation would be clearer (and more precise) if we:

* Defined some helper function `offsetOf(L, LL)` which gives offset of layout LL in L (no need to specify how it's done, just use it as an explanation device (perhaps hinting at `MemoryLayout::byteOffset`
* Defined a mapping from (offset, layout, class<T>) to a value of type T. This mapping needs to be defined for _all_ layouts
* This allow us to define the sequence case as just using recursion: e.g. given the triple (O, Sequence(E), T[]), for each element E at offset OO = O + offsetOf(Sequence(E), E), we compute the value associated with (OO, E, T). Then we collect all such values into a new T[] array (whose size is the same as that of the sequence layout). I believe this does the right thing even in the case of multi-dimensional arrays.
* Then , recordMapper(G, R) is defined simply as the value associated with the triple (0, G, R).

src/java.base/share/classes/java/lang/foreign/MemoryLayout.java line 168:

> 166:  * <h2 id="record-mapping">Record mapping</h2>
> 167:  * <p>A {@linkplain GroupLayout group layout} can provide mapping capabilities from memory segments to Java
> 168:  * {@linkplain Record Records} by means of matching named elements in the group layout with component names

This should be a link to the JLS section for records - we do that in other parts of the javadoc - e.g.:

Note the use of the <em>try-with-resources</em> construct: this idiom ensures that the off-heap region
 * of memory backing the native segment will be released at the end of the block, according to the semantics described
 * in Section {@jls 14.20.3} of <cite>The Java Language Specification</cite>.
 ```

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

> 64:         implements Function<MemorySegment, T> {
> 65: 
> 66:     enum Allow {EXACT, BOXING, BOXING_NARROWING_AND_WIDENING}

I'd prefer if this implementation class (which is already complex) started off simple, and provided only mappings for doing exact match (which is what the API point says anyway). There is some noise that is introduced later on to deal with wrapper classes, and I'd like us to focus on the basics first.

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

> 87: 
> 88:         var recordComponents = Arrays.asList(type.getRecordComponents());
> 89:         var recordComponentNames = recordComponents.stream()

I have a feeling that the use of streams here is hiding a much simpler loop where we loop over the record components, we check that each of them have a corresponding layout in the Group. For instance:

Map<String, MemoryLayout> nameToLayoutMap = ...
for (RecordComponent rc : type.getRecordComponents()) {
    try {
         MemoryLayout memberLayout = layout.select(PathElement.groupElement(rc.name());
         nameToLayoutMap = nameToLayoutMap(memberLayout.name().get(), memberLayout);
    } catch (IllegalArgumentException ex) {
         throw new IllegalArgumentException("There is no mapping...");
    }
}

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

> 177: 
> 178:                                 // Handle multi-dimensional arrays
> 179:                                 if (info.sequences().size() > 1) {

I have a feeling that this could be simplified if we embraced recursion, and defined the mapping as a function taking three arguments: offset, layout and carrier. The output of that function is a MethodHandle. Record mapper is not special - I mean, it is special as that's the only entry point we provide in the API, but that is based on a more general way to map layouts and carriers to value, so we should probably write that general function first, and then map that to the recordMapper API point.

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

PR Review Comment: https://git.openjdk.org/panama-foreign/pull/833#discussion_r1206851511
PR Review Comment: https://git.openjdk.org/panama-foreign/pull/833#discussion_r1206860611
PR Review Comment: https://git.openjdk.org/panama-foreign/pull/833#discussion_r1206865064
PR Review Comment: https://git.openjdk.org/panama-foreign/pull/833#discussion_r1206882508
PR Review Comment: https://git.openjdk.org/panama-foreign/pull/833#discussion_r1206890942
PR Review Comment: https://git.openjdk.org/panama-foreign/pull/833#discussion_r1206875188