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

[Peter Levart]

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
>