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

[Raffaello Giulietti]

Fine, thanks!

Raffaello


On 2019-12-09 22:30, Maurizio Cimadamore wrote:
> Hi again,
> after some offline discussions with the hotspot team, it became clear 
> that the recently added restriction on source/destination segments being 
> disjoint is redundant and that Unsafe::copyMemory can cope with overlap. 
> For now I'l keep the API as is, but I will file a followup issue to 
> remove the restriction from the API - which, I think, will also address 
> your issue around lack of memmove ability.
> 
> Thanks
> Maurizio
> 
> On 09/12/2019 11:10, Maurizio Cimadamore wrote:
>> Hi Raffaello,
>> I think there is room to add more copy-related features in the future. 
>> One thing to note, however: the rationale behind having a 'copy' 
>> method is to expose a (safe) interface to the underlying 
>> Unsafe::copyMemory method - which is an hotspot intrinsic, hence 
>> understood and optimized by the JIT compiler. I'd say that, if we were 
>> to add another method like 'memmove' we'd have to work out the VM 
>> plumbing first - and then add the MemoryAddress API method. This has 
>> been the informal guiding principle for the operations exposed by this 
>> API.
>>
>> That said - I'll keep a note of the suggestion, and add it to the list.
>>
>> Cheers
>> Maurizio
>>
>> On 09/12/2019 10:11, Raffaello Giulietti wrote:
>>> Hi,
>>>
>>> will there be a
>>>
>>> MemoryAddress.move(MemoryAddress src, MemoryAddress dst, long bytes)
>>>
>>> method with POSIX memmove(3) semantics at some point?
>>>
>>> That would be useful, e.g., to "open a hole" into an array by 
>>> shifting existing elements towards higher indices (provided there's 
>>> room).
>>>
>>> MemoryAddress.copy(), with its lower-to-higher semantics, doesn't 
>>> really help here, so without move() one would need to code an 
>>> explicit loop for such a case, I guess. Not a big deal, just a little 
>>> bit annoying.
>>>
>>>
>>> Greetings
>>> Raffaello
>>>
>>>
>>>
>>> On 2019-12-07 00:51, Maurizio Cimadamore wrote:
>>>>
>>>> On 06/12/2019 18:29, Raffaello Giulietti wrote:
>>>>> Hello,
>>>>>
>>>>> 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.
>>>>
>>>> Maurizio
>>>>
>>>>>
>>>>> * 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
>>>>>
>>>>>
>>>