[foreign-memaccess] RFR 8234814: Eager layout size computation trips on unbound sequence layouts

[Maurizio Cimadamore]

On 27/11/2019 09:21, Jorn Vernee wrote:
> Hi,
>
> Some comments:
>
> - SequenceLayout:
> This overrides badSizeExceptionMessage. I see the subtle difference in 
> text, but a sequence layout can also have an unspecified size if the 
> element layout has an unspecified size, in which case the message in 
> AbstractLayout would be more correct. You could check whether the 
> current sequence has an element count and return 
> super.badSizeExceptionMessage() in that case, though I think going 
> with one message that reads "Cannot compute the size of a layout which 
> is, or depends on a sequence layout with unspecified size" in 
> AbstractLayout would be good enough.
Fair point
>
> - Shouldn't the JShell fix should go into jdk/jdk instead?
Ideally yes - in practice the problem is so specific to VarHandles that 
we can go both ways.
>
> One observation:
>
> * Looking at the tests; I think something like 
> `MemorySegment.allocateNative(8 + 8 * 4)` would in practice be more 
> likely `MemorySegment.allocateNative(layout.byteSize() + 
> MemoryLayout.ofSequence(4, MemoryLayouts.JAVA_DOUBLE).byteSize())` 
> which makes me think we need a SequenceLayout::withElementCount(long) 
> method to create a new layout with an element count from a sequence 
> layout that doesn't have one:
>
>         SequenceLayout vla = 
> MemoryLayout.ofSequence(MemoryLayouts.JAVA_DOUBLE);
>         MemoryLayout header = MemoryLayout.ofStruct(
>                 MemoryLayouts.JAVA_INT.withName("size"),
>                 MemoryLayout.ofPaddingBits(32),
>                 vla.withName("arr"));
>         ...
>         try (MemorySegment segment = 
> MemorySegment.allocateNative(header.byteSize() + 
> vla.withElementCount(4).byteSize())) {
>         ...
>
> Especially when 'vla' in the above example could be a lot more 
> complex. What do you think?

This is essentially a restricted version of 'pluggable layouts with size 
holes'. I don't mind the suggestion too much, although I'm a bit worried 
it could be perceived as half-way there - e.g. another way to skin the 
cat would be to take 'header' and inject sizes into that one (perhaps 
using layout paths). E.g.

header.withSize(4, PathElement.groupLayout("arr"))

(name TBD, of course, but I hope you get what I mean)

Maurizio

>
> Cheers,
> Jorn
>
> On 26/11/2019 16:03, Maurizio Cimadamore wrote:
>> After a discussion offline with Jorn I've decided to revert the 
>> changes to constants:
>>
>> cr.openjdk.java.net/~mcimadamore/panama/8234814_v3
>>
>> As some of the tests demonstrate, among the original goals of these 
>> JAVA_INT constants and friends was interop with ByteBuffers, hence 
>> the endianness set to BE.
>>
>> I'll leave it as is for now (after all, the behavior is documented). 
>> We can revise later, if needs be.
>>
>> Maurizio
>>
>> On 26/11/2019 14:46, Maurizio Cimadamore wrote:
>>>
>>> On 26/11/2019 14:40, Maurizio Cimadamore wrote:
>>>> * constants such as JAVA_INT should use native order (this is, 
>>>> after all, the layout of an int in the VM) 
>>>
>>> To expand a bit - we eventually would like to see these constants 
>>> moved in the primitive wrapper classes - e.g.
>>>
>>> MemoryLayout::JAVA_INT -> Integer::LAYOUT
>>>
>>> in which case I think these constants should return whatever layout 
>>> the VM thinks a Java int (in this case) has.
>>>
>>> Maurizio
>>>