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

[Maurizio Cimadamore]

Another iteration:

http://cr.openjdk.java.net/~mcimadamore/panama/8234049_v4/

Delta of the changes since last version (v3) here:

http://cr.openjdk.java.net/~mcimadamore/panama/8234049_v4_delta/

The javadoc has been updated inline here:

http://cr.openjdk.java.net/~mcimadamore/panama/memaccess_javadoc/jdk/incubator/foreign/package-summary.html 


Summary of changes:

* Improved javadoc regarding alignment and access modes in MemoryHandles
* related to the above, the memory access varhandle now checks for 
low-level VM alignment to for access other than get/set (as happens for 
regular byte buffer view var handle)
* fixed MemoryHandles, JavaLangInvokeAccess, VarHandles, 
MethodHandleImpl to speak about alignmentMask, rather than alignment (as 
per Roger comments)
* added some more javadoc to internal classes MemoryAddressProxy, 
VarHandleMemoryAddressBase, JavaNioAccess.java and 
JavaLangInvokeAccess.java  (as per Roger comments)
* fixed mising {code} block around alignment param A in 
MemoryLayout::bitAlignment/bytesAlignment (as per offline comments from 
Daniel)
* added positive test to TestMemoryCopy

Cheers
Maurizio

On 08/12/2019 01:44, Maurizio Cimadamore wrote:
> Another update:
>
> http://cr.openjdk.java.net/~mcimadamore/panama/8234049_v3/
>
> And a delta of the changes since last version (v2) here:
>
> http://cr.openjdk.java.net/~mcimadamore/panama/8234049_v3_delta/
>
> The javadoc has been updated inline here:
>
> http://cr.openjdk.java.net/~mcimadamore/panama/memaccess_javadoc/jdk/incubator/foreign/package-summary.html 
>
>
> Summary of changes:
>
> * fixed some (cosmetic) issues in FileChannelmpl following offline 
> discussion with Alan
> * changed tests group definition, so that now jdk_foreign is part of 
> tier1_part3
> * updated javadoc in various places to fix code samples (as per Paul 
> comments)
> * updated javadoc in MemoryHandles to talk about access modes (as per 
> Paul comments) - I added a new section on top, and then referred to in 
> from all relevant places
> * some changes to use Objects.hash (as per Paul comments), and some 
> minor refactor in the AddreddGenerator (to use switch expression)
> * tightened javadoc for MemoryAddress::copy - the method now is 
> specified to throw IAE if the range of source/dest addresses overlap - 
> I've fixed the impl and added a test (as per Raffaello comments)
> * tightened byte buffer VarHandle view - if the view is created from a 
> byte buffer obtained from a segment (!!!) we should do a segment check 
> - added tests
>
>
> And here's a list of the pending API-related issues. Since these are 
> IMHO minor issues, I suggest we defer them to a followup minor 
> cleanup, so that we can move ahead with finalization of the CSR with 
> the current API. Here's the list:
>
> * Should MemoryAddress implement Comparable? I think the answer is 
> "probably not" - an address doesn't have a 'length' so you can't 
> really do a range comparison (maybe memory segments though?). For now, 
> users can project to byte buffer and take it from there (since 
> ByteBuffer <: Comparable)
> * should we have a  way to ask a Layout if its size is specified ? (Paul)
> * MemoryAddress::offset() vs. MemoryAddress::offset(long) -- not much 
> distance between these two semantically different operations (Paul 
> suggested using 'add' - which I'm not enthusiastic about because it's 
> not adding two addresses - it's adding a long to an address...)
> * MemorySegment::isAccessible() -- as the A* word is overloaded, some 
> other name should be found?
> * MemorySegment::acquire() -- replace with MemorySegment::fork?
>
> Thanks
> Maurizio
>
> On 06/12/2019 10:43, Maurizio Cimadamore wrote:
>> Hi,
>> here's an updated version of the patch:
>>
>> http://cr.openjdk.java.net/~mcimadamore/panama/8234049_v2/
>>
>> And a delta of the changes since last version here:
>>
>> http://cr.openjdk.java.net/~mcimadamore/panama/8234049_v2_delta/
>>
>> The javadoc has been updated inline here:
>>
>> http://cr.openjdk.java.net/~mcimadamore/panama/memaccess_javadoc/jdk/incubator/foreign/package-summary.html 
>>
>>
>> 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?
>>
>> Cheers
>> Maurizio
>>
>>
>> On 05/12/2019 21:04, Maurizio Cimadamore 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
>>>