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

Wed May 29 09:58:43 UTC 2019

Looks very good!

Cheers,
Jorn

[1] : 
https://javadoc.lwjgl.org/org/lwjgl/opengl/GL15.html#glBufferData(int,java.nio.FloatBuffer,int)

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