[foreign-memaccess+abi] RFR: 8301801: Implement arena-centric API
Jorn Vernee
jvernee at openjdk.org
Thu Feb 9 18:49:24 UTC 2023
On Tue, 7 Feb 2023 14:32:46 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:
> This patch implements the API proposal described here:
>
> https://cr.openjdk.java.net/~mcimadamore/panama/scoped_arenas.html
>
> The main changes are:
>
> * Static factories `MemorySegment::allocateNative` are gone;
> * The static factory `SegmentAllocator::nativeAllocator` is also gone;
> * `SegmentScope` moved as nested class in `MemorySegment` (e.g. `MemorySegment.Scope`);
> * Tweak methods accepting `SegmentScope` to accept `Arena` instead;
> * Simplify `Arena` API, by removing predicates (`isCloseable`/`isAccessibleBy`) and by dropping `whileAlive`;
> * Change arena factories to use `of` prefix instead of `open` - and rename `auto()` to `ofAuto()`.
>
> The API changes are rather straightforward, but they lead to several shallow test and microbenchmark changes, so the number of affected file is quite big (mostly because of the rename `openConfined` -> `ofConfined`).
>
> A javadoc of the proposed changes is available here:
>
> http://cr.openjdk.java.net/~mcimadamore/panama/scoped_arena_interface_lump_predicates/
Looks mostly good, see the inline comments.
src/java.base/share/classes/java/lang/foreign/Arena.java line 25:
> 23: * questions.
> 24: *
> 25: */
The changes to the copyright header don't look intentional?
src/java.base/share/classes/java/lang/foreign/Arena.java line 156:
> 154: * @since 20
> 155: */
> 156: @PreviewFeature(feature =PreviewFeature.Feature.FOREIGN)
Spurious?
Suggestion:
@PreviewFeature(feature=PreviewFeature.Feature.FOREIGN)
src/java.base/share/classes/java/lang/foreign/Linker.java line 38:
> 36: import java.lang.foreign.ValueLayout.OfAddress;
> 37: import java.lang.invoke.MethodHandle;
> 38: import java.util.Arrays;
Spurious import?
src/java.base/share/classes/java/lang/foreign/Linker.java line 132:
> 130: * <p>
> 131: * An upcall stub argument whose corresponding layout is an {@linkplain ValueLayout.OfAddress address layout}
> 132: * is a native segment associated with a scope that is alive for the entire duration of the upcall.
The new text is incorrect. Only structs/unions (associate with GroupLayout) are alive for the duration of the upcall. Addresses are always alive (same as downcall address returns).
src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 1177:
> 1175: * (often as a plain {@code long} value).
> 1176: * <p>
> 1177: * The returned segment is not read-only (see {@link MemorySegment#isReadOnly()}), and its lifetime is controlled
Suggestion:
* The returned segment is not {@linkplain MemorySegment#isReadOnly() read-only}, and its lifetime is controlled
src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 1220:
> 1218: * (often as a plain {@code long} value).
> 1219: * <p>
> 1220: * The returned segment is not read-only (see {@link MemorySegment#isReadOnly()}), and its lifetime is controlled
Suggestion:
* The returned segment is not {@linkplain MemorySegment#isReadOnly() read-only}, and its lifetime is controlled
src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 1815:
> 1813: * @throws IllegalArgumentException if provided address layout has a {@linkplain OfAddress#targetLayout() target layout}
> 1814: * {@code T}, and the address of the returned segment
> 1815: * <a href="MemorySegment.html#segment-alignment">incompatible with the alignment constraint</a> in {@code T}.
I don't think this bit should be removed?
src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 2043:
> 2041:
> 2042: /**
> 2043: /**
Spurious
Suggestion:
src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 2257:
> 2255: /**
> 2256: * {@return {@code true}, if the segments associated with this scope can be accessed}.
> 2257: */
Is this doc still correct now that we also have `MS::isAccessibleBy`?
src/java.base/share/classes/java/lang/foreign/SymbolLookup.java line 198:
> 196: *
> 197: * @param name the name of the library in which symbols should be looked up.
> 198: * @param arena the arema associated with symbols obtained from the returned lookup.
Suggestion:
* @param arena the arena associated with symbols obtained from the returned lookup.
src/java.base/share/classes/java/lang/foreign/package-info.java line 87:
> 85: *}
> 86: *
> 87: * A confined arena can be {@linkplain java.lang.foreign.Arena#close()}. When a confined
Suggestion:
* A confined arena can be {@linkplain java.lang.foreign.Arena#close() closed}. When a confined
src/java.base/share/classes/jdk/internal/foreign/AbstractMemorySegmentImpl.java line 89:
> 87: public static Arena arena(MemorySegment segment) {
> 88: return ((AbstractMemorySegmentImpl)segment).scope.asArena();
> 89: }
I can't find any usages of this.
src/java.base/share/classes/jdk/internal/foreign/MemorySessionImpl.java line 63:
> 61:
> 62: public static final MemorySessionImpl GLOBAL = new GlobalSession(null, false);
> 63: public static final MemorySessionImpl NATIVE = new GlobalSession(null, true);
I don't get why this distinction is needed. GlobalSession has an isInternal() predicate but I don't see it called anywhere.
Could you explain why we need 2 kinds of global session?
src/java.base/share/classes/jdk/internal/foreign/MemorySessionImpl.java line 106:
> 104: if (arena instanceof SessionArena arenaImpl) {
> 105: return arenaImpl.sessionImpl;
> 106: } else {
is this instanceof needed? Seems like we should always be able to call `arena.scope()`?
src/java.base/share/classes/jdk/internal/foreign/abi/SharedUtils.java line 157:
> 155:
> 156: private static MemorySegment bufferCopy(MemorySegment dest, MemorySegment buffer) {
> 157: return dest.asUnbounded().copyFrom(buffer);
Why is this change needed? The destination segment should already have the right size.
Looking at the current code, this is used for upcall IMR, where we get a pointer from native code to copy the return value into. When that pointer is boxed up, we should be attaching the right size to it (since we know the layout of the return value).
test/jdk/java/foreign/LibraryLookupTest.java line 75:
> 73: if (session instanceof Arena arena) {
> 74: assertEquals(addr.scope(), arena.scope());
> 75: }
This looks strange. Isn't this check redundant?
test/jdk/java/foreign/NativeTestHelper.java line 103:
> 101: public static MemorySegment allocateMemory(long size) {
> 102: try {
> 103: return ((MemorySegment) MALLOC.invokeExact(size)).asUnbounded();
Not sure why `asUnbounded` is needed here, since `C_POINTER` already has an unbounded target layout attached.
test/jdk/java/foreign/StdLibTest.java line 187:
> 185: buf.setUtf8String(0, s1);
> 186: MemorySegment other = arena.allocateUtf8String(s2);
> 187: return ((MemorySegment)strcat.invokeExact(buf, other)).asUnbounded().getUtf8String(0);
Same here
test/jdk/java/foreign/StdLibTest.java line 289:
> 287: static int qsortCompare(MemorySegment addr1, MemorySegment addr2) {
> 288: return addr1.asUnbounded().get(C_INT, 0) -
> 289: addr2.asUnbounded().get(C_INT, 0);
Same here
test/jdk/java/foreign/TestAdaptVarHandles.java line 197:
> 195: try (Arena arena = Arena.ofConfined()) {
> 196: Arena scope = arena;
> 197: MemorySegment seg = scope.allocate(ValueLayout.JAVA_INT);
Looks redundant
Suggestion:
MemorySegment seg = arena.allocate(ValueLayout.JAVA_INT);
test/jdk/java/foreign/TestAdaptVarHandles.java line 209:
> 207: try (Arena arena = Arena.ofConfined()) {
> 208: Arena scope = arena;
> 209: MemorySegment seg = scope.allocate(ValueLayout.JAVA_INT);
Suggestion:
MemorySegment seg = arena.allocate(ValueLayout.JAVA_INT);
test/jdk/java/foreign/TestArrays.java line 131:
> 129: long byteAlignment = layout.byteSize();
> 130: Arena scope = arena;
> 131: MemorySegment segment = scope.allocate(byteSize, byteAlignment);
Suggestion:
MemorySegment segment = arena.allocate(byteSize, byteAlignment);
test/jdk/java/foreign/TestArrays.java line 141:
> 139: Arena arena = Arena.ofConfined();
> 140: Arena scope = arena;
> 141: MemorySegment segment = scope.allocate(layout);
Suggestion:
MemorySegment segment = arena.allocate(layout);
test/jdk/java/foreign/TestByteBuffer.java line 185:
> 183: try (Arena arena = Arena.ofConfined()) {
> 184: Arena scope = arena;
> 185: MemorySegment segment = scope.allocate(tuples);;
Suggestion:
MemorySegment segment = arena.allocate(tuples);
test/jdk/java/foreign/TestMismatch.java line 241:
> 239: t.getCause().printStackTrace();
> 240: throw new AssertionError(t);
> 241: }
I guess this try/catch is no longer needed now that you fixed that circular init issue?
test/jdk/java/foreign/TestMismatch.java line 298:
> 296: NATIVE(i -> {
> 297: return Arena.ofAuto().allocate(i, 1);
> 298: }),
Could be:
Suggestion:
NATIVE(i -> Arena.ofAuto().allocate(i, 1)),
test/jdk/java/foreign/TestNulls.java line 123:
> 121: "java.lang.foreign.ValueLayout$OfDouble/withAttribute(java.lang.String,java.lang.constant.Constable)/1/0",
> 122: "java.lang.foreign.GroupLayout/withAttribute(java.lang.String,java.lang.constant.Constable)/1/0",
> 123: "java.lang.foreign.FunctionDescriptor/withAttribute(java.lang.String,java.lang.constant.Constable)/1/0"
Nice!
test/jdk/java/foreign/TestSegmentAllocators.java line 354:
> 352: Arena scope = drop;
> 353: return SegmentAllocator.slicingAllocator(scope.allocate(size, 1));
> 354: });
Suggestion:
SLICING(true, (size, scope) ->SegmentAllocator.slicingAllocator(scope.allocate(size, 1)));
test/jdk/java/foreign/TestSegments.java line 229:
> 227: () -> {
> 228: return Arena.ofAuto().allocate(4L, 8);
> 229: },
Suggestion:
() -> Arena.ofAuto().allocate(4L, 8),
test/jdk/java/foreign/TestSegments.java line 234:
> 232: () -> {
> 233: return Arena.ofAuto().allocate(4L, 8);
> 234: },
Suggestion:
() -> Arena.ofAuto().allocate(4L, 8),
test/micro/org/openjdk/bench/java/lang/foreign/BulkMismatchAcquire.java line 52:
> 50: @OutputTimeUnit(TimeUnit.MILLISECONDS)
> 51: @Fork(value = 3, jvmArgsAppend = "--enable-preview")
> 52: public class BulkMismatchAcquire {
Why was this dropped?
test/micro/org/openjdk/bench/java/lang/foreign/BulkOps.java line 71:
> 69: Arena scope = arena;
> 70: segment = scope.allocate(ALLOC_SIZE, 1);
> 71: }
I guess this is an artifact of an earlier iteration. Would be nice to clean these up though, I think.
test/micro/org/openjdk/bench/java/lang/foreign/StrLenTest.java line 198:
> 196: public MemorySegment allocate(long byteSize, long byteAlignment) {
> 197: MemorySegment segment = slicing.allocate(byteSize, byteAlignment);
> 198: return MemorySegment.ofAddress(segment.address(), byteSize, arena);
I don't think the re-wrapping is needed here? The returned slice should already have the same arena, right?
test/micro/org/openjdk/bench/java/lang/foreign/pointers/Pointer.java line 57:
> 55: public <Z extends NativeType.OfPointer<X>> X get(Z type, long index) {
> 56: MemorySegment address = segment.getAtIndex(type.layout(), index);
> 57: return (X)new Pointer<>(address.asUnbounded());
Shouldn't be needed? The used layout already has an unbounded pointee attached AFAICS
-------------
PR: https://git.openjdk.org/panama-foreign/pull/781
More information about the panama-dev
mailing list