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

[Maurizio Cimadamore]

On Fri, 10 Apr 2020 16:41:26 GMT, Henry Jen <henryjen 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.
>
> test/jdk/java/foreign/TestByteBuffer.java line 613:
> 
>> 612:                     { ByteBuffer.allocateDirect(256), nativeTest },
>> 613:                     { channel.map(FileChannel.MapMode.READ_ONLY, 0L, 256), mappedTest }
>> 614:             };
> 
> Isn't the file channel closed after return? I am suppose this is OK as we don't access it and probably intentional here.
> 
> What I am curious about, is what suppose to happen to the MS after this channel is closed? Since this is from
> MappedByteBuffer, I suppose it inherits the unspecified behavior.

My understanding is that lifecycle of a mapped buffer is not tied to that of the channel where it came from. The API
for `FileChannel::map` says:

 A mapping, once established, is not dependent upon the file channel that was used to create it. Closing the channel, in
 particular, has no effect upon the validity of the mapping.

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

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