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

[Maurizio Cimadamore]

I've fixed the exception, removed jshell changes (see [1]) and pushed.

We can discuss separately on the opportunity to add new API points to 
make it easier to deal with VLAs.

Thanks
Maurizio

[1] - 
https://mail.openjdk.java.net/pipermail/kulla-dev/2019-November/002446.html

On 27/11/2019 09:39, Maurizio Cimadamore wrote:
>
> 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
>>>>