[foreign-memaccess] RFC: remove CompoundLayout and PaddingLayout

[Brian Goetz]

Overall -- I like this version much better!  We have whittled the number 
of abstractions the user has to understand before being able to write 
"Hello Native" down to the minimum.  Very nice.

I love that all the factories for layouts are now in Layout.  The first 
thing a user has to do is make a layout; having them all in one place 
helps them get past this task quickly.  (When we have static patterns, 
these factories should be paired with patterns.)

Some naming nits:
  - The naming of all the methods that produce VarHandles need a good 
overhaul.  `dereferenceHandle()` sounds to me like an action -- 
"dereference the handle now" -- rather than making a handle that does 
the dereferencing.  Similarly, the names of the combinators could use 
some work.
  - MethodAccessVarHandles is a mouthful but I assume that these methods 
will eventually end up in MethodHandles.Lookup, or in VarHandle, so this 
is a temporary problem.
  - Suggest bitsSize -> bitSize (and similar for other bitty and bytey 
methods)
  - Suggest MemorySegment.resize -> narrow
  - Suggest elementsSize -> elementCount (elementSize could easily be 
the size of an individual element, and newer APIs are using count() in 
preference to size() for clarity.)

The treatment of unbounded sequences seems iffy.  If Layout::elements 
might return an unbounded sequence, and there is no way to ask "are you 
an unbounded sequence", what is the utility of calling 
Layout::elements?  I get I might want to grovel through _named_ group 
elements, but an infinite stream of the same layout doesn't seem like 
anything I'd ever want to get.  Similarly, what should elementsSize() 
return when I have an unbounded sequence?  I have factories for both 
bounded and unbounded sequences, but then I can't really tell the 
difference between them.  I think some rationalization here is needed.

The `Layout::alignTo` method needs more specification.  Am I guaranteed 
that it will have the same elements, for example?  I assume so, but we 
should say this.

I like that we can get a handle from a ValueLayout with `layout.handle()`.

Add comment that says Layout will eventually be a sealed type, and that 
all subtypes will be value-based types.

Add comments to the effect that PathElement, *Layout, and MemoryAddress 
are value-based.

I know this is a can of worms, but given that layouts for, say, 32-bit 
int in native bit order will be super-common, I'd prefer to have 
constants for these rather than calling the factories over and over.

Nice work!




On 7/3/2019 11:58 AM, Maurizio Cimadamore wrote:
> Hi,
> over the last few days I've been discussing the existing memory access 
> API with Brian (thanks!). While the overall feedback was positive 
> (we're moving in the right direction!), Brian raised a couple of 
> interesting points:
>
> * the PaddingLayout abstracton is super thin - there's no method in 
> there - virtually can be a factory on Layout (e.g. Layout.ofPadding)
>
> * while the lambda approach for creating layout paths is clever, there 
> are some issues with it:
>
> - methods like dereferenceHandle are only on ValueLayout or 
> CompoundLayout. If the user has a variable of type Layout (likely!) 
> those won't work. This creates some friction
>
> - having a lambda chain here prevents features such as compile-driven 
> constant folding (JEP ...) to be able, in the future, to spit out 
> these constants
>
> * the CompoundLayout abstraction seems also a bit 'thin'. Yes there 
> are three methods in there, but there's no method in there that 
> 'screams' for having its own abstraction (e.g. the elements() method 
> can easily return an empty stream when invoked on a value/padding 
> layout).
>
>
> We went a bit back and forth, and landed on a slightly different place 
> which, usability-wise doesn't lose anything from the previous model 
> (in fact, I think it gains something) and, in terms of API simplicity 
> and discoverability is much improved.
>
> Here are the basic moves:
>
> - move _all_ layout factory methods into Layout - users will find it 
> easily there
>
> - drop PaddingLayout (this is almost a slamdunk)
>
> - replace the lambda-based layout path mechanism with something more 
> explicit: we introduce a Layout.PathElement interface and then we 
> rewire the dereferenceHandle and offset methods to accept a 'varargs' 
> of these. So instead of this:
>
> groupLayout.dereferenceHandle( int.class, path -> 
> path.sequenceElement().groupElement("foo"))
>
> the new API lets you do this:
>
> groupLayout.dereferenceHandle( int.class, 
> PathElement.sequenceElement(), PathElement.groupElement("foo"))
>
> It's not terribly different, but note two advantages:
>
> * no lambdas, so constant folding will work (when ready)
>
> * this form degenerates nicely to 'zero' path elements, which is 
> amenable to be used for ValueLayouts - e.g.
>
> valyeLayout.dereferenceHandle(int.class)
>
>
> With this move, I could move all CompoundLayout members up to Layout, 
> and still get an API that makes sense.
>
> Here's the new javadoc:
>
> http://cr.openjdk.java.net/~mcimadamore/panama/memaccess_javadoc_v4/javadoc/jdk/incubator/foreign/package-summary.html 
>
>
> And here's the associated webrev:
>
> http://cr.openjdk.java.net/~mcimadamore/panama/compound_layout_removal/
>
> Thoughts?
>
> Maurizio
>
>
>
>
>