Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed [v7]
On Mon, 12 Jun 2023 10:33:41 GMT, Maurizio Cimadamore
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 with a new target base due
> to a merge or a rebase. The pull request now contains 31 commits:
>
> - Merge branch 'master' into javadoc_fixes
> - Fix implementation for byteOffsetHandle to accept ranged open paths.
> - Merge branch 'master' into javadoc_fixes
> - Fix issue with ArithmeticException not being wrapped in IAE for
> SequenceLayout::withElementCount
>Fix issue with IAE thrown instead of UOE for
> ValueLayout::arrayElementVarHandle
>Add apiNote to MemorySegment::address re. reachability
> - Missed a review comment
> - Use more `{@return}` tags
> - Address review comments
> - Fix wrong link in layout well-formedness doc
> - Improve javadoc on supported linker layouts
> - Tweak javadoc of MemorySegment::get/setUtf8String to deal with overflow
> - ... and 21 more: https://git.openjdk.org/jdk/compare/268ec61d...78f7bd24
Re-opening - as per new policies, changes will NOT be automatically forward
ported (see [1]), so we need a separate PR for 22.
[1] - https://mail.openjdk.org/pipermail/jdk-dev/2023-June/007894.html
-
PR Comment: https://git.openjdk.org/jdk/pull/14098#issuecomment-1587608037
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed
On Mon, 12 Jun 2023 14:45:48 GMT, Jorn Vernee wrote: > Btw, besides the other 2 issues this solves (from the other PR), I think this > also solves: https://bugs.openjdk.org/browse/JDK-8255350 Thanks for the reminder - JDK-8255350 is not fixed by this, but by https://git.openjdk.org/jdk/pull/14007 - PR Comment: https://git.openjdk.org/jdk21/pull/7#issuecomment-1587557299
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed
On Mon, 12 Jun 2023 15:19:51 GMT, Maurizio Cimadamore wrote: >> Btw, besides the other 2 issues this solves (from the other PR), I think >> this also solves: https://bugs.openjdk.org/browse/JDK-8255350 > >> Btw, besides the other 2 issues this solves (from the other PR), I think >> this also solves: https://bugs.openjdk.org/browse/JDK-8255350 > > Thanks for the reminder - JDK-8255350 is not fixed by this, but by > https://git.openjdk.org/jdk/pull/14007 @mcimadamore Shouldn't this go into the mainline first? - PR Comment: https://git.openjdk.org/jdk21/pull/7#issuecomment-1587561904
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed [v6]
On Mon, 12 Jun 2023 10:29:07 GMT, Maurizio Cimadamore
wrote:
>> Maurizio Cimadamore has updated the pull request with a new target base due
>> to a merge or a rebase. The pull request now contains 29 commits:
>>
>> - Merge branch 'master' into javadoc_fixes
>> - Fix issue with ArithmeticException not being wrapped in IAE for
>> SequenceLayout::withElementCount
>>Fix issue with IAE thrown instead of UOE for
>> ValueLayout::arrayElementVarHandle
>>Add apiNote to MemorySegment::address re. reachability
>> - Missed a review comment
>> - Use more `{@return}` tags
>> - Address review comments
>> - Fix wrong link in layout well-formedness doc
>> - Improve javadoc on supported linker layouts
>> - Tweak javadoc of MemorySegment::get/setUtf8String to deal with overflow
>> - Merge branch 'master' into javadoc_fixes
>> - Add overflow tests
>> - ... and 19 more: https://git.openjdk.org/jdk/compare/01455a07...b59edd93
>
> Closing, as the review is complete and I have to rebase against the forked 21
> repo. A similar PR will pop up soon :-)
> @mcimadamore Is there a reason that this shouldn't be integrated into the
> mainline?? Absent a compelling reason, fixes should be integrated first into
> the mainline (for 22) and then backported to 21.
This fix was targeting 21 - but then the repo has forked. It is my
understanding that fixes applied to the 21 fork will be forward-ported to 22
automatically.
-
PR Comment: https://git.openjdk.org/jdk/pull/14098#issuecomment-1587564381
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed [v6]
On Mon, 12 Jun 2023 10:29:07 GMT, Maurizio Cimadamore
wrote:
>> Maurizio Cimadamore has updated the pull request with a new target base due
>> to a merge or a rebase. The pull request now contains 29 commits:
>>
>> - Merge branch 'master' into javadoc_fixes
>> - Fix issue with ArithmeticException not being wrapped in IAE for
>> SequenceLayout::withElementCount
>>Fix issue with IAE thrown instead of UOE for
>> ValueLayout::arrayElementVarHandle
>>Add apiNote to MemorySegment::address re. reachability
>> - Missed a review comment
>> - Use more `{@return}` tags
>> - Address review comments
>> - Fix wrong link in layout well-formedness doc
>> - Improve javadoc on supported linker layouts
>> - Tweak javadoc of MemorySegment::get/setUtf8String to deal with overflow
>> - Merge branch 'master' into javadoc_fixes
>> - Add overflow tests
>> - ... and 19 more: https://git.openjdk.org/jdk/compare/01455a07...b59edd93
>
> Closing, as the review is complete and I have to rebase against the forked 21
> repo. A similar PR will pop up soon :-)
@mcimadamore Is there a reason that this shouldn't be integrated into the
mainline?? Absent a compelling reason, fixes should be integrated first into
the mainline (for 22) and then backported to 21.
-
PR Comment: https://git.openjdk.org/jdk/pull/14098#issuecomment-1587513641
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed
On Mon, 12 Jun 2023 11:01:08 GMT, Maurizio Cimadamore wrote: > This is the same PR as https://github.com/openjdk/jdk/pull/14098, but > backported to the JDK 21 repo fork. Btw, besides the other 2 issues this solves (from the other PR), I think this also solves: https://bugs.openjdk.org/browse/JDK-8255350 - PR Comment: https://git.openjdk.org/jdk21/pull/7#issuecomment-1587495022
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed
On Mon, 12 Jun 2023 11:01:08 GMT, Maurizio Cimadamore wrote: > This is the same PR as https://github.com/openjdk/jdk/pull/14098, but > backported to the JDK 21 repo fork. Already reviewed the other PR. - Marked as reviewed by jvernee (Reviewer). PR Review: https://git.openjdk.org/jdk21/pull/7#pullrequestreview-1474902428
RFR: 8308645: Javadoc of FFM API needs to be refreshed
This is the same PR as https://github.com/openjdk/jdk/pull/14098, but backported to the JDK 21 repo fork. - Commit messages: - Initial push Changes: https://git.openjdk.org/jdk21/pull/7/files Webrev: https://webrevs.openjdk.org/?repo=jdk21&pr=7&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8308645 Stats: 964 lines in 28 files changed: 357 ins; 138 del; 469 mod Patch: https://git.openjdk.org/jdk21/pull/7.diff Fetch: git fetch https://git.openjdk.org/jdk21.git pull/7/head:pull/7 PR: https://git.openjdk.org/jdk21/pull/7
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed [v7]
> 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 throw because of an overflow during ...
Maurizio Cimadamore has updated the pull request with a new target base due to
a merge or a rebase. The pull request now contains 31 commits:
- Merge branch 'master' into javadoc_fixes
- Fix implementation for byteOffsetHandle to accept ranged open paths.
- Merge branch 'master' into javadoc_fixes
- Fix issue with ArithmeticException not being wrapped in IAE for
SequenceLayout::withElementCount
Fix issue with IAE thrown instead of UOE for
ValueLayout::arrayElementVarHandle
Add apiNote to MemorySegment::address re. reachability
- Missed a review comment
- Use more `{@return}` tags
- Address review comments
- Fix wrong link in layout well-formedness doc
- Improve javadoc on supported linker layouts
- Tweak javadoc of MemorySegment::get/setUtf8String to deal with overflow
- ... and 21 more: https://git.openjdk.org/jdk/compare/268ec61d...78f7bd24
-
Changes: https://git.openjdk.org/jdk/pull/14098/files
Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=14098&range=06
Stats: 964 lines in 28 files changed: 357 ins; 138 del; 469 mod
Patch: https://git.openjdk.org/jdk/pull/14098.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/14098/head:pull/14098
PR: https://git.openjdk.org/jdk/pull/14098
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed [v6]
On Tue, 6 Jun 2023 10:35:09 GMT, Maurizio Cimadamore
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 with a new target base due
> to a merge or a rebase. The pull request now contains 29 commits:
>
> - Merge branch 'master' into javadoc_fixes
> - Fix issue with ArithmeticException not being wrapped in IAE for
> SequenceLayout::withElementCount
>Fix issue with IAE thrown instead of UOE for
> ValueLayout::arrayElementVarHandle
>Add apiNote to MemorySegment::address re. reachability
> - Missed a review comment
> - Use more `{@return}` tags
> - Address review comments
> - Fix wrong link in layout well-formedness doc
> - Improve javadoc on supported linker layouts
> - Tweak javadoc of MemorySegment::get/setUtf8String to deal with overflow
> - Merge branch 'master' into javadoc_fixes
> - Add overflow tests
> - ... and 19 more: https://git.openjdk.org/jdk/compare/01455a07...b59edd93
Closing, as the review is complete and I have to rebase against the forked 21
repo. A similar PR will pop up soon :-)
-
PR Comment: https://git.openjdk.org/jdk/pull/14098#issuecomment-1587049739
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed [v4]
On Fri, 9 Jun 2023 18:22:48 GMT, Jorn Vernee wrote: >> But, the implementation of `byteOffsetHandle` rejects range elements: >> https://github.com/openjdk/jdk/pull/14098/files/265e0d5b082ad54d1cadea1d4a8bd844e72e3926#diff-90ecdacbee63a90f6e695a8bb4c1ad69b591602501613a774561c4032f01fb95R352 >> >> So this test fails (at least it does here...) > > Though, I agree that it seems fine to support ranges I see, the issue is with the impl (I agree) not with the javadoc as I understood. - PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1226404584
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed [v4]
On Fri, 9 Jun 2023 18:20:51 GMT, Jorn Vernee wrote: >> Note that the javadoc has been rectified here (as part of the PR) as I think >> it has always been wrong in the past (probably cut and paste issue from the >> non-MH version?) > > But, the implementation of `byteOffsetHandle` rejects range elements: > https://github.com/openjdk/jdk/pull/14098/files/265e0d5b082ad54d1cadea1d4a8bd844e72e3926#diff-90ecdacbee63a90f6e695a8bb4c1ad69b591602501613a774561c4032f01fb95R352 > > So this test fails (at least it does here...) Though, I agree that it seems fine to support ranges - PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1224618487
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed [v4]
On Fri, 9 Jun 2023 13:42:17 GMT, Maurizio Cimadamore wrote: >> Note sure - javadoc says: >> >> * @throws IllegalArgumentException if the layout path is not > href="#well-formedness">well-formed for this layout. >> * @throws IllegalArgumentException if the layout path contains one or >> more dereference path elements. >> >> >> Which seems correct. The handle version can accept indices, so that's ok? > > Note that the javadoc has been rectified here (as part of the PR) as I think > it has always been wrong in the past (probably cut and paste issue from the > non-MH version?) But, the implementation of `byteOffsetHandle` rejects range elements: https://github.com/openjdk/jdk/pull/14098/files/265e0d5b082ad54d1cadea1d4a8bd844e72e3926#diff-90ecdacbee63a90f6e695a8bb4c1ad69b591602501613a774561c4032f01fb95R352 So this test fails (at least it does here...) - PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1224616797
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed [v4]
On Fri, 9 Jun 2023 13:40:47 GMT, Maurizio Cimadamore wrote: >> FWIW, it looks like this needs `@Test(expectedExceptions = >> IllegalArgumentException.class)` since range elements are not allowed for >> `byteOffsetHandle` > > Note sure - javadoc says: > > * @throws IllegalArgumentException if the layout path is not href="#well-formedness">well-formed for this layout. > * @throws IllegalArgumentException if the layout path contains one or > more dereference path elements. > > > Which seems correct. The handle version can accept indices, so that's ok? Note that the javadoc has been rectified here (as part of the PR) as I think it has always been wrong in the past (probably cut and paste issue from the non-MH version?) - PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1224329494
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed [v4]
On Thu, 8 Jun 2023 20:21:48 GMT, Jorn Vernee wrote:
>> test/jdk/java/foreign/TestLayoutPaths.java line 125:
>>
>>> 123: }
>>> 124:
>>> 125: public void testByteOffsetHandleRange() {
>>
>> Missing `@Test`?
>
> FWIW, it looks like this needs `@Test(expectedExceptions =
> IllegalArgumentException.class)` since range elements are not allowed for
> `byteOffsetHandle`
Note sure - javadoc says:
* @throws IllegalArgumentException if the layout path is not well-formed for this layout.
* @throws IllegalArgumentException if the layout path contains one or more
dereference path elements.
Which seems correct. The handle version can accept indices, so that's ok?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1224327629
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed [v4]
On Thu, 1 Jun 2023 20:06:32 GMT, Jorn Vernee wrote:
>> Maurizio Cimadamore has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Fix wrong link in layout well-formedness doc
>
> test/jdk/java/foreign/TestLayoutPaths.java line 125:
>
>> 123: }
>> 124:
>> 125: public void testByteOffsetHandleRange() {
>
> Missing `@Test`?
FWIW, it looks like this needs `@Test(expectedExceptions =
IllegalArgumentException.class)` since range elements are not allowed for
`byteOffsetHandle`
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1223511154
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed [v6]
> 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 throw because of an ...
Maurizio Cimadamore has updated the pull request with a new target base due to
a merge or a rebase. The pull request now contains 29 commits:
- Merge branch 'master' into javadoc_fixes
- Fix issue with ArithmeticException not being wrapped in IAE for
SequenceLayout::withElementCount
Fix issue with IAE thrown instead of UOE for
ValueLayout::arrayElementVarHandle
Add apiNote to MemorySegment::address re. reachability
- Missed a review comment
- Use more `{@return}` tags
- Address review comments
- Fix wrong link in layout well-formedness doc
- Improve javadoc on supported linker layouts
- Tweak javadoc of MemorySegment::get/setUtf8String to deal with overflow
- Merge branch 'master' into javadoc_fixes
- Add overflow tests
- ... and 19 more: https://git.openjdk.org/jdk/compare/01455a07...b59edd93
-
Changes: https://git.openjdk.org/jdk/pull/14098/files
Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=14098&range=05
Stats: 963 lines in 28 files changed: 357 ins; 138 del; 468 mod
Patch: https://git.openjdk.org/jdk/pull/14098.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/14098/head:pull/14098
PR: https://git.openjdk.org/jdk/pull/14098
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed [v5]
On Fri, 2 Jun 2023 11:41:38 GMT, Maurizio Cimadamore
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 three
> additional commits since the last revision:
>
> - Missed a review comment
> - Use more `{@return}` tags
> - Address review comments
Marked as reviewed by jvernee (Reviewer).
src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 945:
> 943: * cannot be returned if this segment's size is greater than {@link
> Integer#MAX_VALUE};
> 944: * It is a {@linkplain ByteBuffer#isDirect() direct buffer}, if
> this is a native segment.
> 945: *
Nice!
-
PR Review: https://git.openjdk.org/jdk/pull/14098#pullrequestreview-1457977686
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1214587551
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed [v4]
On Fri, 2 Jun 2023 11:07:42 GMT, Maurizio Cimadamore wrote: >> Yeah - I meant what you said - but now that you said it, I also saw how what >> I've written can be prone to an alternate (and wrong) interpretation. I'll >> clarify. > > Actually, looking back at this I'm not sure it's correct? We have an input > memory segment MS. And a layout path. A layout path identifies an offset O at > which MS has to be accessed in order to read a value (of some type). I'm not > sure any of this depends on "base address" ? Hmm, that's true. The base address that we read and turn into a memory segment isn't necessarily accessed, but the MS we read the base address from is. So, on second thought, I think the current text is good as it is. - PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1214272188
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed [v4]
On Mon, 29 May 2023 10:53:52 GMT, Maurizio Cimadamore 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 Thanks for the comments so far, I've updated the javadoc in place: https://cr.openjdk.org/~mcimadamore/jdk/8308645/javadoc/java.base/java/lang/foreign/package-summary.html - PR Comment: https://git.openjdk.org/jdk/pull/14098#issuecomment-1573599201
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed [v5]
> 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 throw because of an ...
Maurizio Cimadamore has updated the pull request incrementally with three
additional commits since the last revision:
- Missed a review comment
- Use more `{@return}` tags
- Address review comments
-
Changes:
- all: https://git.openjdk.org/jdk/pull/14098/files
- new: https://git.openjdk.org/jdk/pull/14098/files/265e0d5b..44b382dd
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=14098&range=04
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=14098&range=03-04
Stats: 79 lines in 12 files changed: 12 ins; 15 del; 52 mod
Patch: https://git.openjdk.org/jdk/pull/14098.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/14098/head:pull/14098
PR: https://git.openjdk.org/jdk/pull/14098
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed [v4]
On Thu, 1 Jun 2023 21:02:13 GMT, Maurizio Cimadamore wrote: >> 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. > > Yeah - I meant what you said - but now that you said it, I also saw how what > I've written can be prone to an alternate (and wrong) interpretation. I'll > clarify. Actually, looking back at this I'm not sure it's correct? We have an input memory segment MS. And a layout path. A layout path identifies an offset O at which MS has to be accessed in order to read a value (of some type). I'm not sure any of this depends on "base address" ? - PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1214235804
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed [v4]
On Thu, 1 Jun 2023 19:58:49 GMT, Jorn Vernee wrote:
>> Maurizio Cimadamore has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Fix wrong link in layout well-formedness doc
>
> 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 ...}`
Possibly. The text is consistent in using different wording between the header
and return (as I did a pass in that sense), but perhaps that's a bug.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1214217959
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed [v4]
On Thu, 1 Jun 2023 21:09:12 GMT, Maurizio Cimadamore
wrote:
>> src/java.base/share/classes/java/lang/foreign/Linker.java line 201:
>>
>>> 199: *
>>> 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.
>
> I'll think about it - it's a bit problematic to specify in terms of "at
> least" because we need to be able to refer to "supported by NL" recursively
> in the text.
I'll replace "if" with "iff". I think to address your comment on linker options
affecting what's supported, we can add a paragraph in that sense (but I think
I'd like to do that after we fix https://git.openjdk.org/jdk/pull/14225, so
that I can add a link to somewhere concrete.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1214215055
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed [v4]
On Thu, 1 Jun 2023 13:04:22 GMT, Alan Bateman wrote:
>> Maurizio Cimadamore has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Fix wrong link in layout well-formedness doc
>
> src/java.base/share/classes/java/lang/foreign/FunctionDescriptor.java line 57:
>
>> 55:
>> 56: /**
>> 57: * {@return the argument layouts of this function descriptor (as an
>> immutable list)}.
>
> I assume this should be "unmodifiable" rather than immutable here.
This was derived from MethodType::parameterList, which also mentions immutable.
I'm fine changing to "unmodifiable".
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1214190239
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed [v4]
On Fri, 2 Jun 2023 09:58:54 GMT, Maurizio Cimadamore wrote: >>> I think SegmentAllocator should be agnostic re. thread safety. Allocation >>> is a world of compromises, where if you give up (thread) safety you can >>> gain more performance (and viceversa). So I think having a "one size fits >>> all" thread-safety blanket might not work very well. >> >> I'm just trying to think about the practical implications of this. Arena is >> specified to be thread-safe so I can allocate from a non-confined Arena, and >> from concurrent threads, without needing to coordinate. However, if you >> hand me a SegmentAllocator that is not an Arena then I don't know if I need >> to coordinate allocation with other parties. I'm not too concerned about the >> slicingAllocator and prefixAllocator factory methods as the result >> SegmentAllocators will likely be wrapped. > > In practice, SegmentAllocator will be used in two cases: > * as a parameter to a downcall method handle which returns a by-value struct > * as a building block to construct a custom arena > > In both cases, I think it's up to the client to decide how thread-safe > allocation should be. For instance, if you use a SegmentAllocator in a custom > arena, and the allocator is not thread-safe, well, you can always attach it > to a custom _confined_ arena (so that thread-unsafety is not a problem). I > think using a SegmentAllocator in isolation will be quite rare. Note that, beside thread-safety, there are other things that are inconvenient about a segment allocator: * an allocator is not constrained to return segments with the same lifetime; * an allocator does not provide any guarantee re. aliasing memory. Same memory region could be written over and over (this is what prefixAllocator does); These facts make it hard for clients SegmentAllocator to interact with a SegmentAllocator they don't own (unless they have to allocate precisely _one_ segment, on behalf of the client, and return it to them - which, not surprisingly is also what the Linker does). So, with SegmentAllocators there's always an element of trust: a client gave me an allocator to obtain a slab of memory, I trust the client to have done what it needed to do so that my operation can be safe. If I happen to allocate garbage (as a result of a race) it's on them. If you need to allocate multiple segments in a single "session", and have some lifetime guarantees of the segments that have been allocated and/or you want to make sure that what you get is not garbage (e.g. thread-safety, aliasing, etc.), then you need Arena. - PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1214183868
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed [v4]
On Fri, 2 Jun 2023 08:38:36 GMT, Alan Bateman wrote: >> I think SegmentAllocator should be agnostic re. thread safety. Allocation is >> a world of compromises, where if you give up (thread) safety you can gain >> more performance (and viceversa). So I think having a "one size fits all" >> thread-safety blanket might not work very well. > >> I think SegmentAllocator should be agnostic re. thread safety. Allocation is >> a world of compromises, where if you give up (thread) safety you can gain >> more performance (and viceversa). So I think having a "one size fits all" >> thread-safety blanket might not work very well. > > I'm just trying to think about the practical implications of this. Arena is > specified to be thread-safe so I can allocate from a non-confined Arena, and > from concurrent threads, without needing to coordinate. However, if you hand > me a SegmentAllocator that is not an Arena then I don't know if I need to > coordinate allocation with other parties. I'm not too concerned about the > slicingAllocator and prefixAllocator factory methods as the result > SegmentAllocators will likely be wrapped. In practice, SegmentAllocator will be used in two cases: * as a parameter to a downcall method handle which returns a by-value struct * as a building block to construct a custom arena In both cases, I think it's up to the client to decide how thread-safe allocation should be. For instance, if you use a SegmentAllocator in a custom arena, and the allocator is not thread-safe, well, you can always attach it to a custom _confined_ arena (so that thread-unsafety is not a problem). I think using a SegmentAllocator in isolation will be quite rare. - PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1214174042
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed [v4]
On Thu, 1 Jun 2023 21:12:08 GMT, Maurizio Cimadamore wrote: > I think SegmentAllocator should be agnostic re. thread safety. Allocation is > a world of compromises, where if you give up (thread) safety you can gain > more performance (and viceversa). So I think having a "one size fits all" > thread-safety blanket might not work very well. I'm just trying to think about the practical implications of this. Arena is specified to be thread-safe so I can allocate from a non-confined Arena, and from concurrent threads, without needing to coordinate. However, if you hand me a SegmentAllocator that is not an Arena then I don't know if I need to coordinate allocation with other parties. I'm not too concerned about the slicingAllocator and prefixAllocator factory methods as the result SegmentAllocators will likely be wrapped. - PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1214092803
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed [v4]
On Fri, 2 Jun 2023 08:08:17 GMT, Per Minborg wrote: >> Right, I guess I'm asking: can/should we do better? ;) It might be nice to >> define the term once and for all for the entire JDK, similar to how >> 'reachability' also has a central definition. (and then add a link from >> here). >> >> But, arguably, that seems like too much scope creep for this PR, so let's >> save it for a potential followup. > > I think there is something we could start off from: > https://sdk.amazonaws.com/java/api/latest/software/amazon/awssdk/annotations/ThreadSafe.html > > This could be made under a separate issue. https://bugs.openjdk.org/browse/JDK-8309342 - PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1214075735
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed [v4]
On Thu, 1 Jun 2023 21:47:23 GMT, Jorn Vernee wrote:
>> I think the term is used pretty much all over the javadoc (not just FFM's) -
>> e.g. look for this preamble:
>>
>> * > href="{@docRoot}/java.base/java/lang/doc-files/ValueBased.html">value-based,
>> * immutable and thread-safe
>> ```
>
> Right, I guess I'm asking: can/should we do better? ;) It might be nice to
> define the term once and for all for the entire JDK, similar to how
> 'reachability' also has a central definition. (and then add a link from here).
>
> But, arguably, that seems like too much scope creep for this PR, so let's
> save it for a potential followup.
I think there is something we could start off from:
https://sdk.amazonaws.com/java/api/latest/software/amazon/awssdk/annotations/ThreadSafe.html
This could be made under a separate issue.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1214060977
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed [v4]
On Thu, 1 Jun 2023 20:57:25 GMT, Maurizio Cimadamore wrote: >> On a related note: should we even allow sequences of padding layouts? > > I let sequence of padding slide on the basis that groups of padding are > allowed. But it's a somewhat arbitrary line. That said, we do use stuff like > that in jextract, where we model types we don't support (or bitfields) as > padding. I think starting to ban padding in certain places would result in a > less compositional API - and perhaps tools would have to workaround some of > these limitations. Thanks, I agree with that analogy - PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213714285
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed [v4]
On Thu, 1 Jun 2023 21:00:33 GMT, Maurizio Cimadamore
wrote:
>> src/java.base/share/classes/java/lang/foreign/MemoryLayout.java line 470:
>>
>>> 468: * @throws IllegalArgumentException if the layout path contains
>>> one or more dereference path elements.
>>> 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?)
>
> This is deliberate (but subtle). Basically, for `select` we just want to
> select a target layout (we don't care which). So the method has a preference
> for open sequence layout paths: there's no access or offset to model here,
> the method just needs to end up into some layout at the end of the path. One
> might argue that these restrictions are unnecessary - but I didn't feel
> strongly enough to drop them.
Ok, thanks for explaining.
>> 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 thread-safe,
>>> concurrent access on the same recycling
>>
>> Is the term "thread-safe" defined any where? Should it be?
>
> I think the term is used pretty much all over the javadoc (not just FFM's) -
> e.g. look for this preamble:
>
> * href="{@docRoot}/java.base/java/lang/doc-files/ValueBased.html">value-based,
> * immutable and thread-safe
> ```
Right, I guess I'm asking: can/should we do better? ;) It might be nice to
define the term once and for all for the entire JDK, similar to how
'reachability' also has a central definition. (and then add a link from here).
But, arguably, that seems like too much scope creep for this PR, so let's save
it for a potential followup.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213714496
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213716230
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed [v4]
On Thu, 1 Jun 2023 21:04:42 GMT, Maurizio Cimadamore wrote: >> the idea behind this is to connect with the javadoc of `SymbolLookup` which >> defines and then uses symbol all over the place. > > Maybe a linkplan could help? Okay, but `SymbolLookup` also says this: "A symbol lookup retrieves the address of a symbol in one or more libraries." and "This symbol lookup, which is known as a default lookup, helps clients to quickly find addresses of well-known symbols." This seems to imply that "symbol" and "address" are two different things. I get the connection between the address and the SymbolLookup, but I'll note that it's also not necessary for a function address to come from a SymbolLookup (e.g. the address could be returned from native code, or be the address of an upcall stub). So, I feel like sticking with "address" is better. Maybe a paragraph (or just a `@see` tag) could be added to refer to SymbolLookup instead? - PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213693435
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed [v4]
On Thu, 1 Jun 2023 13:00:58 GMT, Alan Bateman wrote:
>> Maurizio Cimadamore has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Fix wrong link in layout well-formedness doc
>
> src/java.base/share/classes/java/lang/foreign/Arena.java line 269:
>
>> 267: * @throws IllegalStateException if this arena has already been
>> {@linkplain #close() closed}.
>> 268: * @throws WrongThreadException if this arena is confined, and this
>> method is called from a thread {@code T}
>> 269: * other than the arena's owner thread.
>
> "thread T" hints that "T" will be used later, maybe it's not needed.
Correct - this is mostly a copy/paste issue for similar throws clauses in
methods outside arena.
> src/java.base/share/classes/java/lang/foreign/SegmentAllocator.java line 363:
>
>> 361: * The returned allocator throws {@link IndexOutOfBoundsException}
>> when a slice of the provided
>> 362: * segment with the requested size and alignment cannot be found.
>> 363: * @implNote A slicing allocator is not thread-safe.
>
> The implNote about thread safety makes me wonder if the SegmentAllocator
> should say something about thread safety, e.g. should it specify that the
> allocate methods are thread safe?
I think SegmentAllocator should be agnostic re. thread safety. Allocation is a
world of compromises, where if you give up (thread) safety you can gain more
performance (and viceversa). So I think having a "one size fits all"
thread-safety blanket might not work very well.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213686844
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213685702
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed [v4]
On Thu, 1 Jun 2023 17:10:48 GMT, Jorn Vernee wrote:
>> Maurizio Cimadamore has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Fix wrong link in layout well-formedness doc
>
> src/java.base/share/classes/java/lang/foreign/Linker.java line 201:
>
>> 199: *
>> 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.
I'll think about it - it's a bit problematic to specify in terms of "at least"
because we need to be able to refer to "supported by NL" recursively in the
text.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213683303
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed [v4]
On Thu, 1 Jun 2023 21:04:27 GMT, Maurizio Cimadamore wrote: >> 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. > > the idea behind this is to connect with the javadoc of `SymbolLookup` which > defines and then uses symbol all over the place. Maybe a linkplan could help? - PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213679476
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed [v4]
On Thu, 1 Jun 2023 18:12:28 GMT, Jorn Vernee wrote:
>> Maurizio Cimadamore has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Fix wrong link in layout well-formedness doc
>
> src/java.base/share/classes/java/lang/foreign/Linker.java line 444:
>
>> 442: *
>> 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
I agree that's better - I was also puzzled by the text (I think it's there from
before, just shuffled?)
> 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.
the idea behind this is to connect with the javadoc of `SymbolLookup` which
defines and then uses symbol all over the place.
> src/java.base/share/classes/java/lang/foreign/MemoryLayout.java line 393:
>
>> 391: *
>> 392: * Multiple paths can be chained, by using > href=#deref-path-elements>dereference path elements.
>> 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
I will also look for some other way to say this - typically when I end up with
such convoluted sentences there's a better way to write it :-)
> 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.
Yeah - I meant what you said - but now that you said it, I also saw how what
I've written can be prone to an alternate (and wrong) interpretation. I'll
clarify.
> src/java.base/share/classes/java/lang/foreign/MemoryLayout.java line 470:
>
>> 468: * @throws IllegalArgumentException if the layout path contains one
>> or more dereference path elements.
>> 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?)
This is deliberate (but subtle). Basically, for `select` we just want to select
a target layout (we don't care which). So the method has a preference for open
sequence layout paths: there's no access or offset to model here, the method
just needs to end up into some layout at the end of the path. One might argue
that these restrictions are unnecessary - but I didn't feel strongly enough to
drop them.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213681006
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213679251
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213677679
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213676964
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213675483
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed [v4]
On Thu, 1 Jun 2023 19:25:38 GMT, Jorn Vernee wrote:
>> 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.
>
> On a related note: should we even allow sequences of padding layouts?
I let sequence of padding slide on the basis that groups of padding are
allowed. But it's a somewhat arbitrary line. That said, we do use stuff like
that in jextract, where we model types we don't support (or bitfields) as
padding. I think starting to ban padding in certain places would result in a
less compositional API - and perhaps tools would have to workaround some of
these limitations.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213672976
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed [v4]
On Thu, 1 Jun 2023 19:36:48 GMT, Jorn Vernee wrote:
>> Maurizio Cimadamore has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Fix wrong link in layout well-formedness doc
>
> 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).
Good idea - it is not trivial to capture all the properties in a single para.
> 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 thread-safe,
>> concurrent access on the same recycling
>
> Is the term "thread-safe" defined any where? Should it be?
I think the term is used pretty much all over the javadoc (not just FFM's) -
e.g. look for this preamble:
* value-based,
* immutable and thread-safe
```
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213670809
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213669418
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed [v4]
On Thu, 1 Jun 2023 13:16:44 GMT, Alan Bateman wrote:
>> Maurizio Cimadamore has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Fix wrong link in layout well-formedness doc
>
> src/java.base/share/classes/java/lang/foreign/SequenceLayout.java line 75:
>
>> 73: * @return a sequence layout with the same characteristics of this
>> layout, but with the given element count.
>> 74: * @throws IllegalArgumentException if {@code elementCount} is
>> negative.
>> 75: * @throws IllegalArgumentException if {@code
>> elementLayout.bitSize() * elementCount} overflows.
>
> Shouldn't the exception descriptions be combined to avoid IAE being listed
> twice in the generated javadoc?
I think @PaulSandoz suggested this idiom in other places where the `@throws`
clause was very hard to break up, so I thought that would have been preferrable
than having a single para. But if there are strong opinions, I could tweak in
different ways, perhaps with bullet lists (but that will require a lot of work).
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213660125
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed [v4]
On Thu, 1 Jun 2023 19:24:35 GMT, Jorn Vernee wrote:
>> Maurizio Cimadamore has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Fix wrong link in layout well-formedness doc
>
> 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.
On a related note: should we even allow sequences of padding layouts?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213585708
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed [v4]
On Thu, 1 Jun 2023 13:06:15 GMT, Alan Bateman wrote:
>> Maurizio Cimadamore has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Fix wrong link in layout well-formedness doc
>
> src/java.base/share/classes/java/lang/foreign/FunctionDescriptor.java line
> 127:
>
>> 125: /**
>> 126: * Creates a function descriptor with the given argument layouts
>> and no return layout. This is useful to model functions
>> 127: * which return no values.
>
> I think I would use "that return" rather than "which return" here.
I [looked this up](https://www.grammarly.com/blog/which-vs-that/), and learned
something (thanks!).
"which" is used for optional clauses. It implies the sentence could be written
as "This is useful to model functions (which, by the way, return no values)".
i.e. it is optional information about functions. "that" is used to indicate
we're talking about a particular kind of function (implying also that there are
multiple kinds).
> src/java.base/share/classes/java/lang/foreign/SegmentAllocator.java line 325:
>
>> 323: * @return a segment for the newly allocated memory block.
>> 324: * @throws IllegalArgumentException if {@code
>> elementLayout.byteSize() * count} overflows.
>> 325: * @throws IllegalArgumentException if {@code count < 0}.
>
> Another case where the IAE descriptions should probably be combined.
I think having the different failure cases split up like this makes the doc
easier to read. (IIRC we used to have everything combined in the past, but got
the advice at some point to split things up into separate `@throws` clauses).
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213439901
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213611242
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed [v4]
On Mon, 29 May 2023 10:53:52 GMT, Maurizio Cimadamore
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: *
> 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: *
> 203: * {@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:
* {@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: *
> 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 upc
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed [v4]
On Mon, 29 May 2023 10:53:52 GMT, Maurizio Cimadamore
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
src/java.base/share/classes/java/lang/foreign/AddressLayout.java line 116:
> 114: /**
> 115: * Returns an address layout with the same carrier, alignment
> constraint, name and order as this address layout,
> 116: * but with no target layout
Did you mean to drop the period from the end of the sentence?
src/java.base/share/classes/java/lang/foreign/Arena.java line 269:
> 267: * @throws IllegalStateException if this arena has already been
> {@linkplain #close() closed}.
> 268: * @throws WrongThreadException if this arena is confined, and this
> method is called from a thread {@code T}
> 269: * other than the arena's owner thread.
"thread T" hints that "T" will be used later, maybe it's not needed.
src/java.base/share/classes/java/lang/foreign/FunctionDescriptor.java line 38:
> 36: /**
> 37: * A function descriptor models the signature of a foreign function. A
> function descriptor is made up of zero or more
> 38: * argument layouts and zero or one return layout. A function descriptor
> is used to create
You might want want to put a comma after "layouts" to avoid any ambiguity.
src/java.base/share/classes/java/lang/foreign/FunctionDescriptor.java line 57:
> 55:
> 56: /**
> 57: * {@return the argument layouts of this function descriptor (as an
> immutable list)}.
I assume this should be "unmodifiable" rather than immutable here.
src/java.base/share/classes/java/lang/foreign/FunctionDescriptor.java line 127:
> 125: /**
> 126: * Creates a function descriptor with the given argument layouts and
> no return layout. This is useful to model functions
> 127: * which return no values.
I think I would use "that return" rather than "which return" here.
src/java.base/share/classes/java/lang/foreign/Linker.java line 356:
> 354: * attach the segment to some existing {@linkplain Arena arena}, so that
> the lifetime of the region of memory
> 355: * backing the segment can be managed automatically, as for any other
> native segment created directly from Java code.
> 356: * Both these operations are accomplished using the restri
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed [v4]
> 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 throw because of an ... Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Fix wrong link in layout well-formedness doc - Changes: - all: https://git.openjdk.org/jdk/pull/14098/files - new: https://git.openjdk.org/jdk/pull/14098/files/6703eee9..265e0d5b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14098&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14098&range=02-03 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/14098.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14098/head:pull/14098 PR: https://git.openjdk.org/jdk/pull/14098
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed [v3]
On Fri, 26 May 2023 18:11:14 GMT, Maurizio Cimadamore
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:
>
> Improve javadoc on supported linker layouts
src/java.base/share/classes/java/lang/foreign/MemoryLayout.java line 187:
> 185: * ).withName("point")
> 186: * )
> 187: * ).withName("points")
Wrong indentation:
Suggestion:
* )
* ).withName("points")
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1207238878
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed [v2]
On Thu, 25 May 2023 15:31:43 GMT, Maurizio Cimadamore 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: > > Tweak javadoc of MemorySegment::get/setUtf8String to deal with overflow I brought over some javadoc improvements from https://github.com/openjdk/jdk/pull/14037 (which has been withdrawn). These improvements should allow us to enable support for Linker in x86 platforms, as they define the notion of "what is a linker supported layout" much more precisely, and in a way that is not dependent on natural alignment (for value layouts). - PR Comment: https://git.openjdk.org/jdk/pull/14098#issuecomment-1564747719
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed [v3]
> 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 throw because of an ... Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Improve javadoc on supported linker layouts - Changes: - all: https://git.openjdk.org/jdk/pull/14098/files - new: https://git.openjdk.org/jdk/pull/14098/files/df6c1239..6703eee9 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14098&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14098&range=01-02 Stats: 69 lines in 3 files changed: 45 ins; 3 del; 21 mod Patch: https://git.openjdk.org/jdk/pull/14098.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14098/head:pull/14098 PR: https://git.openjdk.org/jdk/pull/14098
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed [v2]
> 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 throw because of an ... Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Tweak javadoc of MemorySegment::get/setUtf8String to deal with overflow - Changes: - all: https://git.openjdk.org/jdk/pull/14098/files - new: https://git.openjdk.org/jdk/pull/14098/files/afb7360e..df6c1239 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14098&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14098&range=00-01 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/14098.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14098/head:pull/14098 PR: https://git.openjdk.org/jdk/pull/14098
RFR: 8308645: Javadoc of FFM API needs to be refreshed
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 throw because of an overflow during offset computation. Finally, I've left most of the snippets alone, as they are being dealt with in https://github.com/openjdk/jdk/pull/14030 - Commit messages: - Merge branch 'master' into javadoc_fixes - Add overflow tests - Fix overflow @throw in MemorySegment::copy - Add overflow @throws clause on SegmentAllocator::allocateArray - More fixes to MemoryLayout - Finish ValueLayout - Merge branch 'master' into javadoc_fixes - Finish SymbolLookup - Finish SequenceLayout/StructLayout/UnionLayout - Finish SegmentAllocator - ... and 11 more: https://git.openjdk.org/jdk/compare/c0c4d771...afb7360e Changes: https://git.openjdk.org/jdk/pull/14098/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=14098&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8308645 Stats: 818 lines in 23 files changed: 269 ins; 116 del; 433 mod Patch: https://git.openjdk.org/jdk/pull/14098.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14098/head:pull/14098 PR: https://git.openjdk.org/jdk/pull/14098
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed
On Tue, 23 May 2023 11:48:59 GMT, Maurizio Cimadamore 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 throw because of an ... Javadoc: https://cr.openjdk.org/~mcimadamore/jdk/8308645/javadoc/java.base/java/lang/foreign/package-summary.html - PR Comment: https://git.openjdk.org/jdk/pull/14098#issuecomment-1559552992
