Re: RFR: 8275063: Implementation of Foreign Function & Memory API (Second incubator) [v14]
> This PR contains the API and implementation changes for JEP-419 [1]. A more > detailed description of such changes, to avoid repetitions during the review > process, is included as a separate comment. > > [1] - https://openjdk.java.net/jeps/419 Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Fix long comment line in Module.java - Changes: - all: https://git.openjdk.java.net/jdk/pull/5907/files - new: https://git.openjdk.java.net/jdk/pull/5907/files/1126133a..c219ae12 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5907=13 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5907=12-13 Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/5907.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5907/head:pull/5907 PR: https://git.openjdk.java.net/jdk/pull/5907
Re: RFR: 8275063: Implementation of Foreign Function & Memory API (Second incubator) [v12]
On Tue, 2 Nov 2021 19:02:51 GMT, Maurizio Cimadamore wrote: >> What is missing, I think, is a check (size > arenaSize) at the beginning of >> the method (we only check this in one of the paths). But we need to check >> before and after, I think, as it is possible to allocate a segment and then >> realize that we ended up overflowing the arena size. > > While what I said above correctly reflects what the implementation does, I > think a broader issue is that the arena allocator implementation is > allocating sometimes more native memory than what its contract specifies. > While in some cases we can prevent that, I think in the general case (e.g. > where we allocate a new block) we cannot, unless we add extra API guarantees > - e.g. that the arena size should be a multiple of the block size (but then > we'd have to special case `Long.MAX_VALUE`, or maybe pick a "big enough" > power of two instead) Maybe we should not support block size in the case of a bounded arena. i.e. just allocate the whole thing upfront, and have 3 APIs: 1. arena with no bounds and default block size. 2. arena with no bounds and custom block size. 3. arena with bounds, that has no blocks size but allocates the whole thing in one go (could be modeled as block size = arena size). Right now we have 1. and 2., but instead of 3. we have a variant that allows setting both the arena size and block size. If we want to keep what we currently have, I'd suggest changing the arena size to a block count for the variant that takes both the arena size and the block size (I think in that case `Long.MAX_VALUE` should still work?). Any ways, that seems like something that could be addressed in 19 as well. - PR: https://git.openjdk.java.net/jdk/pull/5907
Re: RFR: 8275063: Implementation of Foreign Function & Memory API (Second incubator) [v12]
On Tue, 2 Nov 2021 18:55:47 GMT, Maurizio Cimadamore wrote: >> I'll have to think some more about this. I don't think this is covered >> inside the block - that is, the block tries to allocate, and then in the >> finally we throw if we realized we've allocated too much. > > What is missing, I think, is a check (size > arenaSize) at the beginning of > the method (we only check this in one of the paths). But we need to check > before and after, I think, as it is possible to allocate a segment and then > realize that we ended up overflowing the arena size. While what I said above correctly reflects what the implementation does, I think a broader issue is that the arena allocator implementation is allocating sometimes more native memory than what its contract specifies. While in some cases we can prevent that, I think in the general case (e.g. where we allocate a new block) we cannot, unless we add extra API guarantees - e.g. that the arena size should be a multiple of the block size (but then we'd have to special case `Long.MAX_VALUE`, or maybe pick a "big enough" power of two instead) - PR: https://git.openjdk.java.net/jdk/pull/5907
Re: RFR: 8275063: Implementation of Foreign Function & Memory API (Second incubator) [v12]
On Tue, 2 Nov 2021 18:48:57 GMT, Maurizio Cimadamore
wrote:
>> src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/ArenaAllocator.java
>> line 88:
>>
>>> 86: if (size > arenaSize) {
>>> 87: throw new OutOfMemoryError();
>>> 88: }
>>
>> Isn't this already covered by the `finally` block? Also, this seems to be
>> checking the unaltered `size`, which I think should have been already done
>> at the end of the previous `allocate` call right?
>
> I'll have to think some more about this. I don't think this is covered inside
> the block - that is, the block tries to allocate, and then in the finally we
> throw if we realized we've allocated too much.
What is missing, I think, is a check (size > arenaSize) at the beginning of the
method (we only check this in one of the paths). But we need to check before
and after, I think, as it is possible to allocate a segment and then realize
that we ended up overflowing the arena size.
-
PR: https://git.openjdk.java.net/jdk/pull/5907
Re: RFR: 8275063: Implementation of Foreign Function & Memory API (Second incubator) [v13]
On Tue, 2 Nov 2021 19:35:29 GMT, Maurizio Cimadamore
wrote:
>> This PR contains the API and implementation changes for JEP-419 [1]. A more
>> detailed description of such changes, to avoid repetitions during the review
>> process, is included as a separate comment.
>>
>> [1] - https://openjdk.java.net/jeps/419
>
> Maurizio Cimadamore has updated the pull request incrementally with two
> additional commits since the last revision:
>
> - Address impl review comments
> - Address API review comments
src/java.base/share/classes/java/lang/Module.java line 114:
> 112:
> 113: // true, if this module allows restricted native access; @Stable
> makes sure that modules that allow native
> 114: // access capture this property as a constant.
Do you mind fixing this comment to avoid the really long line, it sticks out
compare to everything else around it.
src/java.base/share/classes/sun/nio/ch/IOUtil.java line 478:
> 476: private static final JavaNioAccess NIO_ACCESS =
> SharedSecrets.getJavaNioAccess();
> 477:
> 478: static Runnable acquireScope(ByteBuffer bb, boolean async) {
At some point (not this PR) we should move the "async" out of this file, IOUtil
was for synchronous I/O.
-
PR: https://git.openjdk.java.net/jdk/pull/5907
Re: RFR: 8275063: Implementation of Foreign Function & Memory API (Second incubator) [v13]
> This PR contains the API and implementation changes for JEP-419 [1]. A more > detailed description of such changes, to avoid repetitions during the review > process, is included as a separate comment. > > [1] - https://openjdk.java.net/jeps/419 Maurizio Cimadamore has updated the pull request incrementally with two additional commits since the last revision: - Address impl review comments - Address API review comments - Changes: - all: https://git.openjdk.java.net/jdk/pull/5907/files - new: https://git.openjdk.java.net/jdk/pull/5907/files/7cf4fcd9..1126133a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5907=12 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5907=11-12 Stats: 103 lines in 11 files changed: 8 ins; 23 del; 72 mod Patch: https://git.openjdk.java.net/jdk/pull/5907.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5907/head:pull/5907 PR: https://git.openjdk.java.net/jdk/pull/5907
Re: RFR: 8275063: Implementation of Foreign Function & Memory API (Second incubator) [v12]
On Tue, 2 Nov 2021 16:51:06 GMT, Jorn Vernee wrote:
>> Maurizio Cimadamore has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Tweak javadoc of loaderLookup
>
> src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/ArenaAllocator.java
> line 88:
>
>> 86: if (size > arenaSize) {
>> 87: throw new OutOfMemoryError();
>> 88: }
>
> Isn't this already covered by the `finally` block? Also, this seems to be
> checking the unaltered `size`, which I think should have been already done at
> the end of the previous `allocate` call right?
I'll have to think some more about this. I don't think this is covered inside
the block - that is, the block tries to allocate, and then in the finally we
throw if we realized we've allocated too much.
-
PR: https://git.openjdk.java.net/jdk/pull/5907
Re: RFR: 8275063: Implementation of Foreign Function & Memory API (Second incubator) [v10]
On Mon, 1 Nov 2021 12:05:32 GMT, Maurizio Cimadamore
wrote:
>> This PR contains the API and implementation changes for JEP-419 [1]. A more
>> detailed description of such changes, to avoid repetitions during the review
>> process, is included as a separate comment.
>>
>> [1] - https://openjdk.java.net/jeps/419
>
> Maurizio Cimadamore has updated the pull request with a new target base due
> to a merge or a rebase. The pull request now contains 17 commits:
>
> - Add cache for memory address var handles
> - Merge branch 'master' into JEP-419
> - Fix regression in VaList treatment on AArch64 (contributed by @nick-arm)
> - Merge branch 'master' into JEP-419
> - Fix copyright header in TestArrayCopy
> - Fix failing microbenchmarks. Contributed by @FrauBoes (thanks!)
> - * use `invokeWithArguments` to simplify new test
> - Add test for liveness check with high-aririty downcalls
>(make sure that if an exception occurs in a downcall because of liveness,
>ref count of other resources are left intact).
> - * Fix javadoc issue in VaList
>* Fix bug in concurrent logic for shared scope acquire
> - Address review comments
> - ... and 7 more:
> https://git.openjdk.java.net/jdk/compare/5bb1992b...9b519343
src/java.base/share/classes/java/lang/invoke/MethodHandleImpl.java line 1586:
> 1584: public void ensureCustomized(MethodHandle mh) {
> 1585: mh.customize();
> 1586: }
This is no longer needed, but it probably got picked up in the merge.
src/java.base/share/classes/jdk/internal/access/JavaLangInvokeAccess.java line
144:
> 142: * @param mh the method handle
> 143: */
> 144: void ensureCustomized(MethodHandle mh);
Same here, no longer needed. (it was used by now removed upcall handler code.
See https://github.com/openjdk/panama-foreign/pull/553)
src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemoryAddress.java
line 107:
> 105: *
> 106: * @param offset offset in bytes (relative to this address). The
> final address of this read operation can be expressed as {@code
> toRowLongValue() + offset}.
> 107: * @return a Java UTF-8 string containing all the bytes read from
> the given starting address ({@code toRowLongValue() + offset})
(see also comment on MemorySegment.getUtf8String)
Suggestion:
* @return a Java string constructed from the bytes read from the given
starting address ({@code toRowLongValue() + offset})
src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegment.java
line 387:
> 385:
> 386: /**
> 387: * Performs an element-wise bulk copy from given source segment to
> this segment. More specifically, the bytes at
Suggestion:
* Performs a byte-wise bulk copy from given source segment to this
segment. More specifically, the bytes at
src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegment.java
line 400:
> 398: * a multiple of the source element layout size, if the source
> segment is incompatible with the alignment constraints
> 399: * in the source element layout, or if this segment is incompatible
> with the alignment constraints
> 400: * in the destination element layout.
This speaks about element layouts, but I don't see any element layouts in the
method implementation.
src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegment.java
line 633:
> 631: * java.nio.charset.CharsetDecoder} class should be used when more
> control
> 632: * over the decoding process is required.
> 633: * @param offset offset in bytes (relative to this segment). For
> instance, if this segment is a {@link #isNative()} segment,
Suggestion:
* @param offset offset in bytes (relative to this segment). For instance,
if this segment is a {@link #isNative() native} segment,
src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegment.java
line 636:
> 634: * the final address of this read operation can be
> expressed as {@code address().toRowLongValue() + offset}.
> 635: * @return a Java UTF-8 string containing all the bytes read from
> the given starting address up to (but not including)
> 636: * the first {@code '\0'} terminator character (assuming one is
> found).
The phrase "a Java UTF-8 string" sounds strange to me, as Java Strings are not
encoded in UTF-8. The string that is read is UTF-8 encoded, but then it is
converted from UTF-8 to Java internal String encoding (UTF-16 or Latin1).
I'd suggest just dropping the 'UTF-8', and changing 'containing all' to
'constructed from'.
Suggestion:
* @return a Java string constructed from the bytes read from the given
starting address up to (but not including)
* the first {@code '\0'} terminator character (assuming one is found).
src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegment.java
line 652:
> 650: * java.nio.charset.CharsetDecoder} class should
Re: RFR: 8275063: Implementation of Foreign Function & Memory API (Second incubator) [v10]
On Mon, 1 Nov 2021 15:38:18 GMT, Jorn Vernee 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 17 commits:
>>
>> - Add cache for memory address var handles
>> - Merge branch 'master' into JEP-419
>> - Fix regression in VaList treatment on AArch64 (contributed by @nick-arm)
>> - Merge branch 'master' into JEP-419
>> - Fix copyright header in TestArrayCopy
>> - Fix failing microbenchmarks. Contributed by @FrauBoes (thanks!)
>> - * use `invokeWithArguments` to simplify new test
>> - Add test for liveness check with high-aririty downcalls
>>(make sure that if an exception occurs in a downcall because of liveness,
>>ref count of other resources are left intact).
>> - * Fix javadoc issue in VaList
>>* Fix bug in concurrent logic for shared scope acquire
>> - Address review comments
>> - ... and 7 more:
>> https://git.openjdk.java.net/jdk/compare/5bb1992b...9b519343
>
> src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegment.java
> line 1035:
>
>> 1033: *
>> 1034: * @param layout the layout of the memory region to be read.
>> 1035: * @param offset offset in bytes (relative to this segment). For
>> instance, if this segment is a {@link #isNative()} segment,
>
> Suggestion:
>
> * @param offset offset in bytes (relative to this segment). For
> instance, if this segment is a {@link #isNative() native} segment,
Same suggestion with all the other getters/setters below (I assume you wanted
to add text to the link here?)
> src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegment.java
> line 1549:
>
>> 1547: * @param index index (relative to this segment). For instance, if
>> this segment is a {@link #isNative()} segment,
>> 1548: * the final address of this write operation can be
>> expressed as {@code address().toRowLongValue() + (index *
>> layout.byteSize())}.
>> 1549: * @param value the byte value to be written.
>
> Suggestion:
>
> * @param value the address value to be written.
I think all the setters have this problem.
-
PR: https://git.openjdk.java.net/jdk/pull/5907
Re: RFR: 8275063: Implementation of Foreign Function & Memory API (Second incubator) [v12]
On Mon, 1 Nov 2021 22:36:40 GMT, Maurizio Cimadamore
wrote:
>> This PR contains the API and implementation changes for JEP-419 [1]. A more
>> detailed description of such changes, to avoid repetitions during the review
>> process, is included as a separate comment.
>>
>> [1] - https://openjdk.java.net/jeps/419
>
> Maurizio Cimadamore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Tweak javadoc of loaderLookup
Mostly some minor javadoc comments.
src/java.base/share/classes/java/lang/Module.java line 32:
> 30: import java.lang.annotation.Annotation;
> 31: import java.lang.invoke.MethodHandle;
> 32: import java.lang.invoke.VarHandle;
These imports seem spurious now.
src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/ValueLayout.java
line 177:
> 175: }
> 176: if (carrier.isPrimitive() &&
> Wrapper.forPrimitiveType(carrier).bitWidth() != size &&
> 177: carrier != boolean.class && size != 8) {
I find this condition hard to parse, I'd suggest re-writing it as:
if (carrier.isPrimitive()) {
long expectedSize = carrier == boolean.class ? 8 :
Wrapper.forPrimitiveType(carrier).bitWidth();
if (size != expectedSize) {
throw ...
}
}
(Maybe even change the `if` to an `else` and combine it with the above if).
src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/ValueLayout.java
line 484:
> 482: public static final class OfAddress extends ValueLayout {
> 483: OfAddress(ByteOrder order) {
> 484: super(MemoryAddress.class, order, Unsafe.ADDRESS_SIZE * 8);
I see `Unsafe.ADDRESS_SIZE` used in several places, suggest to maybe add an
`ADDRESS_SIZE_BITS` constants somewhere (it's a bit more readable).
src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/ArenaAllocator.java
line 42:
> 40: final long blockSize;
> 41: final long arenaSize;
> 42: final ResourceScope scope;
Could these field be made private?
src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/ArenaAllocator.java
line 88:
> 86: if (size > arenaSize) {
> 87: throw new OutOfMemoryError();
> 88: }
Isn't this already covered by the `finally` block? Also, this seems to be
checking the unaltered `size`, which I think should have been already done at
the end of the previous `allocate` call right?
src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/ResourceScopeImpl.java
line 122:
> 120: ResourceScopeImpl targetImpl = (ResourceScopeImpl)target;
> 121: targetImpl.acquire0();
> 122: addCloseAction(targetImpl::release0);
Maybe this should explicitly check if target is `null` (though the call to
`acquire0` would also produce an NPE, the stack trace having
Objects::requireNonNull in there would make the error more obvious I think).
Suggestion:
public void keepAlive(ResourceScope target) {
Objects.requireNonNull(target);
if (target == this) {
throw new IllegalArgumentException("Invalid target scope.");
}
ResourceScopeImpl targetImpl = (ResourceScopeImpl)target;
targetImpl.acquire0();
addCloseAction(targetImpl::release0);
src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/SharedScope.java
line 101:
> 99: int value;
> 100: do {
> 101: value = (int)
> STATE.getVolatile(jdk.internal.foreign.SharedScope.this);
Doesn't need to be fully qualified I think?
Suggestion:
value = (int) STATE.getVolatile(this);
src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/SharedScope.java
line 106:
> 104: throw new IllegalStateException("Already closed");
> 105: }
> 106: } while
> (!STATE.compareAndSet(jdk.internal.foreign.SharedScope.this, value, value -
> 1));
Same here
Suggestion:
} while (!STATE.compareAndSet(this, value, value - 1));
-
Marked as reviewed by jvernee (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/5907
Re: RFR: 8275063: Implementation of Foreign Function & Memory API (Second incubator) [v12]
On Mon, 1 Nov 2021 22:36:40 GMT, Maurizio Cimadamore wrote: >> This PR contains the API and implementation changes for JEP-419 [1]. A more >> detailed description of such changes, to avoid repetitions during the review >> process, is included as a separate comment. >> >> [1] - https://openjdk.java.net/jeps/419 > > Maurizio Cimadamore has updated the pull request incrementally with one > additional commit since the last revision: > > Tweak javadoc of loaderLookup Marked as reviewed by psandoz (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5907
Re: RFR: 8275063: Implementation of Foreign Function & Memory API (Second incubator) [v10]
On Tue, 2 Nov 2021 10:30:42 GMT, Maurizio Cimadamore
wrote:
>> src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/Utils.java line
>> 111:
>>
>>> 109: class VarHandleCache {
>>> 110: private static final Map handleMap
>>> = new ConcurrentHashMap<>();
>>> 111: private static final Map
>>> handleMapNoAlignCheck = new ConcurrentHashMap<>();
>>
>> Something to consider later if this is an issue. Since the number of
>> `ValueLayout` instances is fixed, carrier x order = 18, we can use stable
>> arrays with ordinals on the instances.
>
> What about alignment?
Drat, `skipAlignmentCheck` misled me but perhaps there is still benefit for
common constants with 8 bit and size alignment and fallback otherwise.
-
PR: https://git.openjdk.java.net/jdk/pull/5907
Integrated: 8275766: (tz) Update Timezone Data to 2021e
On Thu, 28 Oct 2021 01:02:27 GMT, Yoshiki Sato wrote: > Please review the integration of tzdata2021e (including tzdata2021d) to the > JDK. > The fix has passed all relevant JTREG regression tests and JCK tests. > > 8275754: (tz) Update Timezone Data to 2021d > 8275849: TestZoneInfo310.java fails with tzdata2021e This pull request has now been integrated. Changeset: 5b4e3986 Author:Yoshiki Sato Committer: Sean Coffey URL: https://git.openjdk.java.net/jdk/commit/5b4e39863dbc0d61e91675261dd6887f704ab868 Stats: 52 lines in 6 files changed: 29 ins; 6 del; 17 mod 8275766: (tz) Update Timezone Data to 2021e 8275849: TestZoneInfo310.java fails with tzdata2021e Reviewed-by: naoto, iris, erikj, coffeys - PR: https://git.openjdk.java.net/jdk/pull/6144
Integrated: JDK-8275406: Add copy-to-clipboard feature to snippet UI
On Tue, 19 Oct 2021 13:51:03 GMT, Hannes Wallnöfer wrote: > Please review a change to add a copy-to-clipboard feature to snippets. I took > special care to make the feature usable on mobile devices. Sample output can > be viewed and tested here: > > http://cr.openjdk.java.net/~hannesw/8275406/api.00/java.base/java/lang/Throwable.html#printStackTrace() This pull request has now been integrated. Changeset: 8630f55e Author:Hannes Wallnöfer URL: https://git.openjdk.java.net/jdk/commit/8630f55ed7a0483ec5dcb13a7f53b52bc4ab6fc6 Stats: 180 lines in 14 files changed: 166 ins; 0 del; 14 mod 8275406: Add copy-to-clipboard feature to snippet UI Reviewed-by: erikj, jjg - PR: https://git.openjdk.java.net/jdk/pull/6011
Re: RFR: 8275063: Implementation of Foreign Function & Memory API (Second incubator) [v10]
On Tue, 2 Nov 2021 00:24:12 GMT, Paul Sandoz 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 17 commits:
>>
>> - Add cache for memory address var handles
>> - Merge branch 'master' into JEP-419
>> - Fix regression in VaList treatment on AArch64 (contributed by @nick-arm)
>> - Merge branch 'master' into JEP-419
>> - Fix copyright header in TestArrayCopy
>> - Fix failing microbenchmarks. Contributed by @FrauBoes (thanks!)
>> - * use `invokeWithArguments` to simplify new test
>> - Add test for liveness check with high-aririty downcalls
>>(make sure that if an exception occurs in a downcall because of liveness,
>>ref count of other resources are left intact).
>> - * Fix javadoc issue in VaList
>>* Fix bug in concurrent logic for shared scope acquire
>> - Address review comments
>> - ... and 7 more:
>> https://git.openjdk.java.net/jdk/compare/5bb1992b...9b519343
>
> src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/Utils.java line
> 111:
>
>> 109: class VarHandleCache {
>> 110: private static final Map handleMap
>> = new ConcurrentHashMap<>();
>> 111: private static final Map
>> handleMapNoAlignCheck = new ConcurrentHashMap<>();
>
> Something to consider later if this is an issue. Since the number of
> `ValueLayout` instances is fixed, carrier x order = 18, we can use stable
> arrays with ordinals on the instances.
What about alignment?
-
PR: https://git.openjdk.java.net/jdk/pull/5907
Re: RFR: 8275766: (tz) Update Timezone Data to 2021e
On Thu, 28 Oct 2021 01:02:27 GMT, Yoshiki Sato wrote: > Please review the integration of tzdata2021e (including tzdata2021d) to the > JDK. > The fix has passed all relevant JTREG regression tests and JCK tests. > > 8275754: (tz) Update Timezone Data to 2021d > 8275849: TestZoneInfo310.java fails with tzdata2021e Marked as reviewed by coffeys (Reviewer). I think you've enough Reviewers already but yes, this looks fine. - PR: https://git.openjdk.java.net/jdk/pull/6144
