RFR JDK-8234049: Implementation of Memory Access API (Incubator)
maurizio.cimadamore at oracle.com
Fri Dec 6 12:06:00 UTC 2019
On 06/12/2019 11:53, Jorn Vernee wrote:
> > * drop offset() - but then add an overload of MemorySegment::asSlice
> which takes an address instead of a plain long offset
> This sounds good to me, since it fits with what we're doing with
> makeUncheckedSegment(MemoryAddress, length), and we added the offset()
> accessor to support slicing. I don't like changing the name of
> offset(long) to advance(long) since the offset can be negative as well
> (and 'advance' implies only positive).
Right - I'd say if we go down this path we need at least two overload:
* MemorySegment::asSlice(MemoryAddress, long)
And then we could be offset() free :-)
> >> I don't see the point of having MemoryLayouts separated from
> > Possibly - I found myself thinking that too - although, with the
> subsequent Panama step (ABI support) we'll be adding a ton of
> ABI-dependent layouts in here... (but we could address that in other
> ways also).
> We had some ideas to move the ABI constants to separate classes I
> remember. I.e. provide separate classes for different platform ABIs
> (implementing SystemABI), and stick the constants in there. Together
> with the idea to make JAVA_INT into Integer::LAYOUT later, I think the
> remaining constants fit into MemoryLayout pretty naturally, and we can
> remove MemoryLayouts.
Yeah - that's what I was referring to when I said " (but we could
address that in other ways also). " :-)
> On 06/12/2019 11:43, Maurizio Cimadamore wrote:
>> 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
>> * 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