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

[Jorn Vernee]

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

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