Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v45]
On Fri, 13 May 2022 09:42:44 GMT, Maurizio Cimadamore wrote: >> This change here also closes >> [JDK-8259034](https://bugs.openjdk.java.net/browse/JDK-8259034) > > @uschindler - the issue you mention with respect lack of UOE for wrong file > system applies to BB as well. I suggest filing an issue. See https://bugs.openjdk.java.net/browse/JDK-8286735 - PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v45]
On Fri, 13 May 2022 11:59:12 GMT, Maurizio Cimadamore wrote: >> RFE = issue? > >> RFE = issue? > > issue, with type RFE (request for enhancement) See: https://bugs.openjdk.java.net/browse/JDK-8286734 - PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v45]
On Fri, 13 May 2022 11:01:09 GMT, Uwe Schindler wrote: > RFE = issue? issue, with type RFE (request for enhancement) - PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v45]
On Fri, 13 May 2022 09:43:55 GMT, Maurizio Cimadamore
wrote:
>> src/java.base/share/classes/sun/nio/ch/FileChannelImpl.java line 1164:
>>
>>> 1162: }
>>> 1163: if (unmapper != null) {
>>> 1164: AbstractMemorySegmentImpl segment = new
>>> MappedMemorySegmentImpl(unmapper.address(), unmapper, size,
>>
>> When reviewing the method for MappedByteBuffer: I think to make this
>> consistent the "old" method should also use `address()` which applies the
>> pagePosition. Currently it is confusing:
>> - New code returning `MemorySegment` uses `unmapper.address()`
>> - Old code returning `MappedByteBuffer` uses `unmapper.address +
>> unmapper.pagePosition` (fields)
>>
>> Should I open an issue or a PR to fix this (because this is already merged)?
>>
>> See the mailing list posts:
>> - https://mail.openjdk.java.net/pipermail/panama-dev/2022-May/016981.html
>> - https://mail.openjdk.java.net/pipermail/panama-dev/2022-May/016990.html
>
> Please file an RFE. I suspect that there will be more little improvements and
> consolidation like this we'll want to make to this code.
RFE = issue?
-
PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v45]
On Fri, 13 May 2022 08:33:11 GMT, Uwe Schindler wrote: >> src/java.base/share/classes/java/nio/channels/FileChannel.java line 1045: >> >>> 1043: * >>> 1044: * @throws UnsupportedOperationException >>> 1045: * If an unsupported map mode is specified. >> >> I think this should mention that UOE is also throws if the filesystem >> implementation does not support memory mapping. This may happen for >> filesystems like jrtfs or custom wrapper filesystems that do not implement >> the method below. >> >> As this is already merged, should I open a PR fixing the docs or open an >> issue? > > This change here also closes > [JDK-8259034](https://bugs.openjdk.java.net/browse/JDK-8259034) @uschindler - the issue you mention with respect lack of UOE for wrong file system applies to BB as well. I suggest filing an issue. - PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v45]
On Fri, 13 May 2022 08:29:13 GMT, Uwe Schindler 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 65 commits:
>>
>> - Merge branch 'master' into foreign-preview
>> - Merge branch 'master' into foreign-preview
>> - Fix crashes in heap segment benchmarks due to misaligned access
>> - Merge branch 'master' into foreign-preview
>> - Update src/java.base/share/classes/jdk/internal/reflect/Reflection.java
>>
>>Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
>> - Add tests for loaderLookup/restricted method corner cases
>> - * Clarify what happens when SymbolLookup::loaderLookup is invoked from JNI
>>* Tweak restricted method check to work when there's no caller class
>> - Tweak examples in SymbolLookup javadoc
>> - Merge branch 'master' into foreign-preview
>> - Update
>> src/java.base/share/classes/jdk/internal/misc/X-ScopedMemoryAccess.java.template
>>
>>Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
>> - ... and 55 more:
>> https://git.openjdk.java.net/jdk/compare/82aa0455...f170415b
>
> src/java.base/share/classes/sun/nio/ch/FileChannelImpl.java line 1164:
>
>> 1162: }
>> 1163: if (unmapper != null) {
>> 1164: AbstractMemorySegmentImpl segment = new
>> MappedMemorySegmentImpl(unmapper.address(), unmapper, size,
>
> When reviewing the method for MappedByteBuffer: I think to make this
> consistent the "old" method should also use `address()` which applies the
> pagePosition. Currently it is confusing:
> - New code returning `MemorySegment` uses `unmapper.address()`
> - Old code returning `MappedByteBuffer` uses `unmapper.address +
> unmapper.pagePosition` (fields)
>
> Should I open an issue or a PR to fix this (because this is already merged)?
>
> See the mailing list posts:
> - https://mail.openjdk.java.net/pipermail/panama-dev/2022-May/016981.html
> - https://mail.openjdk.java.net/pipermail/panama-dev/2022-May/016990.html
Please file an RFE. I suspect that there will be more little improvements and
consolidation like this we'll want to make to this code.
-
PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v45]
On Thu, 12 May 2022 15:45:01 GMT, Maurizio Cimadamore
wrote:
>> This PR contains the API and implementation changes for JEP-424 [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/424
>
> Maurizio Cimadamore has updated the pull request with a new target base due
> to a merge or a rebase. The pull request now contains 65 commits:
>
> - Merge branch 'master' into foreign-preview
> - Merge branch 'master' into foreign-preview
> - Fix crashes in heap segment benchmarks due to misaligned access
> - Merge branch 'master' into foreign-preview
> - Update src/java.base/share/classes/jdk/internal/reflect/Reflection.java
>
>Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
> - Add tests for loaderLookup/restricted method corner cases
> - * Clarify what happens when SymbolLookup::loaderLookup is invoked from JNI
>* Tweak restricted method check to work when there's no caller class
> - Tweak examples in SymbolLookup javadoc
> - Merge branch 'master' into foreign-preview
> - Update
> src/java.base/share/classes/jdk/internal/misc/X-ScopedMemoryAccess.java.template
>
>Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
> - ... and 55 more:
> https://git.openjdk.java.net/jdk/compare/82aa0455...f170415b
Some late comments and suggestions to fix inconsistencies and wrong javadocs.
src/java.base/share/classes/java/nio/channels/FileChannel.java line 1045:
> 1043: *
> 1044: * @throws UnsupportedOperationException
> 1045: * If an unsupported map mode is specified.
I think this should mention that UOE is also throws if the filesystem
implementation does not support memory mapping. This may happen for filesystems
like jrtfs or custom wrapper filesystems that do not implement the method below.
As this is already merged, should I open a PR fixing the docs or open an issue?
src/java.base/share/classes/sun/nio/ch/FileChannelImpl.java line 1164:
> 1162: }
> 1163: if (unmapper != null) {
> 1164: AbstractMemorySegmentImpl segment = new
> MappedMemorySegmentImpl(unmapper.address(), unmapper, size,
When reviewing the method for MappedByteBuffer: I think to make this consistent
the "old" method should also use `address()` which applies the pagePosition.
Currently it is confusing:
- New code returning `MemorySegment` uses `unmapper.address()`
- Old code returning `MappedByteBuffer` uses `unmapper.address +
unmapper.pagePosition` (fields)
Should I open an issue or a PR to fix this (because this is already merged)?
See the mailing list posts:
- https://mail.openjdk.java.net/pipermail/panama-dev/2022-May/016981.html
- https://mail.openjdk.java.net/pipermail/panama-dev/2022-May/016990.html
-
PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v45]
On Fri, 13 May 2022 08:25:01 GMT, Uwe Schindler 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 65 commits: >> >> - Merge branch 'master' into foreign-preview >> - Merge branch 'master' into foreign-preview >> - Fix crashes in heap segment benchmarks due to misaligned access >> - Merge branch 'master' into foreign-preview >> - Update src/java.base/share/classes/jdk/internal/reflect/Reflection.java >> >>Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> >> - Add tests for loaderLookup/restricted method corner cases >> - * Clarify what happens when SymbolLookup::loaderLookup is invoked from JNI >>* Tweak restricted method check to work when there's no caller class >> - Tweak examples in SymbolLookup javadoc >> - Merge branch 'master' into foreign-preview >> - Update >> src/java.base/share/classes/jdk/internal/misc/X-ScopedMemoryAccess.java.template >> >>Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> >> - ... and 55 more: >> https://git.openjdk.java.net/jdk/compare/82aa0455...f170415b > > src/java.base/share/classes/java/nio/channels/FileChannel.java line 1045: > >> 1043: * >> 1044: * @throws UnsupportedOperationException >> 1045: * If an unsupported map mode is specified. > > I think this should mention that UOE is also throws if the filesystem > implementation does not support memory mapping. This may happen for > filesystems like jrtfs or custom wrapper filesystems that do not implement > the method below. > > As this is already merged, should I open a PR fixing the docs or open an > issue? This change here also closes [JDK-8259034](https://bugs.openjdk.java.net/browse/JDK-8259034) - PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v34]
On Fri, 29 Apr 2022 08:29:52 GMT, Guoxiong Li wrote: >>> Remind: please use the command `/jep JEP-424` [1] to mark this PR. >>> >>> [1] >>> https://wiki.openjdk.java.net/display/SKARA/Pull+Request+Commands#PullRequestCommands-/jep >> >> Question: I'm willing to try it out. If something goes wrong - would a `jep >> unneeded` be enough to "unstuck" this PR? > >> would a jep unneeded be enough to "unstuck" this PR? > > Yes if no bug. Conceptually, the `/jep unneeded` will behave as no jep > command. @lgxbslgx - JEP has been targeted, but the JEP action seems to be blocking this - what should we do? - PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v45]
> This PR contains the API and implementation changes for JEP-424 [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/424 Maurizio Cimadamore has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 65 commits: - Merge branch 'master' into foreign-preview - Merge branch 'master' into foreign-preview - Fix crashes in heap segment benchmarks due to misaligned access - Merge branch 'master' into foreign-preview - Update src/java.base/share/classes/jdk/internal/reflect/Reflection.java Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> - Add tests for loaderLookup/restricted method corner cases - * Clarify what happens when SymbolLookup::loaderLookup is invoked from JNI * Tweak restricted method check to work when there's no caller class - Tweak examples in SymbolLookup javadoc - Merge branch 'master' into foreign-preview - Update src/java.base/share/classes/jdk/internal/misc/X-ScopedMemoryAccess.java.template Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> - ... and 55 more: https://git.openjdk.java.net/jdk/compare/82aa0455...f170415b - Changes: https://git.openjdk.java.net/jdk/pull/7888/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=44 Stats: 65711 lines in 373 files changed: 43720 ins; 19432 del; 2559 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v44]
> This PR contains the API and implementation changes for JEP-424 [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/424 Maurizio Cimadamore has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 64 commits: - Merge branch 'master' into foreign-preview - Fix crashes in heap segment benchmarks due to misaligned access - Merge branch 'master' into foreign-preview - Update src/java.base/share/classes/jdk/internal/reflect/Reflection.java Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> - Add tests for loaderLookup/restricted method corner cases - * Clarify what happens when SymbolLookup::loaderLookup is invoked from JNI * Tweak restricted method check to work when there's no caller class - Tweak examples in SymbolLookup javadoc - Merge branch 'master' into foreign-preview - Update src/java.base/share/classes/jdk/internal/misc/X-ScopedMemoryAccess.java.template Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> - Tweak support for Linker lookup Improve javadoc of SymbolLookup - ... and 54 more: https://git.openjdk.java.net/jdk/compare/73c5e993...cdd006e7 - Changes: https://git.openjdk.java.net/jdk/pull/7888/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=43 Stats: 65711 lines in 373 files changed: 43720 ins; 19432 del; 2559 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v43]
On Mon, 9 May 2022 18:09:51 GMT, ExE Boss wrote: >> Maurizio Cimadamore has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Fix crashes in heap segment benchmarks due to misaligned access > > test/micro/org/openjdk/bench/java/lang/foreign/LoopOverPollutedSegments.java > line 69: > >> 67: static final ValueLayout.OfInt JAVA_INT_UNALIGNED = >> JAVA_INT.withBitAlignment(8); >> 68: static final ValueLayout.OfFloat JAVA_FLOAT_UNALIGNED = >> JAVA_FLOAT.withBitAlignment(8); >> 69: static final VarHandle intHandleUnaligned = >> JAVA_INT_UNALIGNED.arrayElementVarHandle(); > > To match `LoopOverNonConstantHeap`, this should probably be named > `VH_INT_UNALIGNED`. > > > > Maybe these could also be moved into > `org.openjdk.bench.java.lang.foreign.Utils`[^1] > > [^1]: > https://github.com/openjdk/jdk/blob/7b774297b1d04e104a988ea5bd2f2b04c8de3461/test/micro/org/openjdk/bench/java/lang/foreign/Utils.java#L29-L43 I noted other possible cleanups for benchmarks, I'll work on something after we integrate this PR as I'd like to minimize the churn. - PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v43]
On Mon, 9 May 2022 17:41:10 GMT, Maurizio Cimadamore wrote: >> This PR contains the API and implementation changes for JEP-424 [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/424 > > Maurizio Cimadamore has updated the pull request incrementally with one > additional commit since the last revision: > > Fix crashes in heap segment benchmarks due to misaligned access test/micro/org/openjdk/bench/java/lang/foreign/LoopOverPollutedSegments.java line 69: > 67: static final ValueLayout.OfInt JAVA_INT_UNALIGNED = > JAVA_INT.withBitAlignment(8); > 68: static final ValueLayout.OfFloat JAVA_FLOAT_UNALIGNED = > JAVA_FLOAT.withBitAlignment(8); > 69: static final VarHandle intHandleUnaligned = > JAVA_INT_UNALIGNED.arrayElementVarHandle(); To match `LoopOverNonConstantHeap`, this should probably be named `VH_INT_UNALIGNED`. Maybe these could also be moved into `org.openjdk.bench.java.lang.foreign.Utils`[^1] [^1]: https://github.com/openjdk/jdk/blob/7b774297b1d04e104a988ea5bd2f2b04c8de3461/test/micro/org/openjdk/bench/java/lang/foreign/Utils.java#L29-L43 - PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v43]
> This PR contains the API and implementation changes for JEP-424 [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/424 Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Fix crashes in heap segment benchmarks due to misaligned access - Changes: - all: https://git.openjdk.java.net/jdk/pull/7888/files - new: https://git.openjdk.java.net/jdk/pull/7888/files/3c88a2ef..7b774297 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=42 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=41-42 Stats: 41 lines in 2 files changed: 3 ins; 4 del; 34 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v42]
> This PR contains the API and implementation changes for JEP-424 [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/424 Maurizio Cimadamore has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 62 commits: - Merge branch 'master' into foreign-preview - Update src/java.base/share/classes/jdk/internal/reflect/Reflection.java Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> - Add tests for loaderLookup/restricted method corner cases - * Clarify what happens when SymbolLookup::loaderLookup is invoked from JNI * Tweak restricted method check to work when there's no caller class - Tweak examples in SymbolLookup javadoc - Merge branch 'master' into foreign-preview - Update src/java.base/share/classes/jdk/internal/misc/X-ScopedMemoryAccess.java.template Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> - Tweak support for Linker lookup Improve javadoc of SymbolLookup - Tweak Addressable javadoc - Downcall and upcall tests use wrong layout which is missing some trailing padding - ... and 52 more: https://git.openjdk.java.net/jdk/compare/39f4434f...3c88a2ef - Changes: https://git.openjdk.java.net/jdk/pull/7888/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=41 Stats: 65712 lines in 373 files changed: 43721 ins; 19432 del; 2559 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v41]
On Fri, 6 May 2022 16:48:11 GMT, Maurizio Cimadamore wrote: >> This PR contains the API and implementation changes for JEP-424 [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/424 > > Maurizio Cimadamore has updated the pull request incrementally with one > additional commit since the last revision: > > Update src/java.base/share/classes/jdk/internal/reflect/Reflection.java > > Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> Marked as reviewed by mchung (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v41]
> This PR contains the API and implementation changes for JEP-424 [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/424 Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Update src/java.base/share/classes/jdk/internal/reflect/Reflection.java Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> - Changes: - all: https://git.openjdk.java.net/jdk/pull/7888/files - new: https://git.openjdk.java.net/jdk/pull/7888/files/b71c4e93..f823bf84 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=40 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=39-40 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v40]
On Fri, 6 May 2022 11:51:46 GMT, Maurizio Cimadamore wrote: >> This PR contains the API and implementation changes for JEP-424 [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/424 > > Maurizio Cimadamore has updated the pull request incrementally with one > additional commit since the last revision: > > Add tests for loaderLookup/restricted method corner cases src/java.base/share/classes/jdk/internal/reflect/Reflection.java line 116: > 114: // if there is no caller class, act as if the call came from > unnamed module of system class loader > 115: Module module = currentClass != null ? > 116: currentClass.getModule() : > ClassLoader.getSystemClassLoader().getUnnamedModule(); **Nit:** maybe add a line break Suggestion: currentClass.getModule() : ClassLoader.getSystemClassLoader().getUnnamedModule(); - PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v39]
On Fri, 6 May 2022 08:42:12 GMT, Maurizio Cimadamore wrote: >> src/java.base/share/classes/jdk/internal/reflect/Reflection.java line 120: >> >>> 118: // if there is no caller class, act as if the call came from >>> an unnamed module >>> 119: Module module = currentClass != null ? >>> 120: currentClass.getModule() : Holder.FALLBACK_MODULE; >> >> This can be simplified to: >> >> Module module = currentClass != null ? >>currentClass.getModule() : >> ClassLoader::getSystemClassLoader().getUnnamedModule(); >> >> >> No need to have the Holder class. > > gah! I missed that :-) I've addressed these comments (thanks!) and also added some tests for these corner cases. - PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v40]
> This PR contains the API and implementation changes for JEP-424 [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/424 Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Add tests for loaderLookup/restricted method corner cases - Changes: - all: https://git.openjdk.java.net/jdk/pull/7888/files - new: https://git.openjdk.java.net/jdk/pull/7888/files/4d24ffa9..b71c4e93 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=39 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=38-39 Stats: 248 lines in 10 files changed: 239 ins; 4 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v39]
On Thu, 5 May 2022 21:28:32 GMT, Mandy Chung wrote: >> Maurizio Cimadamore has updated the pull request incrementally with one >> additional commit since the last revision: >> >> * Clarify what happens when SymbolLookup::loaderLookup is invoked from JNI >> * Tweak restricted method check to work when there's no caller class > > src/java.base/share/classes/java/lang/foreign/SymbolLookup.java line 161: > >> 159: ClassLoader.getSystemClassLoader(); >> 160: MemorySession loaderSession = (loader == null) ? >> 161: MemorySession.global() : // boot loader never goes away > > The other built-in class loaders (`ClassLoaders::appClassLoader` and > `ClassLoaders::platformClassLoader` are both instance of > `BuiltinClassLoader`) will never be unloaded. Should they use the globel > memory session? good idea > src/java.base/share/classes/jdk/internal/reflect/Reflection.java line 120: > >> 118: // if there is no caller class, act as if the call came from an >> unnamed module >> 119: Module module = currentClass != null ? >> 120: currentClass.getModule() : Holder.FALLBACK_MODULE; > > This can be simplified to: > > Module module = currentClass != null ? >currentClass.getModule() : > ClassLoader::getSystemClassLoader().getUnnamedModule(); > > > No need to have the Holder class. gah! I missed that :-) - PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v39]
On Thu, 5 May 2022 20:54:53 GMT, Maurizio Cimadamore wrote: >> This PR contains the API and implementation changes for JEP-424 [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/424 > > Maurizio Cimadamore has updated the pull request incrementally with one > additional commit since the last revision: > > * Clarify what happens when SymbolLookup::loaderLookup is invoked from JNI > * Tweak restricted method check to work when there's no caller class src/java.base/share/classes/java/lang/foreign/SymbolLookup.java line 161: > 159: ClassLoader.getSystemClassLoader(); > 160: MemorySession loaderSession = (loader == null) ? > 161: MemorySession.global() : // boot loader never goes away The other built-in class loaders (`ClassLoaders::appClassLoader` and `ClassLoaders::platformClassLoader` are both instance of `BuiltinClassLoader`) will never be unloaded. Should they use the globel memory session? src/java.base/share/classes/jdk/internal/reflect/Reflection.java line 120: > 118: // if there is no caller class, act as if the call came from an > unnamed module > 119: Module module = currentClass != null ? > 120: currentClass.getModule() : Holder.FALLBACK_MODULE; This can be simplified to: Module module = currentClass != null ? currentClass.getModule() : ClassLoader::getSystemClassLoader().getUnnamedModule(); No need to have the Holder class. - PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v39]
> This PR contains the API and implementation changes for JEP-424 [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/424 Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: * Clarify what happens when SymbolLookup::loaderLookup is invoked from JNI * Tweak restricted method check to work when there's no caller class - Changes: - all: https://git.openjdk.java.net/jdk/pull/7888/files - new: https://git.openjdk.java.net/jdk/pull/7888/files/853f06b8..4d24ffa9 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=38 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=37-38 Stats: 22 lines in 2 files changed: 16 ins; 1 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v37]
On Thu, 5 May 2022 16:39:08 GMT, Mandy Chung wrote: >> `BootLoader` is what you want in this case - it defines the static methods >> to access resources, packages etc defined to the boot loader (aka null >> `ClassLoader`). >> >> To find a symbol defined in the native libraries loaded by the boot loader, >> you can call `BootLoader.getNativeLibraries().find(name)`. > > When a caller-sensitive method is invoked from a thread attached via JNI, the > caller class returned from `Reflection::getCallerClass` is `null`.I would > recommend the default to be a class in the unnamed module defined to the > system class loader; hence it defaults to the system class loader. This is > consistent with JNI `FindClass` when invoked with no caller frame. > > It's an open issue [1] to revisit the default for `System::load` and > `System::loadLibrary` when invoked with null caller class. However, the > existing behavior may likely be unchanged because of the compatibility risk. > > [1] https://bugs.openjdk.java.net/browse/JDK-8281001 Thanks for the comments. I've pushed a new change which fixes a couple of thing: * first, for `SymbolLookup.loaderLookup`, the system class loader is used when no caller class exists (e.g. when method is called from JNI). If caller class exist but its loader is null (boot loader), we just call ClassLoader::findNative with a `null` loader which will do the right thing (thanks @mlchung for the tips!) * second, the restricted method check in `Reflection::ensureNativeAccess` has been improved to also work in case where caller class is `null`. In such cases, a dummy unnamed module module is used for the check. - PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v37]
On Thu, 5 May 2022 16:22:41 GMT, Mandy Chung wrote: >> Looking deeper, `System::loadLibrary` seems to search the boot loader >> libraries when invoked with a `null` caller class, so replicating that >> behavior should be safe. > > `BootLoader` is what you want in this case - it defines the static methods to > access resources, packages etc defined to the boot loader (aka null > `ClassLoader`). > > To find a symbol defined in the native libraries loaded by the boot loader, > you can call `BootLoader.getNativeLibraries().find(name)`. When a caller-sensitive method is invoked from a thread attached via JNI, the caller class returned from `Reflection::getCallerClass` is `null`.I would recommend the default to be a class in the unnamed module defined to the system class loader; hence it defaults to the system class loader. This is consistent with JNI `FindClass` when invoked with no caller frame. It's an open issue [1] to revisit the default for `System::load` and `System::loadLibrary` when invoked with null caller class. However, the existing behavior may likely be unchanged because of the compatibility risk. [1] https://bugs.openjdk.java.net/browse/JDK-8281001 - PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v37]
On Wed, 4 May 2022 23:44:08 GMT, Maurizio Cimadamore wrote: >> Another option would be to treat calls to `ensureNativeAccess` with `null` >> caller class as coming from unnamed module. > > Looking deeper, `System::loadLibrary` seems to search the boot loader > libraries when invoked with a `null` caller class, so replicating that > behavior should be safe. `BootLoader` is what you want in this case - it defines the static methods to access resources, packages etc defined to the boot loader (aka null `ClassLoader`). To find a symbol defined in the native libraries loaded by the boot loader, you can call `BootLoader.getNativeLibraries().find(name)`. - PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v37]
On Wed, 4 May 2022 23:29:53 GMT, Maurizio Cimadamore wrote: >> Good points. Regarding `ClassLoader` being null, I think we can still return >> something using the `BootLoader`'s `NativeLibraries` object - that would >> allow this method to be called internally. @mlchung Can you please confirm? >> >> As for the caller sensitive having no real caller (e.g. because method is >> called directly from native code), I think we should add a check in >> `Reflection::ensureNativeAccess`. Few weeks ago I verified that adding such >> a check does not compromise upcall stub created via the Linker interface >> (e.g. a Java upcall might require to perform restricted operations - but >> those operation always happen inside the upcall MH, which is always >> associated with some class). > > Another option would be to treat calls to `ensureNativeAccess` with `null` > caller class as coming from unnamed module. Looking deeper, `System::loadLibrary` seems to search the boot loader libraries when invoked with a `null` caller class, so replicating that behavior should be safe. - PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v37]
On Wed, 4 May 2022 23:20:21 GMT, Maurizio Cimadamore
wrote:
>> src/java.base/share/classes/java/lang/foreign/SymbolLookup.java line 153:
>>
>>> 151: static SymbolLookup loaderLookup() {
>>> 152: Class caller = Reflection.getCallerClass();
>>> 153: ClassLoader loader =
>>> Objects.requireNonNull(caller.getClassLoader());
>>
>> Shouldn’t this be changed to throw `IllegalCallerException` instead of
>> throwing `NullPointerException` when the `caller`’s `ClassLoader` is
>> `null`[^1] or when `caller` itself is `null`[^2]?
>>
>> [^1]: This happens when `caller` is on the bootstrap classpath.
>> [^2]: This happens when `SymbolLookup.loaderLookup()` is called by native
>> code and no **Java** code is on the call stack.
>
> Good points. Regarding `ClassLoader` being null, I think we can still return
> something using the `BootLoader`'s `NativeLibraries` object - that would
> allow this method to be called internally. @mlchung Can you please confirm?
>
> As for the caller sensitive having no real caller (e.g. because method is
> called directly from native code), I think we should add a check in
> `Reflection::ensureNativeAccess`. Few weeks ago I verified that adding such a
> check does not compromise upcall stub created via the Linker interface (e.g.
> a Java upcall might require to perform restricted operations - but those
> operation always happen inside the upcall MH, which is always associated with
> some class).
Another option would be to treat calls to `ensureNativeAccess` with `null`
caller class as coming from unnamed module.
-
PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v38]
> This PR contains the API and implementation changes for JEP-424 [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/424 Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Tweak examples in SymbolLookup javadoc - Changes: - all: https://git.openjdk.java.net/jdk/pull/7888/files - new: https://git.openjdk.java.net/jdk/pull/7888/files/41d055ab..853f06b8 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=37 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=36-37 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v37]
On Wed, 4 May 2022 16:47:28 GMT, ExE Boss 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 57 commits:
>>
>> - Merge branch 'master' into foreign-preview
>> - Update
>> src/java.base/share/classes/jdk/internal/misc/X-ScopedMemoryAccess.java.template
>>
>>Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
>> - Tweak support for Linker lookup
>>Improve javadoc of SymbolLookup
>> - Tweak Addressable javadoc
>> - Downcall and upcall tests use wrong layout which is missing some trailing
>> padding
>> - Simplify libraryLookup impl
>> - Address CSR comments
>> - Merge branch 'master' into foreign-preview
>> - Add ValueLayout changes
>> - Tweak support for array element var handle
>> - ... and 47 more:
>> https://git.openjdk.java.net/jdk/compare/af1ee1cc...41d055ab
>
> src/java.base/share/classes/java/lang/foreign/SymbolLookup.java line 153:
>
>> 151: static SymbolLookup loaderLookup() {
>> 152: Class caller = Reflection.getCallerClass();
>> 153: ClassLoader loader =
>> Objects.requireNonNull(caller.getClassLoader());
>
> Shouldn’t this be changed to throw `IllegalCallerException` instead of
> throwing `NullPointerException` when the `caller`’s `ClassLoader` is
> `null`[^1] or when `caller` itself is `null`[^2]?
>
> [^1]: This happens when `caller` is on the bootstrap classpath.
> [^2]: This happens when `SymbolLookup.loaderLookup()` is called by native
> code and no **Java** code is on the call stack.
Good points. Regarding `ClassLoader` being null, I think we can still return
something using the `BootLoader`'s `NativeLibraries` object - that would allow
this method to be called internally. @mlchung Can you please confirm?
As for the caller sensitive having no real caller (e.g. because method is
called directly from native code), I think we should add a check in
`Reflection::ensureNativeAccess`. Few weeks ago I verified that adding such a
check does not compromise upcall stub created via the Linker interface (e.g. a
Java upcall might require to perform restricted operations - but those
operation always happen inside the upcall MH, which is always associated with
some class).
-
PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v37]
On Tue, 3 May 2022 10:40:02 GMT, Maurizio Cimadamore
wrote:
>> This PR contains the API and implementation changes for JEP-424 [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/424
>
> Maurizio Cimadamore has updated the pull request with a new target base due
> to a merge or a rebase. The pull request now contains 57 commits:
>
> - Merge branch 'master' into foreign-preview
> - Update
> src/java.base/share/classes/jdk/internal/misc/X-ScopedMemoryAccess.java.template
>
>Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
> - Tweak support for Linker lookup
>Improve javadoc of SymbolLookup
> - Tweak Addressable javadoc
> - Downcall and upcall tests use wrong layout which is missing some trailing
> padding
> - Simplify libraryLookup impl
> - Address CSR comments
> - Merge branch 'master' into foreign-preview
> - Add ValueLayout changes
> - Tweak support for array element var handle
> - ... and 47 more:
> https://git.openjdk.java.net/jdk/compare/af1ee1cc...41d055ab
src/java.base/share/classes/java/lang/foreign/SymbolLookup.java line 116:
> 114: * Linker nativeLinker = Linker.nativeLinker();
> 115: * SymbolLookup stdlib = nativeLinker.defaultLookup();
> 116: * MemorySegment malloc = stdlib.lookup("malloc");
This should be:
Suggestion:
* Optional malloc = stdlib.lookup("malloc");
or
Suggestion:
* MemorySegment malloc = stdlib.lookup("malloc").orElseThrow();
src/java.base/share/classes/java/lang/foreign/SymbolLookup.java line 153:
> 151: static SymbolLookup loaderLookup() {
> 152: Class caller = Reflection.getCallerClass();
> 153: ClassLoader loader =
> Objects.requireNonNull(caller.getClassLoader());
Shouldn’t this be changed to throw `IllegalCallerException` instead of throwing
`NullPointerException` when the `caller`’s `ClassLoader` is `null`[^1] or when
`caller` itself is `null`[^2]?
[^1]: This happens when `caller` is on the bootstrap classpath.
[^2]: This happens when `SymbolLookup.loaderLookup()` is called by native code
and no **Java** code is on the call stack.
-
PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v37]
> This PR contains the API and implementation changes for JEP-424 [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/424 Maurizio Cimadamore has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 57 commits: - Merge branch 'master' into foreign-preview - Update src/java.base/share/classes/jdk/internal/misc/X-ScopedMemoryAccess.java.template Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> - Tweak support for Linker lookup Improve javadoc of SymbolLookup - Tweak Addressable javadoc - Downcall and upcall tests use wrong layout which is missing some trailing padding - Simplify libraryLookup impl - Address CSR comments - Merge branch 'master' into foreign-preview - Add ValueLayout changes - Tweak support for array element var handle - ... and 47 more: https://git.openjdk.java.net/jdk/compare/af1ee1cc...41d055ab - Changes: https://git.openjdk.java.net/jdk/pull/7888/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=36 Stats: 65464 lines in 367 files changed: 43470 ins; 19432 del; 2562 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v35]
On Fri, 29 Apr 2022 08:15:32 GMT, Maurizio Cimadamore wrote: >> This PR contains the API and implementation changes for JEP-424 [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/424 > > Maurizio Cimadamore has updated the pull request incrementally with one > additional commit since the last revision: > > Tweak Addressable javadoc We've tweaked the support for looking up symbols in the standard libraries - `CLinker` used to implement `SymbolLookup`, now `CLinker` returns a "default lookup" instead. We've also greatly improved the javadoc of `SymbolLookup` - many thanks to Alex for the help! New javadoc here: http://cr.openjdk.java.net/~mcimadamore/8282191/v3/javadoc/java.base/module-summary.html - PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v36]
> This PR contains the API and implementation changes for JEP-424 [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/424 Maurizio Cimadamore has updated the pull request incrementally with two additional commits since the last revision: - Update src/java.base/share/classes/jdk/internal/misc/X-ScopedMemoryAccess.java.template Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> - Tweak support for Linker lookup Improve javadoc of SymbolLookup - Changes: - all: https://git.openjdk.java.net/jdk/pull/7888/files - new: https://git.openjdk.java.net/jdk/pull/7888/files/d1fcf012..dc309e8b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=35 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=34-35 Stats: 159 lines in 14 files changed: 96 ins; 8 del; 55 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v35]
On Fri, 29 Apr 2022 08:15:32 GMT, Maurizio Cimadamore
wrote:
>> This PR contains the API and implementation changes for JEP-424 [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/424
>
> Maurizio Cimadamore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Tweak Addressable javadoc
src/java.base/share/classes/jdk/internal/misc/X-ScopedMemoryAccess.java.template
line 101:
> 99: }
> 100:
> 101: public final static class ScopedAccessError extends Error {
This should probably use the canonical modifier order as specified in
[JDK‑8276348] ([GH‑6213]):
Suggestion:
public static final class ScopedAccessError extends Error {
[JDK‑8276348]: https://bugs.openjdk.java.net/browse/JDK-8276348 "[JDK‑8276348]
Use blessed modifier order in java.base"
[GH‑6213]: https://github.com/openjdk/jdk/pull/6213
-
PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v34]
On Fri, 29 Apr 2022 08:06:24 GMT, Maurizio Cimadamore wrote: > would a jep unneeded be enough to "unstuck" this PR? Yes if no bug. Conceptually, the `/jep unneeded` will behave as no jep command. - PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v35]
> This PR contains the API and implementation changes for JEP-424 [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/424 Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Tweak Addressable javadoc - Changes: - all: https://git.openjdk.java.net/jdk/pull/7888/files - new: https://git.openjdk.java.net/jdk/pull/7888/files/945317ec..d1fcf012 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=34 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=33-34 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v34]
On Fri, 29 Apr 2022 00:44:01 GMT, Guoxiong Li wrote: > Remind: please use the command `/jep JEP-424` [1] to mark this PR. > > [1] > https://wiki.openjdk.java.net/display/SKARA/Pull+Request+Commands#PullRequestCommands-/jep Question: I'm willing to try it out. If something goes wrong - would a `jep unneeded` be enough to "unstuck" this PR? - PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v34]
On Thu, 28 Apr 2022 18:10:57 GMT, Maurizio Cimadamore wrote: >> This PR contains the API and implementation changes for JEP-424 [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/424 > > Maurizio Cimadamore has updated the pull request incrementally with one > additional commit since the last revision: > > Downcall and upcall tests use wrong layout which is missing some trailing > padding Remind: please use the command `/jep JEP-424` [1] to mark this PR. [1] https://wiki.openjdk.java.net/display/SKARA/Pull+Request+Commands#PullRequestCommands-/jep - PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v34]
> This PR contains the API and implementation changes for JEP-424 [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/424 Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Downcall and upcall tests use wrong layout which is missing some trailing padding - Changes: - all: https://git.openjdk.java.net/jdk/pull/7888/files - new: https://git.openjdk.java.net/jdk/pull/7888/files/5866bbb5..945317ec Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=33 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=32-33 Stats: 9 lines in 2 files changed: 7 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v33]
> This PR contains the API and implementation changes for JEP-424 [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/424 Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Simplify libraryLookup impl - Changes: - all: https://git.openjdk.java.net/jdk/pull/7888/files - new: https://git.openjdk.java.net/jdk/pull/7888/files/6990183f..5866bbb5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=32 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=31-32 Stats: 21 lines in 1 file changed: 6 ins; 11 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v32]
> This PR contains the API and implementation changes for JEP-424 [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/424 Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Address CSR comments - Changes: - all: https://git.openjdk.java.net/jdk/pull/7888/files - new: https://git.openjdk.java.net/jdk/pull/7888/files/746de5ce..6990183f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=31 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=30-31 Stats: 114 lines in 9 files changed: 36 ins; 0 del; 78 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v31]
> This PR contains the API and implementation changes for JEP-424 [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/424 Maurizio Cimadamore has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 50 commits: - Merge branch 'master' into foreign-preview - Add ValueLayout changes - Tweak support for array element var handle - Add @see - Initial push - Remove whitespaces - Drop `UnsupportedOperationException` from throws clause in FileChannel/FileChannelImpl - Add @ForceInline annotation on session accessor Beef up UnrolledAccess benchmark - Use a less expensive array element alignment check The bit to byte conversion shows up in hot paths - Fix UnrolledAccess benchmark - ... and 40 more: https://git.openjdk.java.net/jdk/compare/72f82dd7...746de5ce - Changes: https://git.openjdk.java.net/jdk/pull/7888/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=30 Stats: 65314 lines in 367 files changed: 43344 ins; 19432 del; 2538 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v30]
> This PR contains the API and implementation changes for JEP-424 [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/424 Maurizio Cimadamore has updated the pull request incrementally with four additional commits since the last revision: - Add ValueLayout changes - Tweak support for array element var handle - Add @see - Initial push - Changes: - all: https://git.openjdk.java.net/jdk/pull/7888/files - new: https://git.openjdk.java.net/jdk/pull/7888/files/8637379e..2e3d57a8 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=29 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=28-29 Stats: 129 lines in 4 files changed: 97 ins; 25 del; 7 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v29]
> This PR contains the API and implementation changes for JEP-424 [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/424 Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Remove whitespaces - Changes: - all: https://git.openjdk.java.net/jdk/pull/7888/files - new: https://git.openjdk.java.net/jdk/pull/7888/files/3a87df5e..8637379e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=28 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=27-28 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v28]
> This PR contains the API and implementation changes for JEP-424 [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/424 Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Drop `UnsupportedOperationException` from throws clause in FileChannel/FileChannelImpl - Changes: - all: https://git.openjdk.java.net/jdk/pull/7888/files - new: https://git.openjdk.java.net/jdk/pull/7888/files/66cebe77..3a87df5e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=27 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=26-27 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v27]
On Wed, 13 Apr 2022 16:12:31 GMT, Alan Bateman wrote: >> Maurizio Cimadamore has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Add @ForceInline annotation on session accessor >> Beef up UnrolledAccess benchmark > > src/java.base/share/classes/java/nio/channels/FileChannel.java line 1052: > >> 1050: public MemorySegment map(MapMode mode, long offset, long size, >> 1051: MemorySession session) >> 1052: throws IOException, UnsupportedOperationException > > Just a minor here is that UOE is a runtime exception so probably shouldn't be > in the throws. whoops - good catch - will fix! - PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v27]
On Tue, 12 Apr 2022 10:24:47 GMT, Maurizio Cimadamore wrote: >> This PR contains the API and implementation changes for JEP-424 [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/424 > > Maurizio Cimadamore has updated the pull request incrementally with one > additional commit since the last revision: > > Add @ForceInline annotation on session accessor > Beef up UnrolledAccess benchmark src/java.base/share/classes/java/nio/channels/FileChannel.java line 1052: > 1050: public MemorySegment map(MapMode mode, long offset, long size, > 1051: MemorySession session) > 1052: throws IOException, UnsupportedOperationException Just a minor here is that UOE is a runtime exception so probably shouldn't be in the throws. - PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v27]
> This PR contains the API and implementation changes for JEP-424 [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/424 Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Add @ForceInline annotation on session accessor Beef up UnrolledAccess benchmark - Changes: - all: https://git.openjdk.java.net/jdk/pull/7888/files - new: https://git.openjdk.java.net/jdk/pull/7888/files/a68195ae..66cebe77 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=26 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=25-26 Stats: 32 lines in 2 files changed: 32 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v24]
On Mon, 11 Apr 2022 12:04:41 GMT, Maurizio Cimadamore wrote: >> src/hotspot/share/prims/scopedMemoryAccess.cpp line 141: >> >>> 139: >>> 140: /* >>> 141: * This function performs a thread-local handshake against all threads >>> running at the time >> >> Nit: thread-local?? > > I was assuming the term "thread-local handshake" was a thing in the VM, as > per: > https://openjdk.java.net/jeps/312 Ha! Mea culpa. I've been too buried in the other kind of thread-local lately. I completely forgot we actually described handshakes that way. :) - PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v26]
> This PR contains the API and implementation changes for JEP-424 [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/424 Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Use a less expensive array element alignment check The bit to byte conversion shows up in hot paths - Changes: - all: https://git.openjdk.java.net/jdk/pull/7888/files - new: https://git.openjdk.java.net/jdk/pull/7888/files/c9afcd17..a68195ae Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=25 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=24-25 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v25]
> This PR contains the API and implementation changes for JEP-424 [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/424 Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Fix UnrolledAccess benchmark - Changes: - all: https://git.openjdk.java.net/jdk/pull/7888/files - new: https://git.openjdk.java.net/jdk/pull/7888/files/d84de510..c9afcd17 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=24 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=23-24 Stats: 4 lines in 1 file changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v24]
On Mon, 11 Apr 2022 10:33:54 GMT, David Holmes wrote: >> Maurizio Cimadamore has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Fix TestLinkToNativeRBP > > src/hotspot/share/prims/scopedMemoryAccess.cpp line 141: > >> 139: >> 140: /* >> 141: * This function performs a thread-local handshake against all threads >> running at the time > > Nit: thread-local?? I was assuming the term "thread-local handshake" was a thing in the VM, as per: https://openjdk.java.net/jeps/312 - PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v24]
On Mon, 4 Apr 2022 14:57:30 GMT, Maurizio Cimadamore wrote: >> This PR contains the API and implementation changes for JEP-424 [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/424 > > Maurizio Cimadamore has updated the pull request incrementally with one > additional commit since the last revision: > > Fix TestLinkToNativeRBP VM changes look good. Thanks, David src/hotspot/share/prims/scopedMemoryAccess.cpp line 141: > 139: > 140: /* > 141: * This function performs a thread-local handshake against all threads > running at the time Nit: thread-local?? - Marked as reviewed by dholmes (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v16]
On Wed, 30 Mar 2022 21:51:16 GMT, Maurizio Cimadamore wrote: >> This PR contains the API and implementation changes for JEP-424 [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/424 > > 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 foreign-preview > - Tweak FunctionDescriptor::argumentLayouts to return an immutable list > - Fix bad usage of `@link` with primitive array types > - Switch to daemon threads for async upcalls > - Use thread local storage to optimize attach of async threads > - Drop support for Constable from MemoryLayout/FunctionDescriptor > - Merge branch 'master' into foreign-preview > - Revert changes to RunTests.gmk > - Add --enable-preview to micro benchmark java options > - Address more review comments > - ... and 21 more: > https://git.openjdk.java.net/jdk/compare/ce27d9dd...247e5eb5 Java code looks good (i did not go through the tests). As is common no comments, since code was reviewed in smaller steps in the panama-foreign respository. - Marked as reviewed by psandoz (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v15]
On Wed, 30 Mar 2022 20:59:34 GMT, Maurizio Cimadamore
wrote:
>> This PR contains the API and implementation changes for JEP-424 [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/424
>
> Maurizio Cimadamore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Tweak FunctionDescriptor::argumentLayouts to return an immutable list
src/java.base/share/classes/java/lang/foreign/FunctionDescriptor.java line 73:
> 71: */
> 72: public List argumentLayouts() {
> 73: return Collections.unmodifiableList(argLayouts);
This change doesn’t seem to be necessary, as `FunctionDescriptor` is already
created with `List.of(…)` (or `Stream.toList()` in the case of
`FunctionDescriptor.VariadicFunction`), which produce immutable lists (although
`Stream.toList()` permits `null`s, which
`Stream.collect(Collectors.toImmutableList())` and `List.of(…)` doesn’t).
-
PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v16]
> This PR contains the API and implementation changes for JEP-424 [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/424 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 foreign-preview - Tweak FunctionDescriptor::argumentLayouts to return an immutable list - Fix bad usage of `@link` with primitive array types - Switch to daemon threads for async upcalls - Use thread local storage to optimize attach of async threads - Drop support for Constable from MemoryLayout/FunctionDescriptor - Merge branch 'master' into foreign-preview - Revert changes to RunTests.gmk - Add --enable-preview to micro benchmark java options - Address more review comments - ... and 21 more: https://git.openjdk.java.net/jdk/compare/ce27d9dd...247e5eb5 - Changes: https://git.openjdk.java.net/jdk/pull/7888/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=15 Stats: 64862 lines in 366 files changed: 43028 ins; 19321 del; 2513 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v15]
> This PR contains the API and implementation changes for JEP-424 [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/424 Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Tweak FunctionDescriptor::argumentLayouts to return an immutable list - Changes: - all: https://git.openjdk.java.net/jdk/pull/7888/files - new: https://git.openjdk.java.net/jdk/pull/7888/files/0bcc8664..af41a76c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=14 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=13-14 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v14]
> This PR contains the API and implementation changes for JEP-424 [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/424 Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Fix bad usage of `@link` with primitive array types - Changes: - all: https://git.openjdk.java.net/jdk/pull/7888/files - new: https://git.openjdk.java.net/jdk/pull/7888/files/43dc6be3..0bcc8664 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=13 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=12-13 Stats: 7 lines in 1 file changed: 0 ins; 0 del; 7 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v13]
> This PR contains the API and implementation changes for JEP-424 [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/424 Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Switch to daemon threads for async upcalls - Changes: - all: https://git.openjdk.java.net/jdk/pull/7888/files - new: https://git.openjdk.java.net/jdk/pull/7888/files/55aee872..43dc6be3 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=12 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=11-12 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v12]
> This PR contains the API and implementation changes for JEP-424 [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/424 Maurizio Cimadamore has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 27 additional commits since the last revision: - Use thread local storage to optimize attach of async threads - Drop support for Constable from MemoryLayout/FunctionDescriptor - Merge branch 'master' into foreign-preview - Revert changes to RunTests.gmk - Add --enable-preview to micro benchmark java options - Address more review comments - Update src/java.base/share/classes/java/lang/foreign/SymbolLookup.java Co-authored-by: Jorn Vernee - Update src/java.base/share/classes/java/lang/foreign/SymbolLookup.java Co-authored-by: Jorn Vernee - Address review comments - Update src/java.base/share/classes/java/lang/foreign/MemorySegment.java Co-authored-by: Jorn Vernee - ... and 17 more: https://git.openjdk.java.net/jdk/compare/02333d66...55aee872 - Changes: - all: https://git.openjdk.java.net/jdk/pull/7888/files - new: https://git.openjdk.java.net/jdk/pull/7888/files/504b564a..55aee872 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=11 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=10-11 Stats: 99257 lines in 1550 files changed: 79659 ins; 15544 del; 4054 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v5]
On Thu, 24 Mar 2022 18:35:12 GMT, Jorn Vernee wrote: >> Sure, this is problematic - but at the same time I don't think there's a >> better way to deal with this? I'd prefer to defer this to a separate issue >> (and I think the build team is in a much better position to suggest a better >> fix). IIRC we had this problem in the past as well. > > I'd suggest at least adding `--enable-preview` as an argument when running > benchmarks through the build system in that case. I think this should do the > trick: > > > diff --git a/make/RunTests.gmk b/make/RunTests.gmk > index 81540266ec0..9ed45fb02a8 100644 > --- a/make/RunTests.gmk > +++ b/make/RunTests.gmk > @@ -583,7 +583,7 @@ define SetupRunMicroTestBody >$$(eval $$(call SetMicroValue,$1,MICRO_JAVA_OPTIONS)) > ># Current tests needs to open java.io > - $1_MICRO_JAVA_OPTIONS += --add-opens=java.base/java.io=ALL-UNNAMED > + $1_MICRO_JAVA_OPTIONS += --add-opens=java.base/java.io=ALL-UNNAMED > --enable-preview > ># Save output as JSON or CSV file >ifneq ($$(MICRO_RESULTS_FORMAT), ) > > > People manually running the benchmarks.jar will have to pass > `--enable-preview` still though. After discussing this offline, it seems that javac no longer poisons the minor class file version of every class file, but only of those that use preview features. So, my concern is not warranted. - PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v5]
On Thu, 24 Mar 2022 19:17:40 GMT, Jorn Vernee wrote: >> I'd suggest at least adding `--enable-preview` as an argument when running >> benchmarks through the build system in that case. I think this should do the >> trick: >> >> >> diff --git a/make/RunTests.gmk b/make/RunTests.gmk >> index 81540266ec0..9ed45fb02a8 100644 >> --- a/make/RunTests.gmk >> +++ b/make/RunTests.gmk >> @@ -583,7 +583,7 @@ define SetupRunMicroTestBody >>$$(eval $$(call SetMicroValue,$1,MICRO_JAVA_OPTIONS)) >> >># Current tests needs to open java.io >> - $1_MICRO_JAVA_OPTIONS += --add-opens=java.base/java.io=ALL-UNNAMED >> + $1_MICRO_JAVA_OPTIONS += --add-opens=java.base/java.io=ALL-UNNAMED >> --enable-preview >> >># Save output as JSON or CSV file >>ifneq ($$(MICRO_RESULTS_FORMAT), ) >> >> >> People manually running the benchmarks.jar will have to pass >> `--enable-preview` still though. > > After discussing this offline, it seems that javac no longer poisons the > minor class file version of every class file, but only of those that use > preview features. So, my concern is not warranted. Turns out this is no longer necessary. As part of the support for preview API, javac now only pollutes the classfile if a source file is using preview features, as described in this PR: https://github.com/openjdk/jdk/pull/703 - PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v11]
On Thu, 24 Mar 2022 19:19:34 GMT, Maurizio Cimadamore wrote: >> This PR contains the API and implementation changes for JEP-424 [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/424 > > Maurizio Cimadamore has updated the pull request incrementally with one > additional commit since the last revision: > > Revert changes to RunTests.gmk Looks Good! - Marked as reviewed by jvernee (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v11]
> This PR contains the API and implementation changes for JEP-424 [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/424 Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Revert changes to RunTests.gmk - Changes: - all: https://git.openjdk.java.net/jdk/pull/7888/files - new: https://git.openjdk.java.net/jdk/pull/7888/files/6e7189b4..504b564a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=10 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=09-10 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v10]
> This PR contains the API and implementation changes for JEP-424 [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/424 Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Add --enable-preview to micro benchmark java options - Changes: - all: https://git.openjdk.java.net/jdk/pull/7888/files - new: https://git.openjdk.java.net/jdk/pull/7888/files/d95c6d0f..6e7189b4 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=09 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=08-09 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v9]
> This PR contains the API and implementation changes for JEP-424 [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/424 Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Address more review comments - Changes: - all: https://git.openjdk.java.net/jdk/pull/7888/files - new: https://git.openjdk.java.net/jdk/pull/7888/files/6881b6dc..d95c6d0f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=08 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=07-08 Stats: 2 lines in 1 file changed: 2 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v8]
> This PR contains the API and implementation changes for JEP-424 [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/424 Maurizio Cimadamore has updated the pull request incrementally with two additional commits since the last revision: - Update src/java.base/share/classes/java/lang/foreign/SymbolLookup.java Co-authored-by: Jorn Vernee - Update src/java.base/share/classes/java/lang/foreign/SymbolLookup.java Co-authored-by: Jorn Vernee - Changes: - all: https://git.openjdk.java.net/jdk/pull/7888/files - new: https://git.openjdk.java.net/jdk/pull/7888/files/3e8cfd74..6881b6dc Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=07 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=06-07 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v5]
On Thu, 24 Mar 2022 17:48:23 GMT, Maurizio Cimadamore wrote: >> make/test/BuildMicrobenchmark.gmk line 97: >> >>> 95: SRC := $(MICROBENCHMARK_SRC), \ >>> 96: BIN := $(MICROBENCHMARK_CLASSES), \ >>> 97: JAVAC_FLAGS := --add-exports >>> java.base/sun.security.util=ALL-UNNAMED --enable-preview, \ >> >> It still seems like this would lead to potential issues. i.e. requiring all >> benchmarks to be run with `--enable-preview`? We ended up adding >> `--enable-preview` to our benchmarks, but do other benchmarks still work >> without it? AFAIK the entire benchmarks.jar will have the altered class file >> version. > > Sure, this is problematic - but at the same time I don't think there's a > better way to deal with this? I'd prefer to defer this to a separate issue > (and I think the build team is in a much better position to suggest a better > fix). IIRC we had this problem in the past as well. I'd suggest at least adding `--enable-preview` as an argument when running benchmarks through the build system in that case. I think this should do the trick: diff --git a/make/RunTests.gmk b/make/RunTests.gmk index 81540266ec0..9ed45fb02a8 100644 --- a/make/RunTests.gmk +++ b/make/RunTests.gmk @@ -583,7 +583,7 @@ define SetupRunMicroTestBody $$(eval $$(call SetMicroValue,$1,MICRO_JAVA_OPTIONS)) # Current tests needs to open java.io - $1_MICRO_JAVA_OPTIONS += --add-opens=java.base/java.io=ALL-UNNAMED + $1_MICRO_JAVA_OPTIONS += --add-opens=java.base/java.io=ALL-UNNAMED --enable-preview # Save output as JSON or CSV file ifneq ($$(MICRO_RESULTS_FORMAT), ) People manually running the benchmarks.jar will have to pass `--enable-preview` still though. - PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v5]
On Thu, 24 Mar 2022 13:00:12 GMT, Jorn Vernee wrote:
>> Maurizio Cimadamore has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Drop redundant javadoc statements re. handling of nulls
>> (handling of nulls is specified once and for all in the package javadoc)
>
> src/java.base/share/classes/sun/nio/ch/FileChannelImpl.java line 1071:
>
>> 1069: sessionImpl.checkValidStateSlow();
>> 1070: if (offset < 0) throw new IllegalArgumentException("Requested
>> bytes offset must be >= 0.");
>> 1071: if (size < 0) throw new IllegalArgumentException("Requested
>> bytes size must be >= 0.");
>
> The javadoc also says that IAE will be thrown if `offset + size < 0` I think
> to guard against overflow, but I don't see that checked here. Is it missing?
`mapInternal` in FileChannelImpl takes care of that for both flavors of `map`
-
PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v7]
> This PR contains the API and implementation changes for JEP-424 [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/424 Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Address review comments - Changes: - all: https://git.openjdk.java.net/jdk/pull/7888/files - new: https://git.openjdk.java.net/jdk/pull/7888/files/95f65eea..3e8cfd74 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=05-06 Stats: 16 lines in 3 files changed: 2 ins; 0 del; 14 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v5]
On Thu, 24 Mar 2022 13:10:20 GMT, Jorn Vernee wrote: >> Maurizio Cimadamore has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Drop redundant javadoc statements re. handling of nulls >> (handling of nulls is specified once and for all in the package javadoc) > > make/test/BuildMicrobenchmark.gmk line 97: > >> 95: SRC := $(MICROBENCHMARK_SRC), \ >> 96: BIN := $(MICROBENCHMARK_CLASSES), \ >> 97: JAVAC_FLAGS := --add-exports java.base/sun.security.util=ALL-UNNAMED >> --enable-preview, \ > > It still seems like this would lead to potential issues. i.e. requiring all > benchmarks to be run with `--enable-preview`? We ended up adding > `--enable-preview` to our benchmarks, but do other benchmarks still work > without it? AFAIK the entire benchmarks.jar will have the altered class file > version. Sure, this is problematic - but at the same time I don't think there's a better way to deal with this? I'd prefer to defer this to a separate issue (and I think the build team is in a much better position to suggest a better fix). IIRC we had this problem in the past as well. - PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v6]
> This PR contains the API and implementation changes for JEP-424 [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/424 Maurizio Cimadamore has updated the pull request incrementally with three additional commits since the last revision: - Update src/java.base/share/classes/java/lang/foreign/MemorySegment.java Co-authored-by: Jorn Vernee - Update src/java.base/share/classes/java/lang/foreign/MemorySegment.java Co-authored-by: Jorn Vernee - Update src/java.base/share/classes/java/lang/foreign/MemorySegment.java Co-authored-by: Jorn Vernee - Changes: - all: https://git.openjdk.java.net/jdk/pull/7888/files - new: https://git.openjdk.java.net/jdk/pull/7888/files/c9bc9a70..95f65eea Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=04-05 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v5]
On Wed, 23 Mar 2022 14:06:56 GMT, Maurizio Cimadamore
wrote:
>> This PR contains the API and implementation changes for JEP-424 [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/424
>
> Maurizio Cimadamore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Drop redundant javadoc statements re. handling of nulls
> (handling of nulls is specified once and for all in the package javadoc)
Some more nits.
One potential issue with adding --enable-preview when building benchmarks (last
comment of the bunch).
Other than that, I think this looks good.
make/test/BuildMicrobenchmark.gmk line 97:
> 95: SRC := $(MICROBENCHMARK_SRC), \
> 96: BIN := $(MICROBENCHMARK_CLASSES), \
> 97: JAVAC_FLAGS := --add-exports java.base/sun.security.util=ALL-UNNAMED
> --enable-preview, \
It still seems like this would lead to potential issues. i.e. requiring all
benchmarks to be run with `--enable-preview`? We ended up adding
`--enable-preview` to our benchmarks, but do other benchmarks still work
without it? AFAIK the entire benchmarks.jar will have the altered class file
version.
src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 61:
> 59: * {@linkplain MemorySegment#allocateNative(long, long,
> MemorySession) native memory segments}, backed by off-heap memory;
> 60: * {@linkplain FileChannel#map(FileChannel.MapMode, long, long,
> MemorySession) mapped memory segments}, obtained by mapping
> 61: * a file into main memory ({@code mmap}); tha contents of a mapped
> memory segments can be {@linkplain #force() persisted} and
Suggestion:
* a file into main memory ({@code mmap}); the contents of a mapped memory
segments can be {@linkplain #force() persisted} and
src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 298:
> 296:
> 297: /**
> 298: * Returns a slice of this memory segment, at given offset. The
> returned segment's base address is the base address
Saw a similar change in other places, so I'll suggest this here as well.
Suggestion:
* Returns a slice of this memory segment, at the given offset. The
returned segment's base address is the base address
src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 311:
> 309:
> 310: /**
> 311: * Returns a slice of this memory segment, at given offset. The
> returned segment's base address is the base address
Suggestion:
* Returns a slice of this memory segment, at the given offset. The
returned segment's base address is the base address
src/java.base/share/classes/java/lang/foreign/MemorySession.java line 143:
> 141:
> 142: /**
> 143: * {@return the owner thread associated with this memory session (if
> any)}
Maybe the "if any" here could be more specific. e.g. saying that `null` is
returned if the session doesn't have an owner thread.
src/java.base/share/classes/java/lang/foreign/MemorySession.java line 165:
> 163:
> 164: /**
> 165: * Closes this memory session. As a side effect, if this operation
> completes without exceptions, this session
I'd suggest change this to "As a result of this", since the effects listed are
the main reason for closing a session. (it strikes me as strange. If the things
listed are side-effects, then what is the main effect of closing a segment?)
Suggestion:
* Closes this memory session. As a result of this, if this operation
completes without exceptions, this session
src/java.base/share/classes/java/lang/foreign/SymbolLookup.java line 51:
> 49: *
> 50: * Clients can obtain a {@linkplain #loaderLookup() loader lookup},
> 51: * which can be used to search symbols in libraries loaded by the current
> classloader (e.g. using {@link System#load(String)},
"search symbols" sounds a bit unnatural to me... I like the wording in the
libraryLookup doc more
Suggestion:
* which can be used to find symbols in libraries loaded by the current
classloader (e.g. using {@link System#load(String)},
src/java.base/share/classes/java/lang/foreign/SymbolLookup.java line 59:
> 57: *
> 58: * Finally, clients can load a library and obtain a {@linkplain
> #libraryLookup(Path, MemorySession) library lookup} which can be used
> 59: * to search symbols in that library. A library lookup is associated with
> a {@linkplain MemorySession memory session},
Suggestion:
* to find symbols in that library. A library lookup is associated with a
{@linkplain MemorySession memory session},
src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 7895:
> 7893: * VarHandle handle =
> MethodHandles.memorySegmentViewVarHandle(ValueLayout.JAVA_INT.withOrder(ByteOrder.BIG_ENDIAN));
> //(MemorySegment, long) -> int
> 7894: * handle = MethodHandles.insertCoordinates(handle, 1, 4);
> //(MemoryS
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v5]
> This PR contains the API and implementation changes for JEP-424 [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/424 Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Drop redundant javadoc statements re. handling of nulls (handling of nulls is specified once and for all in the package javadoc) - Changes: - all: https://git.openjdk.java.net/jdk/pull/7888/files - new: https://git.openjdk.java.net/jdk/pull/7888/files/7ec71f73..c9bc9a70 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=03-04 Stats: 12 lines in 2 files changed: 3 ins; 8 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v4]
On Tue, 22 Mar 2022 19:07:12 GMT, Maurizio Cimadamore wrote: >> This PR contains the API and implementation changes for JEP-424 [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/424 > > Maurizio Cimadamore has updated the pull request incrementally with one > additional commit since the last revision: > > Fix indentation in Lib.gmk Marked as reviewed by erikj (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v4]
> This PR contains the API and implementation changes for JEP-424 [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/424 Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Fix indentation in Lib.gmk - Changes: - all: https://git.openjdk.java.net/jdk/pull/7888/files - new: https://git.openjdk.java.net/jdk/pull/7888/files/4b2760d3..7ec71f73 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=02-03 Stats: 7 lines in 1 file changed: 0 ins; 0 del; 7 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v3]
On Tue, 22 Mar 2022 14:04:07 GMT, Maurizio Cimadamore wrote: >> This PR contains the API and implementation changes for JEP-424 [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/424 > > Maurizio Cimadamore has updated the pull request incrementally with three > additional commits since the last revision: > > - rename syslookup lib on Windows > - Add missing LIBS flag > - Simplify syslookup build changes make/modules/java.base/Lib.gmk line 217: > 215: LDFLAGS_linux := -Wl$(COMMA)--no-as-needed, \ > 216: LIBS := $(LIBCXX), \ > 217: LIBS_linux := -lc -lm -ldl, \ This looks much better, thanks! Now if you could just fix the indentation of the parameters to 4 spaces, it would be perfect. - PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v3]
On Mon, 21 Mar 2022 17:36:53 GMT, Maurizio Cimadamore wrote: >> make/modules/java.base/Lib.gmk line 217: >> >>> 215: CXXFLAGS := $(CXXFLAGS_JDKLIB), \ >>> 216: LDFLAGS := $(LDFLAGS_JDKLIB) -Wl$(COMMA)--no-as-needed, \ >>> 217: LIBS := $(LIBCXX) -lc -lm -ldl, \ >> >> Instead of repeating this whole macro call for both Linux and non Linux, you >> can use parameters of the form LDFLAGS_linux and LIBS_linux to add the Linux >> specific flags. Something like this: >> >> >> LDFLAGS := $(LDFLAGS_JDKLIB), \ >> LDFLAGS_linux := -Wl$(COMMA)--no-as-needed, \ >> >> >> For the NAME field, there is no such support, so the way we usually do that >> is through a variable and conditionals before the macro call. What's the >> reason to have a different lib name on Windows? If they were the same, and >> the source file in windows/native/... had the same name, it would just >> automatically override in the build. >> >> I realize now that this is just moved code from jdk.incubator.foreign, and >> this patch is probably big enough as it is so no need to refactor the build >> logic at the same time. > > Good points - there is really no need AFAIK for the lib name to be different. > I'll do few experiments. I've fixed the makefile as you suggested - I agree the result is much simpler. I've tested the changes on mac/linux/win and everything looks good. - PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v3]
> This PR contains the API and implementation changes for JEP-424 [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/424 Maurizio Cimadamore has updated the pull request incrementally with three additional commits since the last revision: - rename syslookup lib on Windows - Add missing LIBS flag - Simplify syslookup build changes - Changes: - all: https://git.openjdk.java.net/jdk/pull/7888/files - new: https://git.openjdk.java.net/jdk/pull/7888/files/6bb1b5c9..4b2760d3 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=01-02 Stats: 28 lines in 3 files changed: 1 ins; 23 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v2]
> This PR contains the API and implementation changes for JEP-424 [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/424 Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Address review comments * Use `new` instead of `fresh` * Drop use of `new` where caching might be used * Remove unused imports * Add static imports to make code more succint * Fix other typos - Changes: - all: https://git.openjdk.java.net/jdk/pull/7888/files - new: https://git.openjdk.java.net/jdk/pull/7888/files/8e6017dc..6bb1b5c9 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=00-01 Stats: 83 lines in 10 files changed: 2 ins; 7 del; 74 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview)
On Mon, 21 Mar 2022 14:17:21 GMT, Jorn Vernee wrote: >> This PR contains the API and implementation changes for JEP-424 [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/424 > > src/java.base/share/classes/jdk/internal/foreign/abi/x64/sysv/CallArranger.java > line 53: > >> (failed to retrieve contents of file, check the PR for context) > Same here, I think keeping the static import for this would make things more > readable. Good catch. I think the IDE did that when I moved files across; I've fixed few of these, but there's more it seems. - PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview)
On Mon, 21 Mar 2022 10:45:27 GMT, Maurizio Cimadamore
wrote:
> This PR contains the API and implementation changes for JEP-424 [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/424
src/java.base/share/classes/java/lang/foreign/CLinker.java line 176:
> 174: * @param symbol the address of the target function.
> 175: * @param function the function descriptor of the target function.
> 176: * @return a new downcall method handle. The method handle type is
> inferred
Maybe drop the "new" from this. Since we want to do caching in the future.
Suggestion:
* @return a downcall method handle. The method handle type is inferred
src/java.base/share/classes/java/lang/foreign/CLinker.java line 199:
> 197: *
> 198: * @param function the function descriptor of the target function.
> 199: * @return a new downcall method handle. The method handle type is
> inferred
Suggestion:
* @return a downcall method handle. The method handle type is inferred
src/java.base/share/classes/java/lang/foreign/MemoryAddress.java line 159:
> 157: * Creates a memory address from the given long value.
> 158: * @param value the long value representing a raw address.
> 159: * @return a new memory address instance.
Similar here. A new address is not always returned.
Suggestion:
* @return a memory address instance.
src/java.base/share/classes/java/lang/foreign/package-info.java line 230:
> 228: * where {@code M1}, {@code M2}, {@code ... Mn} are module names (for
> the unnamed module, the special value {@code ALL-UNNAMED}
> 229: * can be used). If this option is specified, access to restricted
> methods is only granted to the modules listed by that
> 230: * option. If this option is not specified, access to restricted method
> is enabled for all modules, but
Suggestion:
* option. If this option is not specified, access to restricted methods is
enabled for all modules, but
src/java.base/share/classes/jdk/internal/foreign/abi/aarch64/CallArranger.java
line 53:
> (failed to retrieve contents of file, check the PR for context)
Keeping this static import would seem more readable here, instead of prefixing
everything with `AArch64Architecture.`. (especially in the ABI definition below)
src/java.base/share/classes/jdk/internal/foreign/abi/x64/sysv/CallArranger.java
line 53:
> (failed to retrieve contents of file, check the PR for context)
Same here, I think keeping the static import for this would make things more
readable.
-
PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview)
On Mon, 21 Mar 2022 17:16:49 GMT, Erik Joelsson wrote: >> This PR contains the API and implementation changes for JEP-424 [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/424 > > make/modules/java.base/Lib.gmk line 217: > >> 215: CXXFLAGS := $(CXXFLAGS_JDKLIB), \ >> 216: LDFLAGS := $(LDFLAGS_JDKLIB) -Wl$(COMMA)--no-as-needed, \ >> 217: LIBS := $(LIBCXX) -lc -lm -ldl, \ > > Instead of repeating this whole macro call for both Linux and non Linux, you > can use parameters of the form LDFLAGS_linux and LIBS_linux to add the Linux > specific flags. Something like this: > > > LDFLAGS := $(LDFLAGS_JDKLIB), \ > LDFLAGS_linux := -Wl$(COMMA)--no-as-needed, \ > > > For the NAME field, there is no such support, so the way we usually do that > is through a variable and conditionals before the macro call. What's the > reason to have a different lib name on Windows? If they were the same, and > the source file in windows/native/... had the same name, it would just > automatically override in the build. > > I realize now that this is just moved code from jdk.incubator.foreign, and > this patch is probably big enough as it is so no need to refactor the build > logic at the same time. Good points - there is really no need AFAIK for the lib name to be different. I'll do few experiments. - PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview)
On Mon, 21 Mar 2022 10:45:27 GMT, Maurizio Cimadamore wrote: > This PR contains the API and implementation changes for JEP-424 [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/424 Build changes look ok. make/modules/java.base/Lib.gmk line 217: > 215: CXXFLAGS := $(CXXFLAGS_JDKLIB), \ > 216: LDFLAGS := $(LDFLAGS_JDKLIB) -Wl$(COMMA)--no-as-needed, \ > 217: LIBS := $(LIBCXX) -lc -lm -ldl, \ Instead of repeating this whole macro call for both Linux and non Linux, you can use parameters of the form LDFLAGS_linux and LIBS_linux to add the Linux specific flags. Something like this: LDFLAGS := $(LDFLAGS_JDKLIB), \ LDFLAGS_linux := -Wl$(COMMA)--no-as-needed, \ For the NAME field, there is no such support, so the way we usually do that is through a variable and conditionals before the macro call. What's the reason to have a different lib name on Windows? If they were the same, and the source file in windows/native/... had the same name, it would just automatically override in the build. I realize now that this is just moved code from jdk.incubator.foreign, and this patch is probably big enough as it is so no need to refactor the build logic at the same time. - Marked as reviewed by erikj (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview)
On Mon, 21 Mar 2022 10:45:27 GMT, Maurizio Cimadamore
wrote:
> This PR contains the API and implementation changes for JEP-424 [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/424
src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 600:
> 598: * @param elementLayout the source element layout. If the byte order
> associated with the layout is
> 599: * different from the native order, a byte swap operation will be
> performed on each array element.
> 600: * @return a fresh short array copy of this memory segment.
Maybe use "new" instead of "fresh" here and in the other MemorySegment::toArray
methods?
src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 600:
> 598: * @param elementLayout the source element layout. If the byte order
> associated with the layout is
> 599: * different from the native order, a byte swap operation will be
> performed on each array element.
> 600: * @return a fresh short array copy of this memory segment.
Maybe use "new" instead of "fresh" here and in the other MemorySegment::toArray
methods?
src/java.base/share/classes/java/lang/foreign/package-info.java line 149:
> 147: * provided:
> 148: *
> 149: * {@snippet lang=java :
missing leading space in comment on line 150 and 162
src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 7986:
> 7984: *
> 7985: * When calling e.g. {@link VarHandle#get(Object...)} on the
> resulting var handle, the incoming coordinate values
> 7986: * starting at position {@code pos} (of type {@code C1, C2 ... Cn},
> where {@code C1, C2 ... Cn} are the return type
... are the return *types* ...
src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 7986:
> 7984: *
> 7985: * When calling e.g. {@link VarHandle#get(Object...)} on the
> resulting var handle, the incoming coordinate values
> 7986: * starting at position {@code pos} (of type {@code C1, C2 ... Cn},
> where {@code C1, C2 ... Cn} are the return type
... are the return *types* ...
src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 8035:
> 8033: * @param pos the position of the first coordinate to be inserted
> 8034: * @param values the series of bound coordinates to insert
> 8035: * @return an adapter var handle which inserts an additional
> coordinates,
... which inserts additional coordinates, ...
src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 8035:
> 8033: * @param pos the position of the first coordinate to be inserted
> 8034: * @param values the series of bound coordinates to insert
> 8035: * @return an adapter var handle which inserts an additional
> coordinates,
... which inserts additional coordinates, ...
src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 8151:
> 8149: *
> 8150: * @param target the var handle to invoke after the dummy
> coordinates are dropped
> 8151: * @param pos position of first coordinate to drop (zero for the
> leftmost)
... of *the* first coordinate to drop ...
src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 8151:
> 8149: *
> 8150: * @param target the var handle to invoke after the dummy
> coordinates are dropped
> 8151: * @param pos position of first coordinate to drop (zero for the
> leftmost)
... of *the* first coordinate to drop ...
-
PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview)
On Mon, 21 Mar 2022 10:45:27 GMT, Maurizio Cimadamore wrote: > This PR contains the API and implementation changes for JEP-424 [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/424 Here is a list of the main changes in this iteration. java.lang.foreign The API is now a **preview** API in `java.lang.foreign`. As such to be able to use the API, users must pass the `--enable-preview` flag accordingly, to `javac` and `java`. Since the API now lives in `java.base`, we dropped the `MemoryHandles` class and moved all its var handle combinator methods under `MethodHandles`. We have also dropped the `MemorySegment::map` factory and replaced it with a new overload in `FileChannel`, which plays much better with custom file systems. ResourceScope The `ResourceScope` abstraction has been renamed to `MemorySession`. Aside from the naming difference (which also is reflected in some of the factories associated with `MemorySession`, another difference are that `MemorySession` now implements `SegmentAllocator` and can be used straight away to allocate segments. Finally, the fact that some sessions are not closeable is now reflected in the API (see `MemorySession::isCloseable`), and a method has been added to create a non-closeable *view* of the memory session. Restricted methods Addressing the feedback we have received during incubation, the mechanism to control access to restricted methods is now more permissive. Users can still use the `--enable-native-access` flag, to get a strict, opt-in behavior, in case they want to control which modules can access restricted methods in the foreign API. But if no flag is specified, access is allowed, with a runtime warning. Supporting this required some changes in `ModuleBootstrap` so that we could record the fact that we have seen an `--enable-native-access` flag (so that all checks in `Reflection.java` becomes constant). Deterministic library loading/unloading We have enhanced the `SymbolLookup` abstraction to provide a new symbol lookup, called *library lookup*. A library lookup is a symbol lookup built around a specific native library which is loaded/unloaded using dlopen/LoadLibrary. Library lookups are associated with memory sessions, so the library can be unloaded deterministically when the session is closed. Memory layouts All memory layout constants feature the expected alignment constraints. For instance, `JAVA_CHAR` is 2 byte aligned, `JAVA_INT` is 4 byte aligned, and so on. Removed functionalities As we moved the API in `java.base` we have dropped a number of API points which did not seem to add much value, based on the feedback received: * `SequenceLayout`s now always have a bounded size. As a result, `MemoryLayout::byteSize` no longer returns an optional. A zero-length sequence can be used instead; * `NativeSymbol` has been dropped. At the end of the day, its role is undistinguishable from that of a memory segment with zero-length, so API (and users) should use zero-length segments instead; * `MemorySession::keepAlive` - has been removed; in its place we have a simple, high-order method which executes an action (a `Runnable`) while keeping the session alive (`MemorySession::whileAlive`); * `MemoryLayout::map` only provides limited capabilities when rewriting layouts (e.g. it can only replace one layout at a time); as such we removed this API, and we might add something better at a later point. Hotspot changes While the Panama foreign repo contains several Hotspot changes which simplify and regularize the foreign function support, these changes are not included here, as we have discovered some intermittent instability in macosx-aarch64. We might attempt to integrate the hotspot changes at a later date. Javadoc: http://cr.openjdk.java.net/~mcimadamore/8282191/v1/javadoc/java.base/module-summary.html Specdiff: http://cr.openjdk.java.net/~mcimadamore/8282191/v1/specdiff_out/overview-summary.html - PR: https://git.openjdk.java.net/jdk/pull/7888
