[foreign-memaccess] RFC: remove CompoundLayout and PaddingLayout

Wed Jul 3 17:00:25 UTC 2019

On 03/07/2019 17:15, Brian Goetz wrote:
> 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.
yeah - will rename to MemoryHandles
>  - Suggest bitsSize -> bitSize (and similar for other bitty and bytey 
> methods)
ok
>  - Suggest MemorySegment.resize -> narrow
ok - was thinking about slice(), which also seems nice (and consistent 
with byte buffers)
>  - 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.)
ok
>
> 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.
I believe elements() is mostly there as an attempt to unify group and 
sequence (a dream we had back then). I think with this API we can 
probably live w/o elements(). If you really want to access to 
sub-elements, down-cast to the right layout (e.g. GroupLayout) and 
access elements from there (GroupLayout::elementAt(long)). Deal?
>
> 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.
ok - good point
>
> 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.
good point
>
> Add comments to the effect that PathElement, *Layout, and 
> MemoryAddress are value-based.
I believe I did this for most of the classes - but might have missed 
some - will double-check
>
> 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.

Yep - this depends quite a bit on what we decide to do with respect to 
endianness. That is, we can have an UINT32 constant, but what is the 
endianness? This is a problem we also had in mainline Panama/foreign branch.

Overall I agree that users will want to work with constants, rather than 
with factories - so we need to find some compromise in that area, and 
come up with a good set of constants. In Panama we did this:

http://hg.openjdk.java.net/panama/dev/file/9352af5aeb3c/src/java.base/share/classes/java/foreign/NativeTypes.java

(this class defines LayoutType constants, not Layout constants, but the 
concepts are similar).

The main idea is to have many nested interfaces, for LittleEndian, 
BigEndian (and then inside those, more nested interfaces for the target 
platform).

So, if you want to use UINT32, you can import NativeTypes.LittleEndian 
and be done (if LE is what you need).

Alternatively, if what you want to use is C types - you can import the 
full platform NativeTypes.LittleEndian.SysVABI (this is for Linux), and 
start using types like 'INT'.

While this is very complete, it also seems very complex. Offline we 
discussed the possibility for having Java layout constants - e.g.

JAVA_INT = Layout.ofSignedInt(32)

That's also fine.

I'm less fond of introducing C types as of now, since this layer is not 
about C interop - so one possibility here would be:

interface Layouts {

     interface Java {
         //all Java constants here
     }

     interface LittleEndian {
        //all sized LE constants here
     }

     interface BigEndian {
        //all sized BE constants here
     }

     interface HostEndian {
        //all sized platform endian constants here
     }

}

This should be enough of a starter kit?

We could also NOT use nested interfaces and use some naming convention

JAVA_INT
HOST_UINT8
LE_UINT8

It's mostly a trade off here - you get a flatter API (which is probably 
better for Javadoc), but you lose the ability to 'import just what you 
need'. I don't have strong feelings either way.

Maurizio

>
> 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
>>
>>
>>
>>
>>
>