Re: RFR: 8295044: Implementation of Foreign Function and Memory API (Second Preview) [v27]
On Tue, 15 Nov 2022 18:47:39 GMT, Maurizio Cimadamore
wrote:
>> This PR contains the API and implementation changes for JEP-434 [1]. A more
>> detailed description of such changes, to avoid repetitions during the review
>> process, is included as a separate comment.
>>
>> [1] - https://openjdk.org/jeps/434
>
> Maurizio Cimadamore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Fix typo in SegmentScope javadoc
src/java.base/share/classes/jdk/internal/foreign/MemorySessionImpl.java line 77:
> 75: } catch (Throwable ex) {
> 76: throw new ExceptionInInitializerError(ex);
> 77: }
The above `catch` clause should only catch `Exception`s, not `Throwable`s, as
the latter would hide VM errors such as `StackOverflowError` or
`OutOfMemoryError`.
-
PR: https://git.openjdk.org/jdk/pull/10872
Re: RFR: 8295044: Implementation of Foreign Function and Memory API (Second Preview) [v27]
On Tue, 15 Nov 2022 18:47:39 GMT, Maurizio Cimadamore
wrote:
>> This PR contains the API and implementation changes for JEP-434 [1]. A more
>> detailed description of such changes, to avoid repetitions during the review
>> process, is included as a separate comment.
>>
>> [1] - https://openjdk.org/jeps/434
>
> Maurizio Cimadamore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Fix typo in SegmentScope javadoc
src/java.base/share/classes/java/lang/foreign/Arena.java line 132:
> 130: * and all the memory segments associated with it can no longer be
> accessed. Furthermore, any off-heap region of memory backing the
> 131: * segments associated with that scope are also released.
> 132: * @throws IllegalStateException if the arena has already been
> {@linkplain #close() closed}.
JavaDoc was pointing to itself.
Suggestion:
* @throws IllegalStateException if the arena has already been closed.
src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 109:
> 107: * Finally, access operations on a memory segment are subject to the
> thread-confinement checks enforced by the associated
> 108: * scope; that is, if the segment is the {@linkplain
> SegmentScope#global() global scope} or an {@linkplain SegmentScope#auto()
> automatic scope},
> 109: * it can be accessed by multiple threads. If the segment is associatd
> with an arena scope, then it can only be
Typo:
Suggestion:
* it can be accessed by multiple threads. If the segment is associated with an
arena scope, then it can only be
src/java.base/share/classes/java/lang/foreign/SegmentScope.java line 10:
> 8: * A segment scope controls access to a memory segment.
> 9: *
> 10: * A memory segment can only be accessed while its scope is {@linkplain
> #isAlive() alive}. Moreoever,
Typo:
Suggestion:
* A memory segment can only be accessed while its scope is {@linkplain
#isAlive() alive}. Moreover,
-
PR: https://git.openjdk.org/jdk/pull/10872
Re: RFR: 8295044: Implementation of Foreign Function and Memory API (Second Preview) [v27]
On Wed, 16 Nov 2022 16:41:45 GMT, Maurizio Cimadamore
wrote:
>> Most of the AutoCloseable in the platform are Closeables where close is
>> specified to have no effect when already closed. With a confined Arena it
>> would be benign for the owner to invoke close again. If it's been useful at
>> finding bugs then okay. The scenario that made me wonder about this is
>> something like the follow where MyWrapper::close invokes Arena::close.
>>
>> try (var arena = Arena.openConfined();
>> var wrapper = new MyWrapper(arena)) {
>> :
>> }
>
> Actually, I see that the `@apiNote` we used to have has disappeared in the
> API reshuffling. I will add it back.
> Most of the AutoCloseable in the platform are Closeables where close is
> specified to have no effect when already closed. With a confined Arena it
> would be benign for the owner to invoke close again. If it's been useful at
> finding bugs then okay. The scenario that made me wonder about this is
> something like the follow where MyWrapper::close invokes Arena::close.
>
> ```
> try (var arena = Arena.openConfined();
> var wrapper = new MyWrapper(arena)) {
> :
> }
> ```
Sure - this would be problematic - however it seems an edge case (could the TWR
just use MyWrapper?)
I'd prefer to leave it as is for now, and revisit - so far we had no
indications of this being a real problem, whereas we had cases where the thrown
exception has been useful to spot issues. If consistency with the rest of the
JDK is considered more important we can fix it later.
-
PR: https://git.openjdk.org/jdk/pull/10872
Re: RFR: 8295044: Implementation of Foreign Function and Memory API (Second Preview) [v27]
On Wed, 16 Nov 2022 16:13:16 GMT, Maurizio Cimadamore
wrote:
>> src/java.base/share/classes/java/lang/foreign/Arena.java line 132:
>>
>>> 130: * and all the memory segments associated with it can no longer be
>>> accessed. Furthermore, any off-heap region of memory backing the
>>> 131: * segments associated with that scope are also released.
>>> 132: * @throws IllegalStateException if the arena has already been
>>> {@linkplain #close() closed}.
>>
>> It's not wrong to specify that close throw if already closed but it goes
>> against the advice in AutoCloseable to try to have close methods be
>> idempotent. There may be a good reason for this but I can't help wondering
>> if there are error cases when wrapping that might lead to close being called
>> more than once.
>
> In our experience with using the API, having exceptions when something is
> funny about close is very valuable info (as also stated in the javadoc).
> Almost always there's a subtle temporal bug going on which the ISE catches.
> I'm not sure if here you refer to the fact that the javadoc is being overly
> broad in saying "already been closed" instead of "already been closed
> _successfully_" ? What kind of problems are you thinking of?
Most of the AutoCloseable in the platform are Closeables where close is
specified to have no effect when already closed. With a confined Arena it would
be benign for the owner to invoke close again. If it's been useful at finding
bugs then okay. The scenario that made me wonder about this is something like
the follow where MyWrapper::close invokes Arena::close.
try (var arena = Arena.openConfined();
var wrapper = new MyWrapper(arena)) {
:
}
-
PR: https://git.openjdk.org/jdk/pull/10872
Re: RFR: 8295044: Implementation of Foreign Function and Memory API (Second Preview) [v27]
On Wed, 16 Nov 2022 16:38:10 GMT, Alan Bateman wrote:
>> In our experience with using the API, having exceptions when something is
>> funny about close is very valuable info (as also stated in the javadoc).
>> Almost always there's a subtle temporal bug going on which the ISE catches.
>> I'm not sure if here you refer to the fact that the javadoc is being overly
>> broad in saying "already been closed" instead of "already been closed
>> _successfully_" ? What kind of problems are you thinking of?
>
> Most of the AutoCloseable in the platform are Closeables where close is
> specified to have no effect when already closed. With a confined Arena it
> would be benign for the owner to invoke close again. If it's been useful at
> finding bugs then okay. The scenario that made me wonder about this is
> something like the follow where MyWrapper::close invokes Arena::close.
>
> try (var arena = Arena.openConfined();
> var wrapper = new MyWrapper(arena)) {
> :
> }
Actually, I see that the `@apiNote` we used to have has disappeared in the API
reshuffling. I will add it back.
-
PR: https://git.openjdk.org/jdk/pull/10872
Re: RFR: 8295044: Implementation of Foreign Function and Memory API (Second Preview) [v27]
On Tue, 15 Nov 2022 18:47:39 GMT, Maurizio Cimadamore wrote: >> This PR contains the API and implementation changes for JEP-434 [1]. A more >> detailed description of such changes, to avoid repetitions during the review >> process, is included as a separate comment. >> >> [1] - https://openjdk.org/jeps/434 > > Maurizio Cimadamore has updated the pull request incrementally with one > additional commit since the last revision: > > Fix typo in SegmentScope javadoc src/java.base/share/classes/java/lang/foreign/SegmentScope.java line 8: > 6: > 7: /** > 8: * A segment scope controls access to a memory segment. A passing comment here is that "to a memory segment" hints of one-to-one relationship when it's actually one-to-many. Arena is specified to control the lifecycle "of memory segments". - PR: https://git.openjdk.org/jdk/pull/10872
Re: RFR: 8295044: Implementation of Foreign Function and Memory API (Second Preview) [v27]
On Wed, 16 Nov 2022 16:01:52 GMT, Alan Bateman wrote:
>> Maurizio Cimadamore has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Fix typo in SegmentScope javadoc
>
> src/java.base/share/classes/java/lang/foreign/Arena.java line 132:
>
>> 130: * and all the memory segments associated with it can no longer be
>> accessed. Furthermore, any off-heap region of memory backing the
>> 131: * segments associated with that scope are also released.
>> 132: * @throws IllegalStateException if the arena has already been
>> {@linkplain #close() closed}.
>
> It's not wrong to specify that close throw if already closed but it goes
> against the advice in AutoCloseable to try to have close methods be
> idempotent. There may be a good reason for this but I can't help wondering if
> there are error cases when wrapping that might lead to close being called
> more than once.
In our experience with using the API, having exceptions when something is funny
about close is very valuable info (as also stated in the javadoc). Almost
always there's a subtle temporal bug going on which the ISE catches. I'm not
sure if here you refer to the fact that the javadoc is being overly broad in
saying "already been closed" instead of "already been closed _successfully_" ?
What kind of problems are you thinking of?
-
PR: https://git.openjdk.org/jdk/pull/10872
Re: RFR: 8295044: Implementation of Foreign Function and Memory API (Second Preview) [v27]
On Tue, 15 Nov 2022 18:47:39 GMT, Maurizio Cimadamore
wrote:
>> This PR contains the API and implementation changes for JEP-434 [1]. A more
>> detailed description of such changes, to avoid repetitions during the review
>> process, is included as a separate comment.
>>
>> [1] - https://openjdk.org/jeps/434
>
> Maurizio Cimadamore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Fix typo in SegmentScope javadoc
src/java.base/share/classes/java/lang/foreign/Arena.java line 132:
> 130: * and all the memory segments associated with it can no longer be
> accessed. Furthermore, any off-heap region of memory backing the
> 131: * segments associated with that scope are also released.
> 132: * @throws IllegalStateException if the arena has already been
> {@linkplain #close() closed}.
It's not wrong to specify that close throw if already closed but it goes
against the advice in AutoCloseable to try to have close methods be idempotent.
There may be a good reason for this but I can't help wondering if there are
error cases when wrapping that might lead to close being called more than once.
-
PR: https://git.openjdk.org/jdk/pull/10872
Re: RFR: 8295044: Implementation of Foreign Function and Memory API (Second Preview) [v27]
> This PR contains the API and implementation changes for JEP-434 [1]. A more > detailed description of such changes, to avoid repetitions during the review > process, is included as a separate comment. > > [1] - https://openjdk.org/jeps/434 Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Fix typo in SegmentScope javadoc - Changes: - all: https://git.openjdk.org/jdk/pull/10872/files - new: https://git.openjdk.org/jdk/pull/10872/files/5f60d052..876587c3 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10872=26 - incr: https://webrevs.openjdk.org/?repo=jdk=10872=25-26 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/10872.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10872/head:pull/10872 PR: https://git.openjdk.org/jdk/pull/10872
