[foreign-memaccess+abi] RFR: 8291473: Unify MemorySegment and MemoryAddress
Jorn Vernee
jvernee at openjdk.org
Thu Jul 28 15:24:14 UTC 2022
On Thu, 28 Jul 2022 09:51:54 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:
> This patch implements the changes described in [1].
>
> The main things to notice are the removal of `MemoryAddress` and `Addressable`. Everything else falls from that.
>
> There are some changes to the `MemorySegment` API as well, as some methods such as `segmentOffset` and `asOverlappingSlice` have been removed, and instead `MemorySegment::address` now works on heap segments too (and, clients can obtain the array of an heap segment using its `array` method). So, these methods can, if needed, be implemented outside the core API.
>
> `MemorySegment::equals` now compares the segments address, which is the most useful comparison when working with native interop (again, client can implement deeper comparison using `mismatch`).
>
> Finally, restricted factories accepting raw addresses (in `MemorySegment` and `VaList`) have been tweaked to accept a long value instead.
>
> `VaList` also needed some adjustments, since it builds on top of segments. The API is similar to before - but instead of having an address accessor, it has a zero-length segment accessor.
>
> `ValueLayout.OfAddress` has a new method to create *unsafe* address layouts which return segment with maximal size (`Long.MAX_VALUE`). I've used these layouts in a lot of places in the implementation internal, which simplifies things quite a bit (e.g. removing the need to create a new segment from an address).
>
> There are several test and microbenchmark updates, but relatively minor, and all caused by removal of `MemoryAddress`/`Addressable`. Changes here should be relatively simple to follow.
>
> While the javadoc is ok, I didn't put too much effort in trying to provide a complete and cohesive story on how zero-length segments are used to model raw addresses. I'm working on some more extensive doc changes on the side, but I wanted to split the javadoc changes from the impl changes, so as to simplify the review work. I hope that makes sense.
>
> [1] - https://mail.openjdk.org/pipermail/panama-dev/2022-July/017181.html
There seems to be some comments (and 1 string) that mention `MemoryAddress`. Please update those as well.
src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 873:
> 871: throw new IllegalArgumentException("Invalid size : " + bytesSize);
> 872: }
> 873: return NativeMemorySegmentImpl.makeNativeSegmentUnchecked(address, bytesSize);
Maybe this could just call `MemorySegment.ofAddress(address, byteSize, MemorySession.global())` ?
src/java.base/share/classes/java/lang/foreign/VaList.java line 237:
> 235: * @param address the address of the variable argument list.
> 236: * @param session the memory session to be associated with the returned variable argument list.
> 237: * @return a new variable argument list backed by a native memory region based starting at the given address value.
Suggestion:
* @return a new variable argument list backed by a native memory region starting at the given address value.
src/java.base/share/classes/java/lang/foreign/VaList.java line 247:
> 245: */
> 246: @CallerSensitive
> 247: static VaList ofAddress(long address, MemorySession session) {
Don't we still want the native access (and `null`) checks here? I see that the VaList impls eventually end up calling `MemorySegment.ofAddress` any way, but I think it's clearer to check it here as well (both in terms of code readability, as well as stack traces that are produced).
src/java.base/share/classes/java/lang/foreign/ValueLayout.java line 648:
> 646: @CallerSensitive
> 647: public OfAddress asUnbounded() {
> 648: Reflection.ensureNativeAccess(Reflection.getCallerClass(), ValueLayout.OfAddress.class, "asUnsafe");
Suggestion:
Reflection.ensureNativeAccess(Reflection.getCallerClass(), ValueLayout.OfAddress.class, "asUnbounded");
src/java.base/share/classes/java/lang/foreign/package-info.java line 175:
> 173: * MemorySegment foreign = someSegment.get(ValueLayout.ADDRESS.asUnbounded(), 0); // obtain foreign segment (size = Long.MAX_VALUE)
> 174: * int x = foreign.get(ValueLayout.JAVA_INT, 0); //ok
> 175: *}
Suggestion:
* Alternatively, clients can obtain, <em>unsafely</em>, an {@linkplain java.lang.foreign.ValueLayout.OfAddress#asUnbounded() unbounded}
* address value layout. Unbounded address value layouts allow the API to view foreign segments as segments with maximal size
* (e.g. {@linkplain java.lang.Long#MAX_VALUE}), meaning that clients can always perform dereference operations on a foreign
* segment obtained using an unbounded address layout:
*
* {@snippet lang = java:
* MemorySegment foreign = someSegment.get(ValueLayout.ADDRESS.asUnbounded(), 0); // obtain foreign segment (size = Long.MAX_VALUE)
* int x = foreign.get(ValueLayout.JAVA_INT, 0); //ok
*}
src/java.base/share/classes/jdk/internal/foreign/AbstractMemorySegmentImpl.java line 248:
> 246: throw new UnsupportedOperationException("Cannot compute offset from native to heap (or vice versa).");
> 247: }
> 248:
To be honest. I would prefer to save the removal of these methods for another patch.
The methods were added in the first place, so I'm not so sure that they are no longer needed now. Even without the `array()` accessor, it was possible for clients to implement these methods using `isNative` in the past.
Having a separate patch will also make it easier to find this specific change, the reasoning behind it, and potentially weigh in on it.
src/java.base/share/classes/jdk/internal/foreign/HeapMemorySegmentImpl.java line 45:
> 43: * sharp type information, as well as sharp null-check information. For this reason, many concrete subclasses
> 44: * of {@link HeapMemorySegmentImpl} are defined (e.g. {@link OfFloat}, so that each subclass can override the
> 45: * {@link HeapMemorySegmentImpl#base()} method so that it returns an array of the correct (sharp) type. Note that
It seems the implementations of this method are still called `unsafeGetBase`. I guess they should be renamed as well?
src/java.base/share/classes/jdk/internal/foreign/HeapMemorySegmentImpl.java line 72:
> 70: public long maxAlignMask() {
> 71: return 0;
> 72: }
I don't think this implementation of `maxAlignMask` is needed? All the implementations override it already.
src/java.base/share/classes/jdk/internal/foreign/abi/AbstractLinker.java line 53:
> 51: MethodType type = SharedUtils.inferMethodType(fd);
> 52: MethodHandle handle = arrangeDowncall(type, fd);
> 53: handle = SharedUtils.maybeInsertAllocator(function, handle);
Please use `fd` here to avoid `function` being captured as well.
Suggestion:
handle = SharedUtils.maybeInsertAllocator(fd, handle);
src/java.base/share/classes/jdk/internal/foreign/abi/Binding.java line 629:
> 627: * and pushes that onto the operand stack.
> 628: */
> 629: record BoxAddress(long size) implements Binding {
Please update the doc comment as well.
src/java.base/share/classes/jdk/internal/foreign/abi/Binding.java line 655:
> 653: * with the given size, and pushes that onto the operand stack
> 654: */
> 655: record ToSegment(long size) implements Binding {
Please update the doc comment here as well.
src/java.base/share/classes/jdk/internal/foreign/abi/BindingSpecializer.java line 448:
> 446: private static boolean shouldAcquire(Class<?> type) {
> 447: // by this time, the only MemorySegment left are those for by-ref args
> 448: return type == MemorySegment.class;
I don't quite get the comment here. Looking at the caller of `shouldAcquire`, it seems like we can also call this for by-value struct parameters. This seems different from what we do today in that we would also end up acquiring scopes of by-value structs, which is not needed.
src/java.base/share/classes/jdk/internal/foreign/abi/BindingSpecializer.java line 680:
> 678: private void emitUnboxAddress() {
> 679: popType(MemorySegment.class, ASSERT_EQUALS);
> 680: emitAddress();
The implementation of `popType` can be cleaned up/simplified now, since we only need to check for equality.
src/java.base/share/classes/jdk/internal/foreign/abi/CallingSequenceBuilder.java line 114:
> 112: Binding.vmLoad(abi.retBufAddrStorage(), long.class),
> 113: Binding.boxAddress(Long.MAX_VALUE),
> 114: Binding.toSegment(returnBufferSize)));
Looking at this, I think `ToSegment` should just accept a `long` now, since we can also construct segments from `long` now.
Otherwise, I think either the `boxAddress` should use `0`, and then `toSegment` assigns the right size, or I think it should use `returnBufferSize`.
src/java.base/share/classes/jdk/internal/foreign/abi/SharedUtils.java line 203:
> 201:
> 202: private static MemorySegment bufferCopy(MemorySegment dest, MemorySegment buffer) {
> 203: NativeMemorySegmentImpl.makeNativeSegmentUnchecked(dest.address(), buffer.byteSize()).copyFrom(buffer);
The fact that we need to create an unchecked segment from `dest` here seems like a bit of a smell. Although I acknowledge that it recreates the old behavior.
The target here should be the artificial leading address added by CallArranger, which should already be using the unbounded address layout when boxing the address. What happens if you just use `dest.copyFrom(buffer)`?
src/java.base/share/classes/jdk/internal/foreign/abi/UpcallStubs.java line 31:
> 29:
> 30: import jdk.internal.foreign.MemorySessionImpl;
> 31: import jdk.internal.foreign.NativeMemorySegmentImpl;
Spurious import
Suggestion:
src/java.base/share/classes/jdk/internal/foreign/abi/aarch64/CallArranger.java line 458:
> 456: bindings.vmLoad(storage, long.class)
> 457: .boxAddress(Long.MAX_VALUE)
> 458: .toSegment(layout);
Same here, I think this `boxAddress` should either take `layout.byteSize`, but probably `toSegment` should just work on `longs`.
The difference between the 2 bindings would only be the session that gets assigned.
src/java.base/share/classes/jdk/internal/foreign/abi/x64/windows/CallArranger.java line 278:
> 276: bindings.vmLoad(storage, long.class)
> 277: .boxAddress(Long.MAX_VALUE)
> 278: .toSegment(layout);
Same here. ToSegment should take a `long`.
test/jdk/java/foreign/TestLayoutEquality.java line 83:
> 81:
> 82: static ValueLayout valueLayoutForCarrier(Class<?> carrier, ByteOrder order) {
> 83: return MemoryLayout.valueLayout(carrier, order);
Maybe calls to this can just be inlined, and the method removed.
test/jdk/java/foreign/TestUpcallBase.java line 149:
> 147: for (int i = 0; i < o.length; i++) {
> 148: if (o[i] instanceof MemorySegment &&
> 149: !((MemorySegment) o[i]).session().equals(MemorySession.global())) {
This is a bit mystical looking (though I understand what it does). I suggest passing/binding the function descriptor or list of `ParamType` to `passAndSave`, so that it has the right info to determine whether this argument should be copied, and then use that info.
Or, maybe just add a comment here explaining that this is discerning pointers from by-value struct arguments.
test/jdk/java/foreign/TestUpcallHighArity.java line 87:
> 85: for (int i = 0; i < o.length; i++) {
> 86: if (o[i] instanceof MemorySegment &&
> 87: !((MemorySegment)o[i]).session().equals(MemorySession.global())) {
Same here.
looking at this now, using `instanceof` was a bit of a hack in the first place (that I added). Proper way to do it is to bind the function descriptor here, I think.
test/jdk/java/foreign/TestUpcallHighArity.java line 118:
> 116: for (int i = 0; i < capturedArgsArr.length; i++) {
> 117: if (upcallType.parameterType(i) == MemorySegment.class &&
> 118: !isPointer(upcallDescriptor.argumentLayouts().get(i))) {
Maybe just?
Suggestion:
if (upcallDescriptor.argumentLayouts().get(i) instanceof GroupLayout) {
test/jdk/java/foreign/handles/invoker_module/handle/invoker/MethodHandleInvoker.java line 75:
> 73: addDefaultMapping(MemoryLayout.class, ValueLayout.JAVA_INT);
> 74: addDefaultMapping(FunctionDescriptor.class, FunctionDescriptor.ofVoid());
> 75: addDefaultMapping(MemorySession.class, MemorySession.openShared());
I'm curious why this was needed? :)
test/jdk/java/foreign/libNull.c line 1:
> 1: #ifdef _WIN64
Missing copyright header
test/jdk/java/foreign/valist/VaListTest.java line 190:
> 188: BiFunction<Integer, VaList, Double> sumDoublesNative
> 189: = MethodHandleProxies.asInterfaceInstance(BiFunction.class,
> 190: MethodHandles.filterArguments(MH_sumDoubles, 1, VALIST_TO_ADDRESS));
I suggest maybe moving this adaptation to `linkVaList` (the VaList should always be the last parameter)
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 {
Was this removed intentionally?
test/micro/org/openjdk/bench/java/lang/foreign/QSort.java line 82:
> 80: panama_upcall_compar = abi.upcallStub(
> 81: lookup().findStatic(QSort.class,
> 82: "panama_upcall_compar_resize",
I don't see this method. Maybe forgot to revert after an earlier change?
test/micro/org/openjdk/bench/java/lang/foreign/pointers/NativeType.java line 1:
> 1: package org.openjdk.bench.java.lang.foreign.pointers;
missing copyright header
test/micro/org/openjdk/bench/java/lang/foreign/pointers/Point.java line 1:
> 1: package org.openjdk.bench.java.lang.foreign.pointers;
missing copyright header
test/micro/org/openjdk/bench/java/lang/foreign/pointers/Point.java line 18:
> 16: }
> 17: int y() {
> 18: return ptr.segment.getAtIndex(NativeType.C_INT.layout(), 4);
Shouldn't this just be `get`? (otherwise this seems like it would access at offset = 4 * 4)
test/micro/org/openjdk/bench/java/lang/foreign/pointers/Pointer.java line 1:
> 1: package org.openjdk.bench.java.lang.foreign.pointers;
missing copyright header
test/micro/org/openjdk/bench/java/lang/foreign/pointers/PointerBench.java line 1:
> 1: package org.openjdk.bench.java.lang.foreign.pointers;
missing copyright header
test/micro/org/openjdk/bench/java/lang/foreign/pointers/Struct.java line 1:
> 1: package org.openjdk.bench.java.lang.foreign.pointers;
missing copyright header
-------------
PR: https://git.openjdk.org/panama-foreign/pull/694
More information about the panama-dev
mailing list