[foreign-memaccess] RFR 8226421: Remove LayoutPath API

[Jorn Vernee]

On 2019-06-25 15:50, Maurizio Cimadamore wrote:
> Hi
> 
> On 25/06/2019 13:25, Jorn Vernee wrote:
>> Hi,
>> 
>> There are good things in this patch; 1.) I like the fact that group 
>> elements can no longer be accessed by index, as well as 2.) the fact 
>> that we can now only use compound layouts or value layouts as a root 
>> path element.
>> 
>> But, these seem orthogonal to the switch to using path expressions, 
>> and I'm a little more skeptic about that. With path expressions we 
>> have;
>> 
>> * one less interface in the API (but complexity still exists in 
>> grammar)
>> * more compact to create
>> 
>> But imho one of the biggest problems with LayoutPaths was that it was 
>> still possible to create a path that pointed to an element that could 
>> not be de-referenced, which is still possible with path expressions. 
>> Moreover, this removes the static validity of a path, because it's 
>> possible to mess up the expression string. I think LayoutPath has the 
>> following pros compared to path expressions;
> I'm honestly not super convinced this is a big problem... when you
> create a path, the path must end in a leave (e.g. a value layout).
> This is actually easier to enforce in the new expression-based API
> than it was in the path-based world (since a path was just ... a
> path).
>> 
>> * API speaks for itself more (no need for separate grammar description 
>> to describe what's possible)
> I can buy that
>> * Static validity of the path
> 
> Less certain about this - if you mean that, once you construct a chain
> of layout path calls you are certain that the path is good, I think
> this might be too optimistic - you could still have selected something
> that doesn't make sense (e.g. select field "foo" that's not declared
> in the struct). So - not sure what yo mean by 'static' validity - both
> variants need to be checked against the layout - and errors reported
> accordingly.

Thanks, I hadn't considered that yet.

> The only advantage I see is that it's not possible to write an
> ill-formed path (e.g. `foo[bar]`) with the API-based approach because
> the API is itself the grammar. Of course you trade this advantage off
> with compactness.

Yes, I was thinking of ill-formed paths.

>> 
>> And, personally, I like these pros more than the ones we get from path 
>> expressions.
>> 
>> Maybe I'm missing some pros/cons here (as I was not part of the 
>> earlier talks). But, currently, I think I'd prefer sticking with 
>> LayoutPath, and implementing just 1. and 2. There are some other ideas 
>> like spelling out the valid carriers with different xxxHandle methods 
>> (charHandle, byteHandle, etc.) to at least partially close the invalid 
>> carrier type hole. Or replace the toPath-like methods in Layout with 
>> static factory methods in LayoutPath (like we had before), to decouple 
>> Layouts from LayoutPath a bit more.
> 
> The main issue with the layout path API is that is fairly front and
> center in the current memory access API, whereas in reality its role
> is fairly minor (e.g. select a leaf layout from a compound, so that we
> can compute some var handle, or some offset). Although I can agree
> that the layout path API route is self-consistent (which is why we
> went there), the general feeling is that a toplevel abstraction for
> this seems a tad on the heavy side.

I feel this as well, but I also feel that moving this 'weight' into a 
grammar specification is just a way of hiding it.

> One alternative would be to turn LayoutPath into some kind of
> functional interface:
> 
> VarHandle handle = groupLayout.dereferenceHandle(path -> path
>                                   .groupElement("foo")
> .sequenceElement(42)
>                                    .groupElement("bar"));
> 
> While this is similar in spirit to what we currently have, I can see
> some benefits:
> 
> * we can still add the very (very!) handy
> ValueLayout::dereferenceHandle (which basically allows to avoid uses
> of paths in majority of cases)

We could do that now as well. i.e. replace the toPath method in 
ValueLayout with dereferenceHandle, and just have a toPath method on 
CompoundLayout.

> * Usefulness of the LayoutPath API is more clearly defined inside the
> boundaries of the lambda bubble

Seems good, but there might also be cases where some base path needs to 
be shared to create multiple handles, e.g. when creating handles for 
every field of a struct, where I think this would have the opposite 
effect.

> * Which means we can define LayoutPath as a static interface inside
> Layout - e.g. Layout.Path - which is less likely to attract attention

Sounds good! But how about doing CompoundLayout.Path? Since paths only 
really seem to be useful for CompoundLayouts (and CompoundLayout is 
pretty small any ways).

Jorn

> Thoughts?
> 
> Maurizio
> 
> 
>> 
>> Jorn
>> 
>> On 2019-06-20 00:34, Maurizio Cimadamore wrote:
>>> Hi,
>>> now that we have combinators to create memory access handles, it 
>>> seems
>>> like there is less need to have a very rich API to describe layout
>>> paths. But, we still want some higher level way to create var handles
>>> (or compute offsets) from a complex layout expression...
>>> 
>>> Chatting with Brian and John, an idea emerged - to use a simple
>>> grammar to describe a layout path.
>>> 
>>> For instance, if you have the usual array of struct we used in other 
>>> examples:
>>> 
>>> [ 5 : [ x32 i32(value) ] ]
>>> 
>>> A path from the outermost sequence to the 'value' layout element can
>>> be described with the following string:
>>> 
>>> "[].value"
>>> 
>>> So, if we had a language to describe layout paths, we can replace the
>>> LayoutPath API with some methods which take a path expression as
>>> argument.
>>> 
>>> This is the patch:
>>> 
>>> http://cr.openjdk.java.net/~mcimadamore/panama/8226421/
>>> 
>>> I think the results are quite pleasing, and more compact than before.
>>> I also placed the VarHandle accessors where they belong - that is, an
>>> accessor taking a path expression in CompoundLayout. And then one 
>>> that
>>> just takes a carrier in ValueLayout (as no selection is possible from
>>> a leaf).
>>> 
>>> I think this takes care of the last 'shaky' abstraction in the memory
>>> access API.
>>> 
>>> Impl-wise, the expression parser builds on some of the work we've 
>>> done
>>> in the foreign branch - I've added a test which checks several 'bad'
>>> things that can happen during parsing.
>>> 
>>> I've also added a javadoc section on 'path expressions' - which shows
>>> the full grammar production for the path expression language.
>>> 
>>> One thing that I removed in the process was the ability to refer to
>>> struct components by index - in part because it felt a bit 
>>> unjustified
>>> (for now, at least), in part because it also felt a bit shaky (w.r.t.
>>> unions).
>>> 
>>> Cheers
>>> Maurizio