On Mon, 1 Nov 2021 12:05:32 GMT, Maurizio Cimadamore <mcimadam...@openjdk.org> wrote:
>> This PR contains the API and implementation changes for JEP-419 [1]. A more >> detailed description of such changes, to avoid repetitions during the review >> process, is included as a separate comment. >> >> [1] - https://openjdk.java.net/jeps/419 > > Maurizio Cimadamore has updated the pull request with a new target base due > to a merge or a rebase. The pull request now contains 17 commits: > > - Add cache for memory address var handles > - Merge branch 'master' into JEP-419 > - Fix regression in VaList treatment on AArch64 (contributed by @nick-arm) > - Merge branch 'master' into JEP-419 > - Fix copyright header in TestArrayCopy > - Fix failing microbenchmarks. Contributed by @FrauBoes (thanks!) > - * use `invokeWithArguments` to simplify new test > - Add test for liveness check with high-aririty downcalls > (make sure that if an exception occurs in a downcall because of liveness, > ref count of other resources are left intact). > - * Fix javadoc issue in VaList > * Fix bug in concurrent logic for shared scope acquire > - Address review comments > - ... and 7 more: > https://git.openjdk.java.net/jdk/compare/5bb1992b...9b519343 src/java.base/share/classes/java/lang/invoke/MethodHandleImpl.java line 1586: > 1584: public void ensureCustomized(MethodHandle mh) { > 1585: mh.customize(); > 1586: } This is no longer needed, but it probably got picked up in the merge. src/java.base/share/classes/jdk/internal/access/JavaLangInvokeAccess.java line 144: > 142: * @param mh the method handle > 143: */ > 144: void ensureCustomized(MethodHandle mh); Same here, no longer needed. (it was used by now removed upcall handler code. See https://github.com/openjdk/panama-foreign/pull/553) src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemoryAddress.java line 107: > 105: * > 106: * @param offset offset in bytes (relative to this address). The > final address of this read operation can be expressed as {@code > toRowLongValue() + offset}. > 107: * @return a Java UTF-8 string containing all the bytes read from > the given starting address ({@code toRowLongValue() + offset}) (see also comment on MemorySegment.getUtf8String) Suggestion: * @return a Java string constructed from the bytes read from the given starting address ({@code toRowLongValue() + offset}) src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegment.java line 387: > 385: > 386: /** > 387: * Performs an element-wise bulk copy from given source segment to > this segment. More specifically, the bytes at Suggestion: * Performs a byte-wise bulk copy from given source segment to this segment. More specifically, the bytes at src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegment.java line 400: > 398: * a multiple of the source element layout size, if the source > segment is incompatible with the alignment constraints > 399: * in the source element layout, or if this segment is incompatible > with the alignment constraints > 400: * in the destination element layout. This speaks about element layouts, but I don't see any element layouts in the method implementation. src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegment.java line 633: > 631: * java.nio.charset.CharsetDecoder} class should be used when more > control > 632: * over the decoding process is required. > 633: * @param offset offset in bytes (relative to this segment). For > instance, if this segment is a {@link #isNative()} segment, Suggestion: * @param offset offset in bytes (relative to this segment). For instance, if this segment is a {@link #isNative() native} segment, src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegment.java line 636: > 634: * the final address of this read operation can be > expressed as {@code address().toRowLongValue() + offset}. > 635: * @return a Java UTF-8 string containing all the bytes read from > the given starting address up to (but not including) > 636: * the first {@code '\0'} terminator character (assuming one is > found). The phrase "a Java UTF-8 string" sounds strange to me, as Java Strings are not encoded in UTF-8. The string that is read is UTF-8 encoded, but then it is converted from UTF-8 to Java internal String encoding (UTF-16 or Latin1). I'd suggest just dropping the 'UTF-8', and changing 'containing all' to 'constructed from'. Suggestion: * @return a Java string constructed from the bytes read from the given starting address up to (but not including) * the first {@code '\0'} terminator character (assuming one is found). src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegment.java line 652: > 650: * java.nio.charset.CharsetDecoder} class should be used when more > control > 651: * over the decoding process is required. > 652: * @param offset offset in bytes (relative to this segment). For > instance, if this segment is a {@link #isNative()} segment, Suggestion: * @param offset offset in bytes (relative to this segment). For instance, if this segment is a {@link #isNative() native} segment, src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegment.java line 762: > 760: > 761: /** > 762: * Creates a new native memory segment with given size and resource > scope, and whose base address is this address. Suggestion: * Creates a new native memory segment with given size and resource scope, and whose base address is the given address. src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegment.java line 769: > 767: * provided resource scope. > 768: * <p> > 769: * Clients should ensure that the address and bounds refers to a > valid region of memory that is accessible for reading and, Suggestion: * Clients should ensure that the address and bounds refer to a valid region of memory that is accessible for reading and, src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegment.java line 1035: > 1033: * > 1034: * @param layout the layout of the memory region to be read. > 1035: * @param offset offset in bytes (relative to this segment). For > instance, if this segment is a {@link #isNative()} segment, Suggestion: * @param offset offset in bytes (relative to this segment). For instance, if this segment is a {@link #isNative() native} segment, src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegment.java line 1549: > 1547: * @param index index (relative to this segment). For instance, if > this segment is a {@link #isNative()} segment, > 1548: * the final address of this write operation can be > expressed as {@code address().toRowLongValue() + (index * layout.byteSize())}. > 1549: * @param value the byte value to be written. Suggestion: * @param value the address value to be written. src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegment.java line 1563: > 1561: * Copies a number of elements from a source segment to a > destination array, > 1562: * starting at a given segment offset (expressed in bytes), and a > given array index, using the given source element layout. > 1563: * Supported array types are {@code byte[]}, {@code char[]},{@code > short[]},{@code int[]},{@code float[]},{@code long[]} and {@code double[]}. Suggestion: * Supported array types are {@code byte[]}, {@code char[]}, {@code short[]}, {@code int[]}, {@code float[]}, {@code long[]} and {@code double[]}. src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegment.java line 1604: > 1602: * Copies a number of elements from a source array to a destination > segment, > 1603: * starting at a given array index, and a given segment offset > (expressed in bytes), using the given destination element layout. > 1604: * Supported array types are {@code byte[]}, {@code char[]},{@code > short[]},{@code int[]},{@code float[]},{@code long[]} and {@code double[]}. Suggestion: * Supported array types are {@code byte[]}, {@code char[]}, {@code short[]}, {@code int[]}, {@code float[]}, {@code long[]} and {@code double[]}. src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/ResourceScope.java line 208: > 206: */ > 207: static ResourceScope newConfinedScope() { > 208: return ResourceScopeImpl.createConfined( Thread.currentThread(), > null); Suggestion: return ResourceScopeImpl.createConfined(Thread.currentThread(), null); src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/VaList.java line 132: > 130: /** > 131: * Copies this variable argument list at its current position into a > new variable argument list associated > 132: * with the same scope as this variable argument list. using the > segment provided allocator. Copying is useful to I think ". using the segment provided allocator" can be removed. Seems like a leftover from when we had an overload that took an allocator. Suggestion: * with the same scope as this variable argument list. Copying is useful to ------------- PR: https://git.openjdk.java.net/jdk/pull/5907
