RFR JDK-8234049: Implementation of Memory Access API (Incubator)

Fri Dec 6 21:04:57 UTC 2019

I mostly looked at the API and implementation and not the tests.

s/offset/add or plus ? add ‘l’ to the offset of this memory address the result of which is the offset of the returned memory address.

If we ever have controlled operator overloading that’s how I would like to express it :-)

Paul.

VarHandles.java
—

  40     static ClassValue<Map<Integer, MethodHandle>> addressFactories = new ClassValue<Map<Integer, MethodHandle>>() {
  41         @Override
  42         protected Map<Integer, MethodHandle> computeValue(Class<?> type) {
  43             return new ConcurrentHashMap<>();
  44         }
  45     };

s/addressFactories/ADDRESS_FACTORIES/

Perhaps expose as ConcurrentMap to express concurrency requirements.

May be useful to pre-size the map.

JavaNioAccess.java
—

  52 /**
  53      * Constructs an heap ByteBuffer with given backing array, offset, capacity and segment.
  54      */
  55     ByteBuffer newHeapByteBuffer(byte[] hb, int offset, int capacity, MemorySegmentProxy segment);
  56 

Formatting issue.

FileChannelImpl.java
—

 867     private static abstract class Unmapper
 868         implements Runnable, UnmapperProxy
 869     {
 870         // may be required to close file
 871         private static final NativeDispatcher nd = new FileDispatcherImpl();
 872 
 873         // keep track of mapped buffer usage
 874         static volatile int count;
 875         static volatile long totalSize;
 876         static volatile long totalCapacity;

These new field declarations are also declared on the concrete subtypes. Did you intend to remove the declarations from the latter? 

VarHandles
—

I see you are relying on the fallback to lambda forms when linking the var handle, otherwise the explicit guard methods just explode :-)

Clever generation of VH code on demand punching in live constants, while pre-cooking accessors for “base" access.

AddressVarHandleGenerator.java
--

 355     private int returnInsn(Class<?> type) {
 356         switch (LambdaForm.BasicType.basicType(type)) {
 357             case I_TYPE:  return Opcodes.IRETURN;
 358             case J_TYPE:  return Opcodes.LRETURN;
 359             case F_TYPE:  return Opcodes.FRETURN;
 360             case D_TYPE:  return Opcodes.DRETURN;
 361             case L_TYPE:  return Opcodes.ARETURN;
 362             case V_TYPE:  return RETURN;
 363             default:
 364                 throw new InternalError("unknown return type: " + type);
 365         }
 366     }

Replace with expression form? (Same for following method too) I believe "JEP 361: Switch Expressions (Standard)” is integrated.

GroupLayout.java
—

  80         OptionalLong sizeof(List<MemoryLayout> elems) {
  81             long size = 0;
  82             for (MemoryLayout elem : elems) {
  83                 if (AbstractLayout.optSize(elem).isPresent()) {
  84                     size = sizeOp.applyAsLong(size, elem.bitSize());
  85                 } else {
  86                     return OptionalLong.empty();
  87                 }
  88             }
  89             return OptionalLong.of(size);
  90         }

FWIW you can do this:

OptionalLong sizeof(List<MemoryLayout> elems) {
    return elems.stream().filter(e -> AbstractLayout.optSize(e).isPresent()).mapToLong(MemoryLayout::bitSize)
            .reduce(sizeOp);
}

It may be question why there is no way to query if a MemoryLayout has a size or not. Does it require a hasSize method?

MemoryAddress.java
—

Should MemoryAddress implement Comparable?

MemoryHandles.java
—

 * As an example, consider the memory layout expressed by a {@link SequenceLayout} instance constructed as follows:
 * <blockquote><pre>{@code
SequenceLayout seq = MemoryLayout.ofSequence(5,
    MemoryLayout.ofStruct(
        MemoryLayout.ofPaddingBits(32),
        MemoryLayout.ofValueBits(32).withName("value")
    ));
 * }</pre></blockquote>

MemoryLayout.ofValueBits requires a byte order argument e.g.:

  MemoryLayout.ofValueBits(32, ByteOrder.BIG_ENDIAN)

 * <blockquote><pre>{@code
VarHandle handle = MemoryHandles.varHandle(int.class); //(MemoryAddress) -> int
handle = MemoryHandles.withOffset(handle, 4); //(MemoryAddress) -> int
handle = MemoryHandles.withStride(handle, 8); //(MemoryAddress, long) -> int
 * }</pre></blockquote>

MemoryHandles.varHandle requires a byte order argument e.g.:

  VarHandle handle = MemoryHandles.varHandle(int.class, ByteOrder.BIG_ENDIAN); //(MemoryAddress) -> int

(See also SequenceLayout)

 105     public static VarHandle varHandle(Class<?> carrier, ByteOrder byteOrder) {

You may need to specify what access modes are supported, as is the case for MethodHandles.byteBufferViewVarHandle, and also specify how comparison is performed for float/double with atomic update access modes i.e. copy and paste appropriate text. (Same applies to MemoryLayout.varHandle)

SequenceLayout.java
—

 118     @Override
 119     public int hashCode() {
 120         return super.hashCode() ^ elemCount.hashCode() ^ elementLayout.hashCode();
 121     }

I commonly resort to using Objects.hashCode. Don’t have a strong preference here. I guess you might be concerned about efficient?

package-info.java
—

  30  * <pre>{@code
  31 static final VarHandle intHandle = MemoryHandles.varHandle(int.class);
  32 
  33 try (MemorySegment segment = MemorySegment.allocateNative(10 * 4)) {
  34    MemoryAddress base = segment.baseAddress();
  35    for (long i = 0 ; i < 10 ; i++) {
  36      intHandle.set(base.offset(i * 4), (int)i);
  37    }
  38  }
  39  * }</pre>

MemoryHandles.varHandle requires a byte order argument.

(I wish we could compile code snippets in JavaDoc to surface errors.)

MemoryAddressImpl
—

  48     public MemoryAddressImpl(MemorySegmentImpl segment) {
  49         this(segment, 0);
  50     }

IDE reporting this constructor is never used. 

MemorySegmentImpl.java
—

  61     final static long NONCE = new Random().nextLong();

If a better quality NONCE is required do ‘new SplittableRandom()..nextLong();’

 186     private final boolean isSet(int mask) {
 187         return (this.mask & mask) != 0;
 188     }

Unnecessary final modifier.

Utils.java
—

 143             MemoryScope scope = new MemoryScope(null, () -> unmapperProxy.unmap());

Replace with method ref.

> On Dec 5, 2019, at 1:04 PM, Maurizio Cimadamore <maurizio.cimadamore at oracle.com> wrote:
> 
> Hi,
> as part of the effort to upstream the changes related to JEP 370 (foreign memory access API) [1], I'd like to ask for a code review for the corresponding core-libs and hotspot changes:
> 
> http://cr.openjdk.java.net/~mcimadamore/panama/8234049/
> 
> A javadoc for the memory access API is also available here:
> 
> http://cr.openjdk.java.net/~mcimadamore/panama/memaccess_javadoc/jdk/incubator/foreign/package-summary.html
> 
> Note: the patch passes tier1, tier2 and tier3 testing (**)
> 
> 
> Here is a brief summary of the changes in java.base and hotspot (the remaining new files are implementation classes and tests for the new API):
> 
> * ciField.cpp - this one is to trust final fields in the foreign memory access implementation (otherwise VM doesn't trust memory segment bounds)
> 
> * Modules.gmk - these changes are needed to require that the incubating module is loaded by the boot loader (otherwise the above changes are useless)
> 
> * library_call.cpp - this one is a JIT compiler change to treat Thread.currentThread() as a well-known constant - which helps a lot in the confinement checks (thanks Vlad!)
> 
> * various Buffer-related changes; these changes are needed because the memory access API allows a memory segment to be projected into a byte buffer, for interop reasons. As such, we need to insert a liveness check in the various get/put methods. Previously we had an implementation strategy where a BB was 'decorated' by a subclass called ScopedBuffer - but doing so required some changes to the BB API (e.g. making certain methods non-final, so that we could decorate them). Here I use an approach (which I have discussed with Alan) which doesn't require any public API  changes, but needs to add a 'segment' field in Buffer - and then have constructors which keep track of this extra parameter.
> 
> * FileChannel changes - these changes are required so that we can reuse the Unmapper class from the MemorySegment implementation, to deterministically deallocate a mapped memory segment. This should be a 'straight' refactoring, no change in behavior should occur here. Please double check.
> 
> * VarHandles - this class now provides a factory to create memory access VarHandle - this is a bit tricky, since VarHandle cannot really be implemented outside java.base (e.g. VarForm is not public). So we do the usual trick where we define a bunch of proxy interfaces (see jdk/internal/access/foreign) have the classes in java.base refer to these - and then have the implementation classes of the memory access API implement these interfaces.
> 
> * JavaNIOAccess, JavaLangInvokeAccess - because of the above, we need to provide access to otherwise hidden functionalities - e.g. creating a new scoped buffer, or retrieving the properties of a memory access handle (e.g. offset, stride etc.), so that we can implement the memory access API in its own separate module
> 
> * GensrcVarHandles.gmk - these changes are needed to enable the generation of the new memory address var handle implementations; there's an helper class per carrier (e.g. VarHandleMemoryAddressAsBytes, ...). At runtime, when a memory access var handle is needed, we dynamically spin a new VH implementation which makes use of the right carrier. We need to spin because the VH can have a variable number of access coordinates (e.g. depending on the dimensions of the array to be accessed). But, under the hood, all the generated implementation will be using the same helper class.
> 
> * tests - we've tried to add fairly robust tests, often checking all possible permutations of carriers/dimensions etc. Because of that, the tests might not be the easiest to look at, but they have proven to be pretty effective at shaking out issues.
> 
> I think that covers the main aspects of the implementation and where it differs from vanilla JDK.
> 
> P.S.
> 
> In the CSR review [2], Joe raised a fair point - which is MemoryAddress has both:
> 
> offset(long) --> move address of given offset
> offset() --> return the offset of this address in its owning segment
> 
> And this was considered suboptimal, given both methods use the same name but do something quite different (one is an accessor, another is a 'wither'). one obvious option is to rename the first to 'withOffset'. But I think that would lead to verbose code (since that is a very common operation). Other options are to:
> 
> * rename offset(long) to move(long), advance(long), or something else
> * drop offset() - but then add an overload of MemorySegment::asSlice which takes an address instead of a plain long offset
> 
> I'll leave the choice to the reviewers :-)
> 
> 
> 
> Finally, I'd like to thank Mark, Brian, John, Alan, Paul, Vlad, Stuart, Roger, Joe and the Panama team for the feedback provided so far, which helped to get the API in the shape it is today.
> 
> Cheers
> Maurizio
> 
> (**) There is one failure, for "java/util/TimeZone/Bug6329116.java" - but that is unrelated to this patch, and it's a known failing test.
> 
> [1] - https://openjdk.java.net/jeps/370
> [2] - https://bugs.openjdk.java.net/browse/JDK-8234050
> 