[foreign-memaccess+abi] RFR: 8291639: Improve the ability to visualize a MemorySegment in human readable forms [v18]
Jorn Vernee
jvernee at openjdk.org
Mon Jun 5 15:44:44 UTC 2023
On Wed, 31 May 2023 12:05:43 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 incrementally with one additional commit since the last revision:
>
> Clean up code
src/java.base/share/classes/java/lang/foreign/GroupLayout.java line 81:
> 79:
> 80: /**
> 81: * {@return a {@link Function} that can project {@linkplain MemorySegment MemorySegments} onto new instances
Since a new record is returned, 'into' seems more correct. ('onto' sounds like there is an existing record instance).
Suggestion:
* {@return a {@link Function} that can project {@linkplain MemorySegment MemorySegments} into new instances
src/java.base/share/classes/java/lang/foreign/GroupLayout.java line 86:
> 84: * <p>
> 85: * A mapper {@code M} between a record type {@code R}, a memory layout {@code L} and an {@code offset}
> 86: * (which is zero for the initial invocation of this method) is defined as follows {@code M(type, layout, offset)};
Where is `offset` coming from? It's not a parameter of this method. Why is it _initially_ zero?
src/java.base/share/classes/java/lang/foreign/GroupLayout.java line 94:
> 92: * a {@link MemorySegment} {@code ms} and produces a value for {@code c_a}.
> 93: * <p>
> 94: * Let {@code L} be a group layout with the elements {@code e_1, e_2, ..., e_M}, where {@code M >= N}.
WRT terminology: in other documentation 'elements' is used for sequence layouts, but 'member layouts' is used to GroupLayouts. So, I think the latter should be used here as well.
src/java.base/share/classes/java/lang/foreign/GroupLayout.java line 111:
> 109: * <li>
> 110: * <h4>If {@code e_b} is a {@link GroupLayout };</h4>
> 111: * then {@code e_a} must be of another {@link Record} type {@code R2}
Suggestion:
* then {@code c_a} must be of another {@link Record} type {@code R2}
src/java.base/share/classes/java/lang/foreign/GroupLayout.java line 112:
> 110: * <h4>If {@code e_b} is a {@link GroupLayout };</h4>
> 111: * then {@code e_a} must be of another {@link Record} type {@code R2}
> 112: * (such that {@code R2 != R}) that can be mapped to {@code e_b} via
I don't think you have a test for `(such that {@code R2 != R})`?
src/java.base/share/classes/java/lang/foreign/GroupLayout.java line 217:
> 215: * @since 21
> 216: */
> 217: <R extends Record> Function<MemorySegment, R> recordMapper(Class<R> type);
I feel like the documentation is a bit _too_ formal, and will go over some people's heads. In general, I suggest using plain English as the main structure, and then weaving in the formal definitions instead.
I think by re-ordering some of the information, you can end up with simpler/smaller paragraphs. I think there are 3 main steps that are useful to describe:
1. The `recordMapper` function itself finds, for each record component, a corresponding member layout with the same name in this group layout. There are restrictions on the layout type of each corresponding member layout based on the type of the record component (e.g. a java primitive maps to a ValueLayout, MemorySegment maps to an AddressLayout, etc.)
2. An extraction function is determined based on the component/layout type combination (and then have a list with the process for each type combination). In this part you can lean on the information defined in 1, and use language like "the corresponding member layout"/"the corresponding record component", rather than `c_a` and `e_b`.
3. The canonical constructor of the record type is called with the result of the extraction functions.
I think 1 & 2 are too mixed up right now in the big formal definition of the extraction functions.
src/java.base/share/classes/jdk/internal/foreign/LayoutRecordMapper.java line 124:
> 122: throw new IllegalArgumentException("There is no public constructor in '" + type.getName() +
> 123: "' for " + Arrays.toString(ctorParameterTypes) + " using lookup " + lookup, e);
> 124: }
Please shrink the `try` block to be just around the call to `findConstructor`.
src/java.base/share/classes/jdk/internal/foreign/LayoutRecordMapper.java line 154:
> 152: mh = MethodHandles.insertArguments(mh, 1, vl);
> 153: // (MemorySegment, long) -> (MemorySegment)
> 154: return MethodHandles.insertArguments(mh, 1, byteOffset);
I suggest just creating a var handle for this component using `MemoryLayout::varHandle` and then projecting it into a MethodHandle using `VarHandle::toMethodHandle`.
src/java.base/share/classes/jdk/internal/foreign/LayoutRecordMapper.java line 570:
> 568: int[] dimensions = info.dimensions();
> 569: // Create the array to return
> 570: Object result = Array.newInstance(leafType, dimensions);
This is a bit wasteful when we have more than 2 dimensions, since this creates a bunch of nested array objects, which are then overridden below by the recursive call to `toMultiArrayFunction`.
I agree with Maurizio that the array handling seems like it can be made fully recursive for arrays of any dimension.
test/jdk/java/foreign/TestRecordMapper.java line 461:
> 459: MemoryLayout.sequenceLayout(3,
> 460: MemoryLayout.sequenceLayout(4, JAVA_INT).withName("whatever2")
> 461: ).withName("whatever")
Are the names for the nested layouts required?
test/jdk/java/foreign/TestRecordMapper.java line 674:
> 672:
> 673: @Test
> 674: public void testMhComposition() throws Throwable {
I guess this test is a leftover? It doesn't seem to use `recordMapper`.
test/jdk/java/foreign/TestRecordMapper.java line 718:
> 716:
> 717: @Test
> 718: public void testIntegerBoxing() {
Same here, `EXACT` is always `true` so these are essentially disabled tests.
test/jdk/java/foreign/TestRecordMapper.java line 767:
> 765:
> 766: @Test
> 767: public void inspectMemory() {
This also doesn't use `recordMapper`
test/micro/org/openjdk/bench/java/lang/foreign/RecordMapper.java line 82:
> 80: arena = Arena.ofConfined();
> 81: pointSegment = arena.allocate(POINT_LAYOUT);
> 82: var rnd = new Random();
You should use an explicit seed (e.g. `0`) so that the data is the same on each run of the benchmark.
-------------
PR Review Comment: https://git.openjdk.org/panama-foreign/pull/833#discussion_r1218243979
PR Review Comment: https://git.openjdk.org/panama-foreign/pull/833#discussion_r1218066967
PR Review Comment: https://git.openjdk.org/panama-foreign/pull/833#discussion_r1218072478
PR Review Comment: https://git.openjdk.org/panama-foreign/pull/833#discussion_r1218078138
PR Review Comment: https://git.openjdk.org/panama-foreign/pull/833#discussion_r1218196678
PR Review Comment: https://git.openjdk.org/panama-foreign/pull/833#discussion_r1218209462
PR Review Comment: https://git.openjdk.org/panama-foreign/pull/833#discussion_r1218093141
PR Review Comment: https://git.openjdk.org/panama-foreign/pull/833#discussion_r1218106868
PR Review Comment: https://git.openjdk.org/panama-foreign/pull/833#discussion_r1218152749
PR Review Comment: https://git.openjdk.org/panama-foreign/pull/833#discussion_r1218170700
PR Review Comment: https://git.openjdk.org/panama-foreign/pull/833#discussion_r1218176479
PR Review Comment: https://git.openjdk.org/panama-foreign/pull/833#discussion_r1218177397
PR Review Comment: https://git.openjdk.org/panama-foreign/pull/833#discussion_r1218179543
PR Review Comment: https://git.openjdk.org/panama-foreign/pull/833#discussion_r1218191956
More information about the panama-dev
mailing list