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

[Paul Sandoz]

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.

There is a trade-off being monomorphic in type or monomorphic in unsafe access.  One problem with `Buffer` was all the
different types would cause profile pollution if many were intermixed in the same code (e.g. on- and off-heap etc).

There might be a similar issue here, although given the primary target is off-heap there is less risk and i believe the
intermixing of native and mapped should not be an issue.  Still, it might be worth checking how intermixing on- and
off-heap behaves.

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

> 54:     @ForceInline
> 55:     HeapMemorySegmentImpl(long offset, Supplier<H> baseProvider, long length, int mask, Thread owner, MemoryScope
> scope) { 56:         super(length, mask, owner, scope);

Instead of a `Supplier` i wonder if one could pass in `H base, Class<H> baseType` then `base()` becomes:

  H base() {
      return Objects.requireNonNull(baseType.cast(base));
  }

If that works there is less for the JIT to do.

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

Marked as reviewed by psandoz (Committer).

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