[foreign-memaccess] RFR 8224843: Refine ByteBuffer interop support

[Maurizio Cimadamore]

And here's another:

http://cr.openjdk.java.net/~mcimadamore/panama/8224843_v3

This version moves all the code generation in the templates, as for 
other buffer - so that we can generate a ScopedBuffer class for each 
basic type - this allows us to implement buffer views (such as 
'asCharBuffer') cleanly.

I've also added a lot more robustness in the test, which now checks not 
only that the ByteBuffer projection works as expected, but also that the 
derived buffer views also work as expected.

Cheers
Maurizio

On 28/05/2019 13:32, Maurizio Cimadamore wrote:
> Updated webrev:
>
> http://cr.openjdk.java.net/~mcimadamore/panama/8224843_v2/
>
> Changes:
>
> * fixed byte buffer template
>
> * regularized logic for resize (now bytebuffer segments also get to 
> resize()!)
>
> * moved byte buffer segment into its own class
>
> * added several more test to check correctness of resize operation 
> with various segment kinds
>
> * opened up MemorySegment.ofArray to allow all primitive arrays (but 
> MemoryAddress.asByteBuffer will still throw if the base object is not 
> byte[])
>
> Maurizio
>
> On 28/05/2019 12:22, Maurizio Cimadamore wrote:
>> I'll check - thanks
>>
>> Maurizio
>>
>> On 28/05/2019 11:30, Jorn Vernee wrote:
>>> Hi,
>>>
>>> The patch doesn't compile for me:
>>>
>>> === Output from failing command(s) repeated here ===
>>> * For target jdk_modules_java.base__the.java.base_batch:
>>> h:\cygwin64\home\Jorn\cygwin-projects-new\memaccess\src\java.base\share\classes\java\nio\ScopedByteBuffer.java:41: 
>>> error: no suitable constructor found for ByteBuffer(no arguments)
>>>         super();
>>>         ^
>>>     constructor ByteBuffer.ByteBuffer(int,int,int,int,byte[],int) is 
>>> not applicable
>>>       (actual and formal argument lists differ in length)
>>>     constructor ByteBuffer.ByteBuffer(int,int,int,int) is not 
>>> applicable
>>>       (actual and formal argument lists differ in length)
>>> h:\cygwin64\home\Jorn\cygwin-projects-new\memaccess\src\java.base\share\classes\java\nio\ScopedByteBuffer.java:477: 
>>> error: put(byte[]) in ScopedByteBuffer cannot override put(byte[]) 
>>> in ByteBuffer
>>>     public ByteBuffer put(byte[] src) {
>>>                       ^
>>>   overridden method is final
>>> h:\cygwin64\home\Jorn\cygwin-projects-new\memaccess\src\java.base\share\classes\java\nio\ScopedByteBuffer.java:484: 
>>> error: hasArray() in ScopedByteBuffer cannot override hasArray() in 
>>> ByteBuffer
>>>    ... (rest of output omitted)
>>>
>>> * All command lines available in 
>>> /cygdrive/h/cygwin64/home/Jorn/cygwin-projects-new/memaccess/build/windows-x86_64-server-release/make-support/failure-logs.
>>> === End of repeated output ===
>>>
>>> I see that in the patch you made the necessary changes to 
>>> java.nio.Buffer, but shouldn't these changes be made to the template 
>>> file for ByteBuffer as well? Since ScopedByteBuffer extends ByteBuffer?
>>>
>>> Jorn
>>>
>>> Maurizio Cimadamore schreef op 2019-05-27 19:45:
>>>> Hi,
>>>> as you know, the foreign memory access API supports bidirectional
>>>> interop between MemoryAddress and ByteBuffer:
>>>>
>>>> 1) MemorySegment::ofByteBuffer(ByteBuffer)
>>>> 2) MemoryAddress:;asDirectByteBuffer(int bytes)
>>>>
>>>> That is (1) can be used to create a memory segment out of an existing
>>>> byte buffer, whereas (2) can be used to do the opposite, that is, to
>>>> convert a memory address into a byte buffer.
>>>>
>>>> While (1) works pretty reliably (but I've added some tests for it),
>>>> the implementation for (2) leaves to be desired:
>>>>
>>>> * The resulting byte buffer is unaware of the fact that the backing
>>>> memory is attached to a scope that can be closed
>>>> * There's no way to create a buffer if the address encapsulates some
>>>> heap-based memory address
>>>>
>>>> This patch solves both issues - and also adds a supported way for
>>>> creating a memory segment out of a heap-allocated byte array
>>>> (MemorySegment.ofArray(byte[])) which I think is useful.
>>>>
>>>> To solve the scope awareness issue I put together (after discussing
>>>> extensively with Alan, CC'ed) a delegating wrapper of ByteBuffer,
>>>> which delegates to the underlying buffer implementation after doing a
>>>> liveness check on the owning scope. That means that operations such as
>>>> ByteBuffer::getInt will only succeed if the scope the view is
>>>> associated with is still alive. Note that we need the wrapping only if
>>>> the associated scope is _not_ pinned - in fact, if the scope is
>>>> pinned, then it can't be closed, so there's no need to check for
>>>> liveliness.
>>>>
>>>> To do this I had to remove the 'final' modifier from some instance
>>>> methods in ByteBuffer and Buffer - the theory here is that these
>>>> 'final' have been added in early days to help the VM out, but are not
>>>> necessary now (also note that is not possible for a client to create a
>>>> custom byte buffer implementation, as all constructors are
>>>> package-private).
>>>>
>>>> Finally, the wrapped byte buffer implementation does not support
>>>> typeful views such as 'asCharBuffer' - this is tricky to support as
>>>> CharBuffer is not a subtype of ByteBuffer so, to go down that path
>>>> we'd need to wrap also CharBuffer and friends, which is doable, but
>>>> we're leaning towards YAGNI for now.
>>>>
>>>> Finally I've added a test which writes into memory (using var handles)
>>>> then reads it back using a segment-backed byte buffer (there are tests
>>>> for both heap and off-heap variants of the buffer). There's also a
>>>> test which checks interop between MappedByteBuffer and the foreign API
>>>> (which will likely be relevant in the context of JEP 352 [1]), and,
>>>> finally, a test which makes sure that all instance method in the
>>>> scope-wrapped buffer throws ISE after the scope has been closed.
>>>>
>>>> Webrev:
>>>>
>>>> http://cr.openjdk.java.net/~mcimadamore/panama/8224843/
>>>>
>>>> Comments welcome!
>>>>
>>>> Cheers
>>>> Maurizio
>>>>
>>>> [1] - https://openjdk.java.net/jeps/352