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

[Maurizio Cimadamore]

On 06/12/2019 12:28, Vladimir Ivanov wrote:
>
>>>> * ciField.cpp - this one is to trust final fields in the foreign 
>>>> memory access implementation (otherwise VM doesn't trust memory 
>>>> segment bounds)
>>>
>>> New packages aren't part of java.base. Considering the 
>>> implementation resides in an incubator module, is it enough to 
>>> consider them as trusted and well-known to the JVM?
>> This gives same performance as with -XX:+TrustFinalNonStaticFields - 
>> if we remove these changes, then memory segment bounds are never 
>> inlined. I'm happy to change this if you have other suggestions on 
>> how to get there, of course (I can run some benchmarks w/ and w/o and 
>> post some numbers if that helps).
>
> I'm not questioning whether these changes are needed for the 
> implementation or not. I wanted to attract attention that it's not 
> just a mechanical expansion of the list we have done in the past, but 
> a new case (java.base vs incubator) and hear from others are they fine 
> with leaving it as is?
>
> All the options I see (either new JVM annotation or trust final fields 
> by default) require significant engineering effort. So, I'd like to 
> clarify first whether the effort to rewrite current solution is 
> required or not.
Thanks for the clarification - I agree it's a bit unorthodox, but I'll 
leave to others (more qualified than me) to respond on that point.
>
>>>> * 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!)
>>>
>>> FTR it is tracked by JDK-8235143 [1] and the patch is under review [2].
>>
>> Should I remove this from the patch then?
>
> Yes, please.

Done in the recently submitted v2

Thanks
Maurizio

>
> Best regards,
> Vladimir Ivanov
>
>>>
>>> Best regards,
>>> Vladimir Ivanov
>>>
>>> [1] https://bugs.openjdk.java.net/browse/JDK-8235143
>>>
>>> [2] 
>>> https://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2019-December/036295.html 
>>>
>>>
>>>> * 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
>>>>