RFR JDK-8234049: Implementation of Memory Access API (Incubator)
maurizio.cimadamore at oracle.com
Fri Dec 6 10:43:22 UTC 2019
here's an updated version of the patch:
And a delta of the changes since last version here:
The javadoc has been updated inline here:
Summary of changes:
* fixed spurious protected methods in AbstractLayout and subclasses
which leaked into API
* removed library_call.cpp changes, as these are being tracked
separately by Vlad
* compacted ILOAD code in AddressVarHandleGenerator
* replaced uses of ++i/--i with i + 1/i - 1 in MemoryScope
I have made no changes to the *name* of the methods in the API. In fact,
I suggest we keep a list of the names we'd like to revisit, and we
address them all at once at the end of the review (once we're happy with
the contents). Here's a list of the current open naming issues:
* MemoryAddress::offset() vs. MemoryAddress::offset(long) -- not much
distance between these two semantically different operations
* MemorySegment::isAccessible() -- as the A* word is overloaded, some
other name should be found?
* MemorySegment::acquire() -- replace with MemorySegment::fork?
On 05/12/2019 21:04, Maurizio Cimadamore wrote:
> as part of the effort to upstream the changes related to JEP 370
> (foreign memory access API) , I'd like to ask for a code review for
> the corresponding core-libs and hotspot changes:
> A javadoc for the memory access API is also available here:
> 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
> * 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.
> In the CSR review , 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.
> (**) There is one failure, for "java/util/TimeZone/Bug6329116.java" -
> but that is unrelated to this patch, and it's a known failing test.
>  - https://openjdk.java.net/jeps/370
>  - https://bugs.openjdk.java.net/browse/JDK-8234050
More information about the core-libs-dev