RFR: 8308645: Javadoc of FFM API needs to be refreshed [v4]

Jorn Vernee jvernee at openjdk.org
Thu Jun 1 20:27:35 UTC 2023 

On Mon, 29 May 2023 10:53:52 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> As the FFM API evolved over time, some parts of the javadoc went out of sync. Now that most of the API is stable, it is a good time to look again at the javadoc as a whole, and bring some more consistency.
>> 
>> While most of the changes in this PR are stylistic, I'd like to call out few things which resulted in API changes:
>> 
>> * `MemorySegment::asSlice(long, long, long)` did not (incorrectly) specified requirement that its alignment parameter must be a power of two.
>> 
>> * `MemoryLayout::sliceHandle` did not require the absence of dereference paths in the provided layout path. While that is technically possible to allow, currently the method is specified as returning a "slice" corresponding to some "nested layout", so following pointers seems incompatible with the spec. I have decided to disallow for now - we can always compatibly enable it later, if required.
>> 
>> * `MemorySegment::copy` - most of the overloads did not specify that `UnsupportedOperationException` is thrown if the destination segment is read-only.
>> 
>> * In several places, an extra `@throws IllegalArgumentException` or `@throws IllegalArgumentException` has been added to capture cases where element size * index computation can overflow. This is true for all the element-wise segment copy methods, for the indexed accessors in memory segment (e.g. `MemorySegment.getAtIndex(ValueLayout.OfInt, long)`), as well as for `SegmentAllocator::allocateArray(MemoryLayout, long)`.
>> 
>> In all these cases (except for overflows), new tests have been added to cover the additional API changes (a CSR will also be filed to cover these).
>> 
>> The class with most changes is `MemoryLayout`. I realized that the javadoc there was a bit sloppy around the definition of "layout paths". Now there are several subsection in the class javadoc, which explain how different kinds of paths can be used. I have introduced the notion of "open path elements" to denote those path elements that are not fully specified, and result in additional var handle coordinates to be added. There is also now a definition of what it means for a layout path to be "well-formed", so that all methods accepting a layout path can simply refer to it (this definition is tricky to give inline in the javadoc of the various path-accepting methods, as it depends on many factors).
>> 
>> Another biggie change was in `MemorySegment` as now all dereference methods state precisely their spatial checks requirements, as well also specifying when the API can th...
>
> Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix wrong link in layout well-formedness doc

Overall a great cleanup, nice work!

src/java.base/share/classes/java/lang/foreign/FunctionDescriptor.java line 91:

> 89:     /**
> 90:      * Returns a function descriptor with no return layout.
> 91:      * @return a new function descriptor, with no return layout.

Maybe this should be collapsed into a single `{@return ...}` block.

src/java.base/share/classes/java/lang/foreign/Linker.java line 201:

> 199:  * <p>
> 200:  * All native linker implementations operate on a subset of memory layouts. More formally, a layout {@code L}
> 201:  * is supported by a native linker {@code NL} iff:

I think using `iff` (if-and-only-if) is incorrect here, since certain linkers might impose additional constraints. For instance, the fallback linker doesn't support union layouts. Also, we want to further restrict variadic argument layouts as well as part of https://github.com/openjdk/jdk/pull/14225

Maybe we could say that all layouts passed to a linker must _at least_ adhere to the following constraints.

src/java.base/share/classes/java/lang/foreign/Linker.java line 203:

> 201:  * is supported by a native linker {@code NL} iff:
> 202:  * <ul>
> 203:  * <li>{@code L} is a value layout {@code V} and {@code V.withoutName()} is equal to one of the following layout constants:

I think the equivalence this is talking about is MemoryLayout::equals? Maybe a plain link should be added to that method as well?
Suggestion:

 * <li>{@code L} is a value layout {@code V} and {@code V.withoutName()} is {@linkplain MemoryLayout#equals(Object) equal} to one of the following layout constants:

src/java.base/share/classes/java/lang/foreign/Linker.java line 444:

> 442:  * <p>
> 443:  * When an upcall stub is passed to a foreign function, a JVM crash might occur, if the foreign code casts the function pointer
> 444:  * associated with the upcall stub to a type that is incompatible with the type of the upcall stub. Moreover, if the method

This kind of makes it sound like a crash can occur at the time of the cast. I think we should mention that the crash can occur when the function is invoked.
Suggestion:

 * When an upcall stub is passed to a foreign function, a JVM crash might occur, if the foreign code casts the function pointer
 * associated with the upcall stub to a type that is incompatible with the type of the upcall stub, and then attempts to invoke the function through the resulting function pointer. Moreover, if the method

src/java.base/share/classes/java/lang/foreign/Linker.java line 473:

> 471: 
> 472:     /**
> 473:      * Creates a method handle which is used to call a foreign function with the given signature and symbol.

I always think of a function "symbol" mostly as the _name_ of the function, so it seems that "address" would be more correct here. Though, I might be wrong on that. It's hard to find a clear definition of "symbol" that applies to this specific use case.

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

> 52:  * There are two leaves in the layout hierarchy, {@linkplain ValueLayout value layouts}, which are used to represent values of given size and kind (see
> 53:  * and {@linkplain PaddingLayout padding layouts} which are used, as the name suggests, to represent a portion of a memory
> 54:  * segment whose contents should be ignored, and which are primarily present for alignment reasons (see {@link MemoryLayout#paddingLayout(long)}).

I think this `(see {@link MemoryLayout#paddingLayout(long)})` could be removed as well now, as the link was inlined.

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

> 147:  * Some layout path elements, said <em>open path elements</em>, can select multiple layouts at once. For instance,
> 148:  * the open path elements {@link PathElement#sequenceElement()}, {@link PathElement#sequenceElement(long, long)} select
> 149:  * an unspecified element in a sequence layout. A var handles derived from a layout path containing one or more

Suggestion:

 * an unspecified element in a sequence layout. A var handle derived from a layout path containing one or more

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

> 214:  * layout path can be thought of as a function which updates the current layout {@code C_i-1} to some other layout
> 215:  * {@code C_i}. That is, for each path element {@code E1, E2, ... En}, in a layout path {@code P}, we compute
> 216:  * {@code C_i = f_i(C_i-1)}, where {@code f_i} is the selection function expressed the path element under consideration,

Suggestion:

 * {@code C_i = f_i(C_i-1)}, where {@code f_i} is the selection function expressing the path element under consideration,

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

> 217:  * denoted as {@code E_i}. The final layout {@code C_i} is also called the <em>selected layout</em>.
> 218:  * <p>
> 219:  * A layout path P is considered well-formed for an initial layout {@code C_0} if all its path elements

Using `@code`, as above:
Suggestion:

 * A layout path {@code P} is considered well-formed for an initial layout {@code C_0} if all its path elements

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

> 269:      * @apiNote This can be useful to compare two layouts that have different names, but are otherwise equal.
> 270:      *
> 271:      * @return a memory layout with the same characteristics as this layout, but with no name.

Maybe `{@return ...}` should also be used here and for the above `withName` method.

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

> 295:      *
> 296:      * @param byteAlignment the layout alignment constraint, expressed in bytes.
> 297:      * @return a memory layout with the same characteristics of this layout, but with the given alignment constraint.

Same here.

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

> 323:      *     <li>its return type is {@code long};</li>
> 324:      *     <li>it has a leading parameter of type {@code MemorySegment}, corresponding to the memory segment
> 325:      *     to be accessed;</li>

This is not correct. The returned method handle does not accept a MemorySegment.

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

> 324:      *     <li>it has a leading parameter of type {@code MemorySegment}, corresponding to the memory segment
> 325:      *     to be accessed;</li>
> 326:      *     <li>it has as many parameters of type {@code long}, one for each <a href=#open-path-elements>open path elements</a>

Grammatically, I think this should either use the singular form "open path element", or use "each *of the*".
Suggestion:

     *     <li>it has as many parameters of type {@code long}, one for each <a href=#open-path-elements>open path element</a>

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

> 362:      *     selected value layout;</li>
> 363:      *     <li>it has as many access coordinates of type {@code long}, one for each
> 364:      *     <a href=#open-path-elements>open path elements</a> in the provided layout path. The order of these access

Suggestion:

     *     <a href=#open-path-elements>open path element</a> in the provided layout path. The order of these access

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

> 391:      * <p>
> 392:      * Multiple paths can be chained, by using <a href=#deref-path-elements>dereference path elements</a>.
> 393:      * A dereference path element allows to obtain a native memory segment whose base address is the address value

"allows to obtain" doesn't sound right to me. I think it should either be "allows _subject_ to obtain" (where _subject_ is for instance "the user"), or it could also be "allows obtaining" (the the former seems more natural to me).
Suggestion:

     * A dereference path element allows the user to obtain a native memory segment whose base address is the address value

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

> 416:      *
> 417:      * @param elements the layout path elements.
> 418:      * @return a var handle that accesses a memory segment at the offset selected by the given layout path.

This doesn't seem quite right. It is not the memory segment which is found at the offset given by the layout path, it is the base address.

Maybe:
Suggestion:

     * @return a var handle that accesses a memory segment whose base address is found at the offset selected by the given layout path.

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

> 434:      *     <li>its return type is {@code MemorySegment};</li>
> 435:      *     <li>it has a leading parameter of type {@code MemorySegment}, corresponding to the memory segment
> 436:      *     to be accessed;</li>

Maybe accessed -> sliced, since no direct memory access occurs here.
Suggestion:

     *     to be sliced;</li>

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

> 468:      * @throws IllegalArgumentException if the layout path contains one or more <a href=#deref-path-elements>dereference path elements</a>.
> 469:      * @throws IllegalArgumentException if the layout path contains one or more path elements that select one or more
> 470:      * sequence element indices, such as {@link PathElement#sequenceElement(long)} and {@link PathElement#sequenceElement(long, long)}).

Looks like the first method link should like to `PathElement#sequenceElement()` instead? (I think using a bound `sequenceElement` is fine right?)

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

> 497:      *     an address layout as its target layout.</li>
> 498:      * </ul>
> 499:      * Sequence path elements selecting more than a sequence element layout are called

Suggestion:

     * Sequence path elements selecting more than one sequence element layout are called

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

> 643:      * is 1. As such, regardless of its size, in the absence of an {@linkplain #withByteAlignment(long) explicit}
> 644:      * alignment constraint, a padding layout does not affect the alignment constraint of the group or sequence layout
> 645:      * it is nested into.

It is possible to override the alignment constraints of group and sequence layouts, so maybe this should say _natural alignment_.
Suggestion:

     * alignment constraint, a padding layout does not affect the natural alignment of the group or sequence layout
     * it is nested into.

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

> 936:      * the properties of this segment. For instance, if this segment is a {@linkplain #isReadOnly() read-only segment},
> 937:      * then the resulting buffer is also {@linkplain ByteBuffer#isReadOnly() read-only}. Additionally, if this is a native
> 938:      * segment, the resulting buffer is a {@linkplain ByteBuffer#isDirect() direct buffer}.

(Pre-existing, but seemed useful to mention) Rather than giving a few examples with 'For instance', perhaps this should more explicitly list _all_ the properties that are reflected in the returned buffer (not sure if there are more).

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

> 941:      * the returned buffer's {@linkplain ByteBuffer#capacity() capacity} and {@linkplain ByteBuffer#limit() limit}
> 942:      * are both set to this segment' {@linkplain MemorySegment#byteSize() size}. For this reason, a byte buffer cannot be
> 943:      * returned if this segment' size is greater than {@link Integer#MAX_VALUE}.

Suggestion:

     * returned if this segment's size is greater than {@link Integer#MAX_VALUE}.

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

> 949:      * <p>
> 950:      * If this segment is {@linkplain #isAccessibleBy(Thread) accessible} from a single thread, calling read/write I/O
> 951:      * operations on the resulting buffer might result in an unspecified exceptions being thrown. Examples of such problematic operations are

Suggestion:

     * operations on the resulting buffer might result in unspecified exceptions being thrown. Examples of such problematic operations are

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

> 2194:      * with the alignment constraint</a> in the source element layout.
> 2195:      * @throws IllegalArgumentException if {@code srcLayout.byteAlignment() > srcLayout.byteSize()}.
> 2196:      * @throws UnsupportedOperationException if {@code srcSegment} is {@linkplain #isReadOnly() read-only}.

Isn't the source segment being read-only fine? (It seems to work when I test it).

src/java.base/share/classes/java/lang/foreign/SegmentAllocator.java line 388:

> 386:      * knows that they have fully processed the contents of the allocated segment before the subsequent allocation request
> 387:      * takes place.
> 388:      * @implNote While a prefix allocator is <em>thread-safe</em>, concurrent access on the same recycling

Is the term "thread-safe" defined any where? Should it be?

src/java.base/share/classes/java/lang/foreign/ValueLayout.java line 71:

> 69:      *
> 70:      * @param order the desired byte order.
> 71:      * @return a value layout with the same characteristics as this layout, but with the given byte order.

Also seems like a candidate for `{@return ...}`

test/jdk/java/foreign/TestLayoutPaths.java line 125:

> 123:     }
> 124: 
> 125:     public void testByteOffsetHandleRange() {

Missing `@Test`?

test/jdk/java/foreign/TestSegmentCopy.java line 92:

> 90:         if (type.layout.byteSize() > 1) {
> 91:             MemorySegment segment = MemorySegment.ofArray(new byte[100]);
> 92:             // check failure with read-only dest

Comment seems unrelated?
Suggestion:

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

PR Comment: https://git.openjdk.org/jdk/pull/14098#issuecomment-1572729100
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213428607
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213450622
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213444071
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213513178
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213519613
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213537495
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213543002
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213546737
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213547221
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213549246
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213550462
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213553113
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213554350
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213560057
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213564647
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213567889
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213568928
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213575093
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213577557
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213584838
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213599402
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213600750
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213601348
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213608551
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213613960
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213620105
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213626975
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213628954

More information about the core-libs-dev mailing list