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

[Raffaello Giulietti]

Hello,

great job!

I think that the doc of MemoryAddress.copy() should be explicit about 
the direction of the copying, so it should either:

* explicitly specify a direction, e.g., lower-to-higher addresses

* or specify that in the case of an overlap the copying is smart enough 
to not destroy the src bytes before they have landed in dst

* or accept a negative third argument to encode a higher-to-lower 
addresses copying direction.


Greetings
Raffaello



> 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