[foreign-memaccess] RFR 8223768: Rethink relationship between Sequence vs. Group layouts

[Maurizio Cimadamore]

On 13/05/2019 16:33, Jorn Vernee wrote:
> In that case I think removing Value::content sounds good as well. If 
> the focus is memory access, a part of the Layout API that can not be 
> used for that seems unneeded. Besides, this seems like something that 
> can be added back fairly easily later. Eventually we might even want 
> to choose a different implementation for this (e.g. sub-type of Group, 
> or something else).

Agree on both fronts - e.g. on dropping 'contents' for now, and on 
providing some 'facilities' to do bitmasking (e.g. some ways to build MH 
on top of the 'container' VH?) at a later point.

P.S.
For the case you were suggesting, I think what would be great would be 
to surface an int128 as an IntVector with 4 lanes, which then gives you 
all the primitives you need to access/express computation on the lane 
elements. But this needs to wait for Vector API - luckily we can add 
more carriers as we're more ready for it :-)

Maurizio

>
> Jorn
>
> Maurizio Cimadamore schreef op 2019-05-13 16:30:
>> On 13/05/2019 15:16, Jorn Vernee wrote:
>>>> But I also think that dereferencing sub-value
>>>> content should not be supported and best left to clients (as that
>>>> almost always involve some bitmasking and a different set of carriers
>>>> that is hard to be expressed in a single the API).
>>>
>>> I think supporting this is good, as long as the sub-value matches a 
>>> Java carrier. e.g. this would make it possible to split larger than 
>>> 64 bit values into separate parts: u128=[2:u64] and still make them 
>>> accessible that way, while keeping the semantic meaning that it's 
>>> actually a 128 bit value. For those cases no bitmasking is needed. 
>>> Clients could create their own high-level carrier which wraps a 
>>> `long` carrier VarHandle, and does multiple gets/sets on the 
>>> underlying VarHandle per high-level get/set.
>>
>> Not sure... it seems to me that if you have u128 = [2:u64] and you
>> make a VarHandle::compareAndSet - I think the desired semantics is
>> that atomic access occurs when reading the 128 bits from memory - but,
>> in reality, this will result in weaker access mode - e.g. there will
>> be atomic access only on the accessed 64 bits, which means another
>> client would be able to update the other 64 bits at the same time as
>> access is performed.
>>
>> In other words, the dereference operation should map into an
>> equivalent Unsafe::xyz operation which does the low level memory
>> access; there's no operation to do a read of 128 bits - so I don't
>> think we want to fake it.
>>
>> When we will have more carriers (e.g. Valhalla values) we will be able
>> to add more basic types to the dereference operation. Until then I
>> believe its better to punt.
>>
>> Your use case of u128=[2:u64] would really be better served by just
>> having [2:u64], which is actually the only semantics we can implement
>> with the VM plumbing we have available so far.
>>
>> Maurizio
>>
>>>
>>>> Regardless of the decision, I think the top type 'Compound' is an
>>>> useful abstraction.
>>>
>>> +1
>>>
>>> Jorn
>>>
>>> Maurizio Cimadamore schreef op 2019-05-13 15:52:
>>>> On 13/05/2019 14:40, Jorn Vernee wrote:
>>>>> Hi,
>>>>>
>>>>> One comment:
>>>>>
>>>>> - LayoutPathImpl::sequenceElement
>>>>>
>>>>>   Currently, looking up sequence elements as contents of a Value 
>>>>> does not work. e.g.
>>>>>
>>>>>   Value v = Value.ofUnsignedInt(64).withContents(Sequence.of(8, 
>>>>> Value.ofUnsignedInt(8)));
>>>>>   VarHandle vh = 
>>>>> v.toPath().sequenceElement().dereferenceHandle(byte.class); // 
>>>>> Boom! IllegalStateException in LayoutPathImpl::sequenceElement
>>>>>
>>>>>   If this case is not meant to be supported, I think the type of 
>>>>> Value::contents should just be Optional<Group> instead of 
>>>>> Optional<Sequence>.
>>>>
>>>> There are two parts to this: I think constructing paths to sub-value
>>>> contents should work. But I also think that dereferencing sub-value
>>>> content should not be supported and best left to clients (as that
>>>> almost always involve some bitmasking and a different set of carriers
>>>> that is hard to be expressed in a single the API).
>>>>
>>>> So, the idea is that deferencing a path that points inside the guts of
>>>> some value should throw some sane exception (I have some code which
>>>> makes this more explicit).
>>>>
>>>> Of course, when we get to a more general version of
>>>> compoundElement(long) which works on both sequences and groups, we
>>>> should make sure that, given your layout, this works:
>>>>
>>>> v.toPath().compoundElement(2);
>>>>
>>>> A separate question is: given we don't intend to support dereference
>>>> of value innards, should we keep the Value::contents API?
>>>>
>>>> Regardless of the decision, I think the top type 'Compound' is an
>>>> useful abstraction.
>>>>
>>>> Maurizio
>>>>
>>>>>
>>>>> Cheers,
>>>>> Jorn
>>>>>
>>>>> Maurizio Cimadamore schreef op 2019-05-13 14:14:
>>>>>> Hi,
>>>>>> as discussed previously, the relationship between sequences and 
>>>>>> groups
>>>>>> is a bit shaky, as there are sequences that cannot be expressed as
>>>>>> groups (sequences whose size is unbound).
>>>>>>
>>>>>> This patch models groups and sequences independently:
>>>>>>
>>>>>> * they all have a (minimal) common supertype called Compound (which
>>>>>> supports only a Stream<Layout> elements() method)
>>>>>>
>>>>>> * Group implements Iterable, always has a size, and you can access
>>>>>> elements by index
>>>>>>
>>>>>> * Sequence doesn't implement Iterable, has an optional size, and has
>>>>>> an accessor to retrieve the element layout (there's only one of 
>>>>>> them,
>>>>>> regardless of the size)
>>>>>>
>>>>>> I think this is much saner.
>>>>>>
>>>>>> Webrev:
>>>>>>
>>>>>> http://cr.openjdk.java.net/~mcimadamore/panama/8223768/
>>>>>>
>>>>>> P.S.
>>>>>> of course Compound should be renamed to CompoundLayout in a later
>>>>>> webrev (as for all other layout names).
>>>>>>
>>>>>> P.P.S.
>>>>>> in a separate patch I'll explore generalizing this method:
>>>>>>
>>>>>> LayoutPath::groupElement(long index)
>>>>>>
>>>>>> with this:
>>>>>>
>>>>>> LayoutPath::compoundElement(long index)
>>>>>>
>>>>>> Since now we should be able to implement indexed access on both 
>>>>>> group
>>>>>> and sequences.
>>>>>>
>>>>>> Maurizio