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

[Maurizio Cimadamore]

Had a chat with Alan,
there's no way to unmap a MBB - but what other tests do is something 
like this

             var ref = new WeakReference<>(mbb);
             mbb = null;
             System.gc();
             while (ref.get() == null) {
                 Thread.sleep(20);
             }


I'll add this.

Thanks
Maurizio

On 28/05/2019 14:20, Jorn Vernee wrote:
> 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