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

Fri Dec 6 01:07:33 UTC 2019

On 06/12/2019 00:13, Remi Forax wrote:
> Hi Maurizio,
> that's a huge work :)
>
> So as a guy discovering the API,
> i've not taken a deep look to the implementation because i've noob questions.
>
> The first sentence of the overview of GroupLayout should say that there are two types of GroupLayout struct and union instead of talking about members layouts in italic.
> Still in GroupLayout, there is a static method "optSize" with no documentation and you don"t use the prefix "opt" anywhere else so i think it should be dropped.
> hasNaturalAlignment() is protected, but you don"t allow user to implement their own GroupLayout, so it's like the method was package private but visible in the javadoc.
Good catch - these two methods were never meant to be exposed - they are 
morally package private members of a shared implementation class - I 
will fix that
>
> MemoryLayout and MemoryLayout.PathElement static methods have names that are ok to use in static imports, but it's not explicit written in the javadoc.
> People will say that Java is verbose :)

To give some context here - we had an earlier variant of the API which 
was much more concise - e.g.

layout.varHandle(p -> p.groupElement("foo").sequenceElement())

This uses a builder-style idiom. The problem with this approach is that 
is less constant-folding friendly - which is the main reason as to why 
we went the current path.

>
> I don't see the point of having MemoryLayouts separated from MemoryLayout.

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).

If there's enough consensus one way or another I'm happy to consolidate 
the two classes.

>
> MemoryAddress should have a method that does raw bytes comparison like there is MemoryAddress.copy() so simulate struct comparison.
Uhm - it's not a bad idea - but I don't immediately see why it *has* to 
be in the API - it will be a loop - we can't do much more than that, in 
terms of making it fast.
>
> I don't understand how MemorySegment.acquire() solve the fact that you have two threads that can access to the same underlying memory area with unprotected access.
> When you own a memory segment, an other thread can acquire a new memory segment and then both thread can mutate the same memory location ?

With concurrent access you have two kinds of races:

- access vs. access (e.g. thread A reads while threads B write)

- access vs. close (e.g. thread A reads while thread B close the segment)

The first race is *not* what we want to protect you against here. The 
VarHandle API has plenty of primitives which allow clients to roll in 
their own synchronization. The really nasty issue is with the latter 
race - if you access a segment that has been closed, you don't just read 
a stale value, you can crash the VM.

When you acquire a segment you obtain a new view of the same memory 
block which is temporally bounded by the original segment. The original 
segment cannot be closed until all the acquired views have been closed. 
This guarantees that the memory will never go away behind your back.

>
> MemorySegment.isAccessible() is a very overloaded term, isOwnByCurrentThread() is maybe better ?
Open to suggestion - I don't particularly like yours though :-)
>
> In the implementation of MemoryScope, please don't use i++ or i-- to change the lambda parameter, what you want is i - 1 or i + 1.
> And i wander if it's not more efficient to use getAndIncrement and if it's negative (overflow), to do a supplementary volatile write to Integer.MIN_VALUE before raising the IllegalStateException.
Goo points - note that, in these cases, we don't really care about 
performance much - because acquiring a segment is not on the hot path 
(which is memory access).
>
> In AddressVarHandleGenerator.iConstInsn(),
> ICONST_M1, ICONST_0, etc are subsequent bytecodes so you can group all the case to: mv.visitInsn(ICONST_0 + i);

will do thanks!

Maurizio

>
> regards,
> Rémi
>
> ----- Mail original -----
>> De: "Maurizio Cimadamore" <maurizio.cimadamore at oracle.com>
>> À: "core-libs-dev" <core-libs-dev at openjdk.java.net>, "hotspot-dev" <hotspot-dev at openjdk.java.net>
>> Envoyé: Jeudi 5 Décembre 2019 22:04:55
>> Objet: RFR JDK-8234049: Implementation of Memory Access API (Incubator)
>> 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