[foreign-memaccess+abi] RFR: 8291826: Rework MemoryLayout Sealed Hierarchy

[Paul Sandoz]

On Thu, 18 Aug 2022 13:59:05 GMT, Per Minborg <duke at openjdk.org> wrote:

> This PR is a significant refactor of the `MemoryLayout` sealed hierarchy and whose purpose is described more in detail in  https://bugs.openjdk.org/browse/JDK-8291826. In short, the main purpose is to allow pattern matching.
> 
> The objective of the PR is to fix the issues described above while retaining as much API compatibility as possible with the pre-PR API. No changes were needed in any of the existing tests for example.
> 
> The old sealed hierarchy prevented pattern matching with totality from being used and implied a number of other problems as illustrated in the picture below:
> 
> ![graphviz (33)](https://user-images.githubusercontent.com/7457876/185412743-febeb3cf-aebc-4e2e-b78f-3b8ef87b0510.png)
> 
> Red nodes cannot be used in pattern matching. Light red nodes are implementations that are visible in the API. White nodes are interfaces.
> 
> After this PR has been integrated, an improved sealed hierarchy will exist as depicted here:
> 
> ![graphviz (34)](https://user-images.githubusercontent.com/7457876/185413448-8435b23f-5ac4-4999-8b0a-0d6051a0d9be.png)
> 
> Gray "Impl" nodes are internal and not exported by the `java.base` module. White nodes are interfaces.
> 
> Below, the "Principal Totalities" for the new API are outlined:
> 
> 
> var v0 = switch (memoryLayout) {
>   case MemoryLayout ml -> 0;
> };
> var v1 = switch (memoryLayout) {
>   case GroupLayout gl -> 0;
>   case PaddingLayout pl -> 0; // leaf
>   case SequenceLayout sl -> 0; // leaf
>   case ValueLayout vl -> 0;
> };
> var v2 = switch (memoryLayout) {
>   case PaddingLayout pl -> 0; // leaf
>   case SequenceLayout sl -> 0; // leaf
>   case ValueLayout vl -> 0;
>   case StructLayout sl -> 0; // leaf
>   case UnionLayout ul -> 0; // leaf
> };
> var v3 = switch (memoryLayout) {
>   case PaddingLayout pl -> 0; // leaf
>   case SequenceLayout sl -> 0; // leaf
>   case StructLayout sl -> 0; // leaf
>   case UnionLayout ul -> 0; // leaf
>   case OfAddress oa -> 0; // leaf
>   case OfBoolean ob -> 0; // leaf
>   case OfByte ob -> 0; // leaf
>   case OfChar oc -> 0; // leaf
>   case OfDouble od -> 0; // leaf
>   case OfFloat of -> 0; // leaf
>   case OfInt oi -> 0; // leaf
>   case OfLong ol -> 0; // leaf
>   case OfShort os -> 0; // leaf
> };
> var v4 = switch (memoryLayout) {
>   case GroupLayout gl -> 0;
>   case PaddingLayout pl -> 0; // leaf
>   case SequenceLayout sl -> 0; // leaf
>   case OfAddress oa -> 0; // leaf
>   case OfBoolean ob -> 0; // leaf
>   case OfByte ob -> 0; // leaf
>   case OfChar oc -> 0; // leaf
>   case OfDouble od -> 0; // leaf
>   case OfFloat of -> 0; // leaf
>   case OfInt oi -> 0; // leaf
>   case OfLong ol -> 0; // leaf
>   case OfShort os -> 0; // leaf
> };
> 
> 
> 
>  Additional improvements can be made later in a separate PR, including removing/improving some of the internal abstract classes.

It generally looks good and consistent. A few comments inline.

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

> 57:      * {@return {@code true}, if this group layout is a struct layout}
> 58:      */
> 59:     boolean isStruct();

If we are going to lean into pattern matching then the mutually exclusive methods `isStruct` and `isUnion` are not necessary. Perhaps a change to consider this refactoring.

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

> 618:     static PaddingLayout paddingLayout(long size) {
> 619:         MemoryLayoutUtil.checkSize(size);
> 620:         return PaddingLayoutImpl.create(size);

Suggestion:

        return PaddingLayoutImpl.of(size);

src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 1159:

> 1157:     @ForceInline
> 1158:     default byte get(ValueLayout.OfByte layout, long offset) {
> 1159:         return (byte) ((InternalValueLayout) layout).accessHandle().get(this, offset);

`InternalValueLayout` seems an unnecessary internal abstraction since we already have an internal abstract implementation of `ValueLayout` and further the concrete value implementations are accessible to this class we can just use those (`ValueLayouts.Of{X}Impl`). We should check that performance does not regress by any changes to these access methods.

test/jdk/java/foreign/MemoryLayoutPrincipalTotalityTest.java line 27:

> 25:  * @test
> 26:  * @enablePreview
> 27:  * @run testng/othervm --enable-native-access=ALL-UNNAMED MemoryLayoutPrincipalTotalityTest

Is `--enable-native-access=ALL-UNNAMED` required?

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

PR: https://git.openjdk.org/panama-foreign/pull/710