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

Mon Dec 9 18:31:06 UTC 2019

Hi Peter,
this looks like a clever optimization which basically takes advantage of 
the fact that you can only access a segment if you are the same thread 
as the segment owner. I think after we integrate, I'd love to give your 
approach a try and see what happens performance-wise.

Right now we don't pay much for confinement check because the Thread 
owner is a final constant - so these checks are optimized away nicely. 
But we pay something for the liveness check, because the 'isAlive' field 
is mutable. I believe your approach won't change a lot here - it folds 
the two checks, but we were already paying zero for one of the two, so...

Anyway - all this needs to be validated by adequate JMH-ing :-)

Thanks very much for your comments!

Maurizio

On 09/12/2019 18:16, Peter Levart wrote:
> Hi Maurizio,
>
> Nice work!
>
> I see the API prefixes each independent access to memory with a call 
> to the following MemorySegmentImpl method:
>
>  157     @Override
>  158     public final void checkValidState() {
>  159         if (owner != Thread.currentThread()) {
>  160             throw new IllegalStateException("Attempt to access 
> segment outside owning thread");
>  161         }
>  162         scope.checkAlive();
>  163     }
>
> The 1st check is making sure the method is invoked in the "owner" 
> thread, while the second is making sure that scope is still alive. The 
> 2nd check is only correct if it is performed in the "owner" thread 
> which is guaranteed. These two are checks of mutable fields in two 
> places (objects).
>
> May I suggest a scheme that combines both checks and only tests for 
> one value in one object on the hot path:
>
> - drop Thread owner field from MemorySegmentImpl
> - drop boolean isAlive field from MemoryScope
> - add Thread owner field to MemoryScope
> - delegate MemorySegment.isAlive() and 
> MemorySegmentImpl.checkValidState() to MemoryScope:
>
>     final boolean isAlive() {
>         return activeCount.get() >= UNACQUIRED;
>     }
>
>     final boolean isAccessible() {
>         return Thread.currentThread() == owner;
>     }
>
>     final void checkAlive() {
>         if (!isAlive()) {
>             throw new IllegalStateException("Segment is not alive");
>         }
>     }
>
>     final void checkValidState() {
>         if (Thread.currentThread() != owner) {
>             checkAlive();
>             throw new IllegalStateException("Attempt to access segment 
> outside owning thread");
>         }
>     }
>
> - make MemoryScope.close() be the following:
>
>     void close() {
>         // assert Thread.currentThread() == MemorySegmentImpl.owner
>         if (!activeCount.compareAndSet(UNACQUIRED, CLOSED)) {
>             throw new IllegalStateException("Cannot close a segment 
> that has active acquired views");
>         }
>         owner = null;
>         if (cleanupAction != null) {
>             cleanupAction.run();
>         }
>     }
>
> - make other arrangements that pass Thread.currentThread() to 
> MemoryScope instead of to MemorySegmentImpl constructor.
>
>
> Voila, only (Thread.currentThread() != owner) check on the hot path!
>
> Notice that .isAlive() is now also thread-safe. Previously it wasn't.
>
>
> What do you think? Might this be better performance wise?
>
> Regards, Peter
>
>
> On 12/5/19 10:04 PM, 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
>>
>