On Mon, 29 May 2023 10:53:52 GMT, Maurizio Cimadamore <mcimadam...@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
