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

[Jorn Vernee]

I'm getting an error on the new test:

TEST RESULT: Error. failed to clean up files after test

This is probably because a stream to a file is not closed (which is 
problematic for Windows), though I can't spot where that's happening. I 
assume a MappedByteBuffer is un-mapped when the FileChannel is closed? 
(there doesn't seem to be a close/unmap method on MBB).

Jorn

Maurizio Cimadamore schreef op 2019-05-28 14:32:
> 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