[foreign-memaccess] RFR: 8249879: Split MemorySegment and MemoryAddress

[Henry Jen]

On Wed, 22 Jul 2020 15:12:29 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

> This patch splits, as the title suggests, the memory segment abstraction from the memory address abstraction, as
> described in this email:
> https://mail.openjdk.java.net/pipermail/panama-dev/2020-July/009928.html
> 
> Aside from the obvious updates to the two interfaces (and their implementation), this patch greatly simplifies the way
> memory access var handles are generated; instead of spinning them on the fly (see `MemoryAccessVarHandleGenerator`,
> which is now removed) now all var handles can be derived from a primitive from which takes a segment and a byte offset.
> This caused the removal of several methods in the `JavaLangInvokeAccess` interface.  The new memory segment interface
> has some more methods to perform slicing - which can basically be used to give a new base to an existing segment and
> can therefore be used in a lot of cases as a replacement for `MemoryAddress::addOffset`.  The memory address interface
> is simplified, and features an additional `segmentOffset` method, which returns the byte offset of an address relative
> to the given segment (this is similar to the old `MemorySegment::rebase` method, which is now removed).  The
> `MemoryAccess` class needed a lot of tweaks in the various signatures, to take a segment instead of a base address.  I
> tried to update the documentation as best as I could, but it's possible I missed some references to the old
> relationship between segment and addresses, please double check.  Of course integrating this patch is gonna cause
> issues in foreign-abi and foreign-jextract, as these branches make (heavy) use of memory access var handles. I suspect
> that integration will trigger a merge failure, so we can fix followup issues as part of the merge process, so that we
> can guarantee that all the branches remain buildable.

Overall looks good to me.
The change in LayoutPath looks reasonable, but I cannot assure I fully get it. Assuming we have enough test coverage. :)

src/java.base/share/classes/java/lang/invoke/MemoryAccessVarHandleBase.java line 42:

> 41:
> 42:     /** alignment constraint (in bytes, expressed as a bit mask) **/
> 43:     final boolean skipOffsetCheck;

comment need to be updated

src/java.base/share/classes/java/lang/invoke/VarHandles.java line 314:

> 313:      * @param carrier the Java carrier type.
> 314:      * @param alignmentMask alignment requirement to be checked upon access. In bytes. Expressed as a mask.
> 315:      * @param byteOrder the byte order.

need comment for skipOffsetCheck

src/java.base/share/classes/java/lang/invoke/X-VarHandleMemoryAccess.java.template line 107:

> 106:                 throw MemoryAccessVarHandleBase.newIllegalStateExceptionForMisalignedAccess(address);
> 107:             }
> 108:         }

Seems a little odd to me, as there is no performance gain with skipOffsetCheck.

The else block check only address but not base and offset separately is not the same, although I think that's correct
as it's what matters.

However, if this is desired, I don't see why we don't simply check address without the difference.

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegment.java line 288:

> 287:      * @return a new memory segment view with updated base/limit addresses.
> 288:      * @throws IndexOutOfBoundsException if {@code offset < 0}, {@code offset > byteSize()}, {@code newSize < 0},
> or {@code newSize > byteSize() - offset} 289:      */

Suggestion:

     * @throws IndexOutOfBoundsException if {@code offset < 0}, {@code offset > byteSize()}

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegment.java line 298:

> 297:      * @return a new memory segment view with updated base/limit addresses.
> 298:      * @throws IndexOutOfBoundsException if {@code offset < 0}, {@code offset > byteSize()}, {@code newSize < 0},
> or {@code newSize > byteSize() - offset} 299:      */

address.segmentOffset can throw IllegalArgumentException.
As IndexOutOfBoundException maybe simply saying the calculated offset < 0 or > byteSize()

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

Marked as reviewed by henryjen (Committer).

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