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

[Maurizio Cimadamore]

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