[foreign-memaccess+abi] RFR: 8310362: Improve composability of handle derived from layouts
Maurizio Cimadamore
mcimadamore at openjdk.org
Wed Jun 21 14:06:36 UTC 2023
On Tue, 20 Jun 2023 12:49:41 GMT, Jorn Vernee <jvernee at openjdk.org> wrote:
> Me and Maurizio have been discussing how to better address certain use cases like accessing structs with variable length array members, or matrices that have a dynamic row and column size.
>
> The solution we came up with is captured in this patch. It adds an additional base offset parameter to the handles produced by `MemoryLayout::varHandle`, `MemoryLayout::sliceHandle` and `MemoryLayout::byteOffsetHandle`. This allows these handles to be composed with other offset computations that are not necessarily derived from layouts.
>
> This patch also adds a `scale` method to MemoryLayout, which can be used to scale and index by the size of a layout. Essentially: `offset + layout.byteSize() * index`. This is useful for expressing ad-hoc offset computations. A `scaleHandle` method is also added for convience, which returns a MethodHandle for `scale` bound to the receiver layout.
>
> Both these changes allowed for several simplifications:
> - `MethodHandles::memorySegmentViewVarHandle` can be removed, as it's now exactly the same as calling ML::varHandle on a ValueLayout with an empty layout path. (the former method also didn't take advantage of the access handle cache found on ValueLayouts).
> - `ValueLayout::arrayHandle` is removed, as most use cases can now simply be expressed using ML::varHandle + ML::scaleHandle. e.g.:
>
> MethodHandles.collectCoordinates(layout.varHandle(),
> 1, MethodHandles.insertArguments(layout.scaleHandle(), 0, 0L));
Overall looks very good. I like how it unifies access to memory segments (now VH access is similar to e.g. MS::get), and how there's less types of var handles to keep track of. The fact that var handles obtained with the MethodHandles combinator were not 100% similar to the ones obtained from layouts has always been a bit of a smell, and this patch neatly unifies the two. Similarly, it was a bit sad that the implementation of MS::get was using "super powers" that were not available to regular developers (ValueLayout::accessHandle). All that is gone now, which is very good.
I might have missed something but I did not see tests for the new scale/scaleHandle methods.
src/java.base/share/classes/java/lang/foreign/MemoryLayout.java line 236:
> 234: * in an {@link IllegalArgumentException}.
> 235: *
> 236: * <h2 id="access-mode-restrictions">Access mode restrictions</h2>
I like the idea of having all the rules there - not 100% on the title of the section, but I can't think of a better one (and good enough for now)
src/java.base/share/classes/java/lang/foreign/MemoryLayout.java line 240:
> 238: * The var handles returned by {@link #varHandle(PathElement...)} and {@link ValueLayout#varHandle()} feature certain
> 239: * <i>access mode restrictions</i>. A var handle is associated with an access size {@code S}, derived from the
> 240: * {@linkplain ValueLayout#byteSize() byte size} of this layout, and an alignment constraint {@code B}, derived from the
`of this layout` looks weird for a toplevel javadoc section (here and elsewhere)
src/java.base/share/classes/java/lang/foreign/MemoryLayout.java line 426:
> 424: * <li>its type is derived from the {@linkplain ValueLayout#carrier() carrier} of the
> 425: * selected value layout;</li>
> 426: * <li>it has two leading access coordinates of type {@link MemorySegment} and {@code long},
Since we're in a bullet list, should we have distinct bullets for segment/offset? (this comment is repeated also below)
src/java.base/share/classes/java/lang/foreign/MemoryLayout.java line 456:
> 454: * {@code 0 <= x_i < b_i}, where {@code 1 <= i <= n}, or {@link IndexOutOfBoundsException} is thrown.
> 455: * <p>
> 456: * Multiple path sections can be chained, with <a href=#deref-path-elements>dereference path elements</a>.
I'm not too fond of the "path section" concept. Perhaps we could just talk about paths and sub-paths? Not 100% sure. A possible angle would be to show that a path containing dereference elements is broken up into distinct paths, which are then joined together using combinators.
src/java.base/share/classes/java/lang/foreign/MemoryLayout.java line 516:
> 514: * <ul>
> 515: * <li>its return type is {@code MemorySegment};</li>
> 516: * <li>it has two leading parameter of type {@code MemorySegment} and {@code long}, corresponding to the memory segment
Since we're in a bullet list, should we have distinct bullets for segment/offset?
src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 130:
> 128: * }
> 129: *
> 130: * More complex access operations can be expressed using var handles. The {@link ValueLayout#varHandle()}
Expressed or Implemented?
src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 131:
> 129: *
> 130: * More complex access operations can be expressed using var handles. The {@link ValueLayout#varHandle()}
> 131: * method can be used to obtain a var handle that can be used as a getter or setter for values represented by the given value layout.
I think I'd prefer avoiding the term getter/setter - e.g. "can be used to get/set values represented by the given value layout __on a memory segment__" (maybe also "at the given offset?).
src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 132:
> 130: * More complex access operations can be expressed using var handles. The {@link ValueLayout#varHandle()}
> 131: * method can be used to obtain a var handle that can be used as a getter or setter for values represented by the given value layout.
> 132: * It supports the plain get and set access modes just like the get and set methods found on {@link MemorySegment}, but it also
"It" seems a bit weak here. Perhaps we should repeat "A var handle obtained from a layout...". Also, not sure if it's too much detail to dwell on access modes here, since we have a brand new javadoc section on that? Maybe a link is enough?
src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 136:
> 134: * or {@linkplain java.lang.invoke.VarHandle#compareAndExchange(Object...) compare and exchange}. More importantly, var
> 135: * handles can be <em>combined</em> with method handles to express complex access operations. For instance, a var handle
> 136: * that expresses access to an element of an {@code int} array can be created as follows:
expressed access -> expressed indexed access
or better "a var handle that can be used to access an int element at the given logical index"
src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 141:
> 139: * MemorySegment segment = ...
> 140: * VarHandle intHandle = ValueLayout.JAVA_INT.varHandle(); // (MemorySegment, long)
> 141: * MethodHandle scale = ValueLayout.JAVA_INT.scaleHandle(); // <base offset> + <index> * JAVA_INT.byteSize()
Much nicer example than the one it replaces!
src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 155:
> 153: * {@snippet lang=java :
> 154: * MemorySegment segment = ...
> 155: * MemoryLayout segmentLayout = MemoryLayout.sequenceLayout(4, ValueLayout.JAVA_INT); // array of 4 elements
perhaps if we used a struct which contained an array, the example would drive the point home better?
src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 163:
> 161: * {@link MemoryLayout#varHandle(MemoryLayout.PathElement...)}, as well as the method handle returned by
> 162: * {@link MemoryLayout#byteOffsetHandle(MemoryLayout.PathElement...)} and {@link MemoryLayout#sliceHandle(MemoryLayout.PathElement...)}
> 163: * feature a <em>base offset</em> parameter. This parameter represents a base offset for the offset computation. This extra
Maybe drop "extra" - e.g. I wonder if we're writing "extra" as a justification as to why it's there coming from the old API
src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 164:
> 162: * {@link MemoryLayout#byteOffsetHandle(MemoryLayout.PathElement...)} and {@link MemoryLayout#sliceHandle(MemoryLayout.PathElement...)}
> 163: * feature a <em>base offset</em> parameter. This parameter represents a base offset for the offset computation. This extra
> 164: * parameter allows a user to combine these handles further with additional offset computations. This is demonstrated
I believe we tend to use "clients" instead of "users" throughout the javadoc
src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 1501:
> 1499: @ForceInline
> 1500: default void set(ValueLayout.OfBoolean layout, long offset, boolean value) {
> 1501: layout.varHandle().set(this, offset, value);
NICE!
src/java.base/share/classes/java/lang/foreign/SegmentAllocator.java line 145:
> 143: Objects.requireNonNull(layout);
> 144: MemorySegment addr = allocate(layout);
> 145: addr.set(layout, 0, value);
also nice!
src/java.base/share/classes/java/lang/foreign/ValueLayout.java line 98:
> 96:
> 97: /**
> 98: * {@return a var handle which can be used to access values represented by this value layout, in a given memory segment.}
"values represented by this layout" I don't think is a concept we have. Perhaps a slightly more colloquial "values described by this layout" would work?
src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 7957:
> 7955: }
> 7956:
> 7957: /**
Whohooo!
-------------
PR Review: https://git.openjdk.org/panama-foreign/pull/840#pullrequestreview-1490590946
PR Comment: https://git.openjdk.org/panama-foreign/pull/840#issuecomment-1600899345
PR Review Comment: https://git.openjdk.org/panama-foreign/pull/840#discussion_r1236909950
PR Review Comment: https://git.openjdk.org/panama-foreign/pull/840#discussion_r1236970222
PR Review Comment: https://git.openjdk.org/panama-foreign/pull/840#discussion_r1236972982
PR Review Comment: https://git.openjdk.org/panama-foreign/pull/840#discussion_r1236905661
PR Review Comment: https://git.openjdk.org/panama-foreign/pull/840#discussion_r1236907311
PR Review Comment: https://git.openjdk.org/panama-foreign/pull/840#discussion_r1236991476
PR Review Comment: https://git.openjdk.org/panama-foreign/pull/840#discussion_r1236987376
PR Review Comment: https://git.openjdk.org/panama-foreign/pull/840#discussion_r1236989728
PR Review Comment: https://git.openjdk.org/panama-foreign/pull/840#discussion_r1236990891
PR Review Comment: https://git.openjdk.org/panama-foreign/pull/840#discussion_r1236992074
PR Review Comment: https://git.openjdk.org/panama-foreign/pull/840#discussion_r1236993977
PR Review Comment: https://git.openjdk.org/panama-foreign/pull/840#discussion_r1236996128
PR Review Comment: https://git.openjdk.org/panama-foreign/pull/840#discussion_r1236994620
PR Review Comment: https://git.openjdk.org/panama-foreign/pull/840#discussion_r1236996633
PR Review Comment: https://git.openjdk.org/panama-foreign/pull/840#discussion_r1236997236
PR Review Comment: https://git.openjdk.org/panama-foreign/pull/840#discussion_r1236999137
PR Review Comment: https://git.openjdk.org/panama-foreign/pull/840#discussion_r1237042321
More information about the panama-dev
mailing list