RFR JDK-8234049: Implementation of Memory Access API (Incubator)
maurizio.cimadamore at oracle.com
Fri Dec 6 23:51:35 UTC 2019
On 06/12/2019 18:29, Raffaello Giulietti wrote:
> great job!
> I think that the doc of MemoryAddress.copy() should be explicit about
> the direction of the copying, so it should either:
Thanks! - I'll rectify the doc to specify lower-to-higher.
> * 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.
>> 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