[foreign-memaccess] RFR: JDK-8242495: Restructure implementation of memory segments

Wed Apr 15 11:54:10 UTC 2020

On Fri, 10 Apr 2020 15:24:06 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

> Currently memory segments use a monomorphic implementation. This implementation doesn't differentiate between heap
> segments and native segments and, as a result can lead to type profile pollution.
> Moreover, for native segment, the accessor to the unsafe base object is redundant (e.g. base is always null), but we
> can't easily shortcircuit that if we use a monomorphic implementation.
> Finally, it would be desirable, at some point, to add public sub-interfaces of MemorySegment, such as
> MappedMemorySegment which could provide a force() method. Unfortunately it would be cumbersome to do that with an
> underlying monomorphic implementation, unless we make force() total on all segments (this work will likely be addressed
> in a separate, follow-up PR).  This patch introduces a new abstract class which contain most of the memory segment
> implementation (it is essentially the same as the old MemorySegmentImpl).  Three subclasses are also added:
> 
> * NativeMemorySegmentImpl - to model native segments (base() returns null)
> * HeapMemorySegmentImpl - to model heap segments (base() returns some array)
> * MappedMemorySegmentImpl - to model memory mapped segments (this extends from NativeMemorySegment)
> 
> Each subclass overrides a dup() method, which must be used by the abstract subclass to implement view methods in a
> generic way. Each subclass also defines what base() and min() look like.
> Since now we have different segment implementation classes, I also decided to move all the factory methods from Utils
> to the segment impl class they belong, which leads to cleaner code.
> Note that, for heap segment there is a subtle trick: instead of storing the `base` object (an array) we store
> a *lambda* which retrieves the base object. This allows us to retain a monomorphic heap segment implementation, but,
> thanks to lambdas, we get sharply typed accessors to the base object, which helps out C2 when determining whether
> memory barriers should be added or not. This approach strikes a good balance between performance and code readability.
> Should we find cases where this approach is not enough, we could always fall back to create separate subclasses for
> each array type.  I've added benchmarks for both heap and mapped segments (they borrow heavily from the current
> LoopOverNonConstant), so that at least we can check performances in all cases. Numbers look good and similar across the
> whole spectrum of segments. As a comparison, before this patch, heap segments were roughly 10x slower than off-heap
> segments.  This patch also tidies up the byte buffer support; that is, it make sure that when going from a buffer to a
> segment (and back) all the correct information is preserved; while, before this patch, this was the case for native vs.
> heap segments, information about mapped segments was lost (as a "normal" direct buffer was created out of a mapped
> segment). This was also true when creating a segment from a mapped buffer - where a regular native segment was created.
> While semantics of access operations was not impacted by the aforementioned problems, calling force() on a buffer
> derived from a mapped segment might not behave as expected; this patch fixes that. To make sure that information is
> correctly preserved, I have enhanced the byte buffer test.

Looks good, some minor nits. I also left some comments about some pre-exisintg things for the record, but they should
probably be addressed separately.

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/InternalForeign.java line 52:

> 51:     public MemoryAddress withSize(MemoryAddress base, long byteSize) throws IllegalAccessError {
> 52:         return NativeMemorySegmentImpl.makeNativeSegmentUnchecked(base.toRawLongValue(), byteSize, null, false)
> 53:                 .baseAddress();

Pre-existing; should this check that `base` has the Nothing segment as segment? (otherwise access modes are dropped for
instance).

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/AbstractMemorySegmentImpl.java line 399:

> 398:         if (base != null) {
> 399:             return new HeapMemorySegmentImpl<>(bbAddress + pos, () -> (byte[])base, size,
> defaultAccessModes(size), Thread.currentThread(), bufferScope); 400:         } else if (unmapper == null) {

Seems to be pre-existing, but using the default access modes here would allow dropping access modes from a segment by
going to ByteBuffer and then back to MemorySegment. I think at least the read-only property of the BB should be
respected here, but maybe we can just retrieve the originally captured MemorySegmentProxy and use the access modes from
that?

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/NativeMemorySegmentImpl.java line 2:

> 1: /*
> 2:  *  Copyright (c) 2019, Oracle and/or its affiliates. All rights reserved.
> 3:  *  DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.

Update copyright year?
Suggestion:

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/HeapMemorySegmentImpl.java line 2:

> 1: /*
> 2:  *  Copyright (c) 2019, Oracle and/or its affiliates. All rights reserved.
> 3:  *  DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.

Update copyright year.
Suggestion:

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/AbstractMemorySegmentImpl.java line 66:

> 65:
> 66:     static JavaNioAccess nioAccess = SharedSecrets.getJavaNioAccess();
> 67:

Could be `final`
Suggestion:

    final static JavaNioAccess nioAccess = SharedSecrets.getJavaNioAccess();

-------------

Marked as reviewed by jvernee (Committer).

PR: https://git.openjdk.java.net/panama-foreign/pull/109