RFR: 8277924: Small tweaks to foreign function and memory API
Following integration of the second incubator of the foreign function and memory API [1], we detected few divergences between the contents of the jdk repo and the panama repo: * the name of some of the `FunctionDescriptor` wither methods is different (e.g. `withAppendedLayoutArguments` vs. `appendLayoutArguments`), as it has been simplified and improved following a change that was not incorporated in [1]. * TestUpcall does not execute all the test combinations, because of an issue in the jtreg header (also fixed in the panama repo) * Addressing some feedback, we would like to bring back alignment to JAVA_INT layout constants (and related constants). Javadoc: http://cr.openjdk.java.net/~mcimadamore/8277924/v1/javadoc/jdk/incubator/for... Specdiff: http://cr.openjdk.java.net/~mcimadamore/8277924/v1/spec_diff/overview-summar... [1] - #5907 ------------- Commit messages: - Further tweak to javadoc of layout constants - Wrong alignment constraints in ValueLayout constants javadoc - Tweak javadoc - Fix jtreg header on TestUpcall - Tweak API names for FunctionDescriptor methods - * Tweak value layout constants alignment to reflect VM alignment Changes: https://git.openjdk.java.net/jdk/pull/6589/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=6589&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8277924 Stats: 343 lines in 14 files changed: 206 ins; 5 del; 132 mod Patch: https://git.openjdk.java.net/jdk/pull/6589.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6589/head:pull/6589 PR: https://git.openjdk.java.net/jdk/pull/6589
On Mon, 29 Nov 2021 11:22:45 GMT, Maurizio Cimadamore <mcimadamore@openjdk.org> wrote:
Following integration of the second incubator of the foreign function and memory API [1], we detected few divergences between the contents of the jdk repo and the panama repo:
* the name of some of the `FunctionDescriptor` wither methods is different (e.g. `withAppendedLayoutArguments` vs. `appendLayoutArguments`), as it has been simplified and improved following a change that was not incorporated in [1].
* TestUpcall does not execute all the test combinations, because of an issue in the jtreg header (also fixed in the panama repo)
* Addressing some feedback, we would like to bring back alignment to JAVA_INT layout constants (and related constants).
Javadoc: http://cr.openjdk.java.net/~mcimadamore/8277924/v1/javadoc/jdk/incubator/for... Specdiff: http://cr.openjdk.java.net/~mcimadamore/8277924/v1/spec_diff/overview-summar...
[1] - #5907
Marked as reviewed by jvernee (Reviewer). src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/FunctionDescriptor.java line 134:
132: * layout array of this function descriptor. 133: * @param index the index at which to insert the arguments 134: * @param addedLayouts the argument layouts to append.
Suggestion: * @param addedLayouts the argument layouts to insert. src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/ValueLayout.java line 205:
203: 204: static final int ALIGNED_POS = 0; 205: static final int UNALIGNED_POS = 1;
Suggestion: private static final int ALIGNED_POS = 0; private static final int UNALIGNED_POS = 1; src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/Utils.java line 179:
177: 178: @Stable 179: public static LayoutAccess LAYOUT_ACCESS;
Clever :) Maybe some comment here about what this is doing would be nice (I had to look at it for a while). Suggestion: public static LayoutAccess LAYOUT_ACCESS; // shared secret Alternatively, I think you could do this with a MethodHandle acquired through `Lookup.privateLookupIn(lookup(), ValueLayout.class).findVirtual(...)`. ------------- PR: https://git.openjdk.java.net/jdk/pull/6589
On Mon, 29 Nov 2021 13:26:11 GMT, Jorn Vernee <jvernee@openjdk.org> wrote:
Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision:
Update src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/ValueLayout.java
Co-authored-by: Jorn Vernee <JornVernee@users.noreply.github.com>
src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/FunctionDescriptor.java line 134:
132: * layout array of this function descriptor. 133: * @param index the index at which to insert the arguments 134: * @param addedLayouts the argument layouts to append.
Suggestion:
* @param addedLayouts the argument layouts to insert.
Fixed. ------------- PR: https://git.openjdk.java.net/jdk/pull/6589
On Mon, 29 Nov 2021 11:22:45 GMT, Maurizio Cimadamore <mcimadamore@openjdk.org> wrote:
Following integration of the second incubator of the foreign function and memory API [1], we detected few divergences between the contents of the jdk repo and the panama repo:
* the name of some of the `FunctionDescriptor` wither methods is different (e.g. `withAppendedLayoutArguments` vs. `appendLayoutArguments`), as it has been simplified and improved following a change that was not incorporated in [1].
* TestUpcall does not execute all the test combinations, because of an issue in the jtreg header (also fixed in the panama repo)
* Addressing some feedback, we would like to bring back alignment to JAVA_INT layout constants (and related constants).
Javadoc: http://cr.openjdk.java.net/~mcimadamore/8277924/v1/javadoc/jdk/incubator/for... Specdiff: http://cr.openjdk.java.net/~mcimadamore/8277924/v1/spec_diff/overview-summar...
[1] - #5907
src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegment.java line 1337:
1335: */ 1336: @ForceInline 1337: default char getAtIndex(ValueLayout.OfChar layout, long index) {
We need to thread lightly here. The alignment check is not free, performance-wise, because of [1]. So, if we can detect that the var handle will always be accessed at offsets that are multiple of the alignment, we can use the more optimized "aligned" var handle, which will skip the alignment check. Eventually this special casing will go away when [1] is fixed. [1] - https://bugs.openjdk.java.net/browse/JDK-8277850 src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/MemoryAddressImpl.java line 273:
271: public char getAtIndex(ValueLayout.OfChar layout, long index) { 272: Reflection.ensureNativeAccess(Reflection.getCallerClass()); 273: if (layout.byteAlignment() <= layout.byteSize()) {
Here we also have to workaround [1]. Since we don't want to slice the everything segment (as that would create an additional instance), the only option is to use the var handle directly, but to use the "aligned" version if we detect that the offset is always aligned. One caveat is that we need to manually check the base address for alignment, since this check will be omitted otherwise. Again, this logic will go away when [1] is fixed. [1] - https://bugs.openjdk.java.net/browse/JDK-8277850 ------------- PR: https://git.openjdk.java.net/jdk/pull/6589
Following integration of the second incubator of the foreign function and memory API [1], we detected few divergences between the contents of the jdk repo and the panama repo:
* the name of some of the `FunctionDescriptor` wither methods is different (e.g. `withAppendedLayoutArguments` vs. `appendLayoutArguments`), as it has been simplified and improved following a change that was not incorporated in [1].
* TestUpcall does not execute all the test combinations, because of an issue in the jtreg header (also fixed in the panama repo)
* Addressing some feedback, we would like to bring back alignment to JAVA_INT layout constants (and related constants).
Javadoc: http://cr.openjdk.java.net/~mcimadamore/8277924/v1/javadoc/jdk/incubator/for... Specdiff: http://cr.openjdk.java.net/~mcimadamore/8277924/v1/spec_diff/overview-summar...
[1] - #5907
Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Update src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/ValueLayout.java Co-authored-by: Jorn Vernee <JornVernee@users.noreply.github.com> ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/6589/files - new: https://git.openjdk.java.net/jdk/pull/6589/files/2041e785..54b89f30 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6589&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6589&range=00-01 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/6589.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6589/head:pull/6589 PR: https://git.openjdk.java.net/jdk/pull/6589
On Mon, 29 Nov 2021 18:32:30 GMT, Maurizio Cimadamore <mcimadamore@openjdk.org> wrote:
Following integration of the second incubator of the foreign function and memory API [1], we detected few divergences between the contents of the jdk repo and the panama repo:
* the name of some of the `FunctionDescriptor` wither methods is different (e.g. `withAppendedLayoutArguments` vs. `appendLayoutArguments`), as it has been simplified and improved following a change that was not incorporated in [1].
* TestUpcall does not execute all the test combinations, because of an issue in the jtreg header (also fixed in the panama repo)
* Addressing some feedback, we would like to bring back alignment to JAVA_INT layout constants (and related constants).
Javadoc: http://cr.openjdk.java.net/~mcimadamore/8277924/v1/javadoc/jdk/incubator/for... Specdiff: http://cr.openjdk.java.net/~mcimadamore/8277924/v1/spec_diff/overview-summar...
[1] - #5907
Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision:
Update src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/ValueLayout.java
Co-authored-by: Jorn Vernee <JornVernee@users.noreply.github.com>
src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/ValueLayout.java line 619:
617: */ 618: public static final OfDouble JAVA_DOUBLE = new OfDouble(ByteOrder.nativeOrder()) 619: .withBitAlignment(ADDRESS_SIZE_BITS);
The code here is still using `ADDRESS_SIZE_BITS` (same for JAVA_LONG), instead of `64` as the javadoc says. Not sure that is intended? ------------- PR: https://git.openjdk.java.net/jdk/pull/6589
On Mon, 29 Nov 2021 18:32:30 GMT, Maurizio Cimadamore <mcimadamore@openjdk.org> wrote:
Following integration of the second incubator of the foreign function and memory API [1], we detected few divergences between the contents of the jdk repo and the panama repo:
* the name of some of the `FunctionDescriptor` wither methods is different (e.g. `withAppendedLayoutArguments` vs. `appendLayoutArguments`), as it has been simplified and improved following a change that was not incorporated in [1].
* TestUpcall does not execute all the test combinations, because of an issue in the jtreg header (also fixed in the panama repo)
* Addressing some feedback, we would like to bring back alignment to JAVA_INT layout constants (and related constants).
Javadoc: http://cr.openjdk.java.net/~mcimadamore/8277924/v1/javadoc/jdk/incubator/for... Specdiff: http://cr.openjdk.java.net/~mcimadamore/8277924/v1/spec_diff/overview-summar...
[1] - #5907
Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision:
Update src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/ValueLayout.java
Co-authored-by: Jorn Vernee <JornVernee@users.noreply.github.com>
I've uploaded a new iteration, which drops the changes to value layout constants. The rationale behind this move is explained below: https://mail.openjdk.java.net/pipermail/panama-dev/2021-November/015852.html Javadoc: http://cr.openjdk.java.net/~mcimadamore/8277924/v2/javadoc/jdk/incubator/for... Specdiff: http://cr.openjdk.java.net/~mcimadamore/8277924/v2/spec_diff/overview-summar... ------------- PR: https://git.openjdk.java.net/jdk/pull/6589
Following integration of the second incubator of the foreign function and memory API [1], we detected few divergences between the contents of the jdk repo and the panama repo:
* the name of some of the `FunctionDescriptor` wither methods is different (e.g. `withAppendedLayoutArguments` vs. `appendLayoutArguments`), as it has been simplified and improved following a change that was not incorporated in [1].
* TestUpcall does not execute all the test combinations, because of an issue in the jtreg header (also fixed in the panama repo)
* Addressing some feedback, we would like to bring back alignment to JAVA_INT layout constants (and related constants).
Javadoc: http://cr.openjdk.java.net/~mcimadamore/8277924/v1/javadoc/jdk/incubator/for... Specdiff: http://cr.openjdk.java.net/~mcimadamore/8277924/v1/spec_diff/overview-summar...
[1] - #5907
Maurizio Cimadamore has updated the pull request incrementally with two additional commits since the last revision: - Drop changes to alignment of layout constants - Simplify logic to access package-private method in ValueLayout ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/6589/files - new: https://git.openjdk.java.net/jdk/pull/6589/files/54b89f30..e2dfb83b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6589&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6589&range=01-02 Stats: 255 lines in 6 files changed: 0 ins; 159 del; 96 mod Patch: https://git.openjdk.java.net/jdk/pull/6589.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6589/head:pull/6589 PR: https://git.openjdk.java.net/jdk/pull/6589
On Tue, 30 Nov 2021 13:20:32 GMT, Maurizio Cimadamore <mcimadamore@openjdk.org> wrote:
Following integration of the second incubator of the foreign function and memory API [1], we detected few divergences between the contents of the jdk repo and the panama repo:
* the name of some of the `FunctionDescriptor` wither methods is different (e.g. `withAppendedLayoutArguments` vs. `appendLayoutArguments`), as it has been simplified and improved following a change that was not incorporated in [1].
* TestUpcall does not execute all the test combinations, because of an issue in the jtreg header (also fixed in the panama repo)
* Addressing some feedback, we would like to bring back alignment to JAVA_INT layout constants (and related constants).
Javadoc: http://cr.openjdk.java.net/~mcimadamore/8277924/v1/javadoc/jdk/incubator/for... Specdiff: http://cr.openjdk.java.net/~mcimadamore/8277924/v1/spec_diff/overview-summar...
[1] - #5907
Maurizio Cimadamore has updated the pull request incrementally with two additional commits since the last revision:
- Drop changes to alignment of layout constants - Simplify logic to access package-private method in ValueLayout
Marked as reviewed by psandoz (Reviewer). src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/FunctionDescriptor.java line 140:
138: public FunctionDescriptor insertArgumentLayouts(int index, MemoryLayout... addedLayouts) { 139: Objects.requireNonNull(addedLayouts); 140: Arrays.stream(addedLayouts).forEach(Objects::requireNonNull);
FWIW you can remove this line if you wish, since the `List.of()` will check for null elements. You could replace lines 139 to 143 with: if (index < 0 || index > argLayouts.size()) throw new IllegalArgumentException("Index out of bounds: " + index); List<MemoryLayout> added = List.of(addedLayouts); // null check on array and its elements List<MemoryLayout> newLayouts = new ArrayList<>(argLayouts.size() + added.size()); ------------- PR: https://git.openjdk.java.net/jdk/pull/6589
Following integration of the second incubator of the foreign function and memory API [1], we detected few divergences between the contents of the jdk repo and the panama repo:
* the name of some of the `FunctionDescriptor` wither methods is different (e.g. `withAppendedLayoutArguments` vs. `appendLayoutArguments`), as it has been simplified and improved following a change that was not incorporated in [1].
* TestUpcall does not execute all the test combinations, because of an issue in the jtreg header (also fixed in the panama repo)
* Addressing some feedback, we would like to bring back alignment to JAVA_INT layout constants (and related constants).
Javadoc: http://cr.openjdk.java.net/~mcimadamore/8277924/v1/javadoc/jdk/incubator/for... Specdiff: http://cr.openjdk.java.net/~mcimadamore/8277924/v1/spec_diff/overview-summar...
[1] - #5907
Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Simplify FunctionDescriptor::insertArgumentLayouts ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/6589/files - new: https://git.openjdk.java.net/jdk/pull/6589/files/e2dfb83b..c305199c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6589&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6589&range=02-03 Stats: 4 lines in 1 file changed: 1 ins; 2 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/6589.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6589/head:pull/6589 PR: https://git.openjdk.java.net/jdk/pull/6589
Following integration of the second incubator of the foreign function and memory API [1], we detected few divergences between the contents of the jdk repo and the panama repo:
* the name of some of the `FunctionDescriptor` wither methods is different (e.g. `withAppendedLayoutArguments` vs. `appendLayoutArguments`), as it has been simplified and improved following a change that was not incorporated in [1].
* TestUpcall does not execute all the test combinations, because of an issue in the jtreg header (also fixed in the panama repo)
* Addressing some feedback, we would like to bring back alignment to JAVA_INT layout constants (and related constants).
Javadoc: http://cr.openjdk.java.net/~mcimadamore/8277924/v1/javadoc/jdk/incubator/for... Specdiff: http://cr.openjdk.java.net/~mcimadamore/8277924/v1/spec_diff/overview-summar...
[1] - #5907
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 12 additional commits since the last revision: - Revert test change - Merge branch 'master' into value_layout_align - Simplify FunctionDescriptor::insertArgumentLayouts - Drop changes to alignment of layout constants - Simplify logic to access package-private method in ValueLayout - Update src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/ValueLayout.java Co-authored-by: Jorn Vernee <JornVernee@users.noreply.github.com> - Further tweak to javadoc of layout constants - Wrong alignment constraints in ValueLayout constants javadoc - Tweak javadoc - Fix jtreg header on TestUpcall - ... and 2 more: https://git.openjdk.java.net/jdk/compare/26afe572...f3f78190 ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/6589/files - new: https://git.openjdk.java.net/jdk/pull/6589/files/c305199c..f3f78190 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6589&range=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6589&range=03-04 Stats: 7127 lines in 257 files changed: 4342 ins; 1547 del; 1238 mod Patch: https://git.openjdk.java.net/jdk/pull/6589.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6589/head:pull/6589 PR: https://git.openjdk.java.net/jdk/pull/6589
On Mon, 29 Nov 2021 11:22:45 GMT, Maurizio Cimadamore <mcimadamore@openjdk.org> wrote:
Following integration of the second incubator of the foreign function and memory API [1], we detected few divergences between the contents of the jdk repo and the panama repo:
* the name of some of the `FunctionDescriptor` wither methods is different (e.g. `withAppendedLayoutArguments` vs. `appendLayoutArguments`), as it has been simplified and improved following a change that was not incorporated in [1].
* TestUpcall does not execute all the test combinations, because of an issue in the jtreg header (also fixed in the panama repo)
* Addressing some feedback, we would like to bring back alignment to JAVA_INT layout constants (and related constants).
Javadoc: http://cr.openjdk.java.net/~mcimadamore/8277924/v1/javadoc/jdk/incubator/for... Specdiff: http://cr.openjdk.java.net/~mcimadamore/8277924/v1/spec_diff/overview-summar...
[1] - #5907
This pull request has now been integrated. Changeset: ea905bd3 Author: Maurizio Cimadamore <mcimadamore@openjdk.org> URL: https://git.openjdk.java.net/jdk/commit/ea905bd3dad5fc1baad66e714bdd01fa679d... Stats: 84 lines in 7 files changed: 46 ins; 5 del; 33 mod 8277924: Small tweaks to foreign function and memory API Reviewed-by: jvernee, psandoz ------------- PR: https://git.openjdk.java.net/jdk/pull/6589
participants (3)
-
Jorn Vernee
-
Maurizio Cimadamore
-
Paul Sandoz