[foreign-memaccess] RFR 8227394: Add MemorySegment::asByteBuffer convenience method

Fri Jul 12 15:26:55 UTC 2019

Here is the updated version: 
http://cr.openjdk.java.net/~jvernee/panama/webrevs/8227394/webrev.04/

* Moved asByteBuffer to MemorySegment
* Added an offset() getter to MemoryAddress
* Updated tests to use the new asByteBuffer where possible (all but one)
* Renamed ForeignUnsafe methods to getUnsafeOffset and getUnsafeBase

FWIW, negative offsets are required in some cases, so I think offset() 
is better than advance().

Jorn

On 2019-07-11 18:55, Maurizio Cimadamore wrote:
> On 11/07/2019 17:51, Maurizio Cimadamore wrote:
>> 
>>> This sounds good as well.
>>> 
>>> I was thinking about the lump/split discussion around MA and MS, 
>>> particularly where we draw the lines of responsibility between the 
>>> two. I think MS should be used to manage a memory resource 
>>> (allocating, slicing, closing), while MA should simply be used to 
>>> access the resource (dereferencing, offset). So, after all, I think 
>>> any method for slicing the segment (i.e. managing the resource) 
>>> should be in MS as well.
>>> 
>>> I'm not so sure about the name offset() though, since that seems to 
>>> clash a bit with ForeignUnsafe::getOffset(), and 
>>> MemoryAddress::offset(long). Maybe an alternative is offsetOf(), or 
>>> toSegmentOffset()?
>> 
>> We have other cases in the JDK where the same name is used for getter 
>> and setter (e.g. ByteBuffer::order). So I'm not overly worried about 
>> that, and I still prefer that that using get/set.
> 
> Actually, virtually all methods here
> 
> https://docs.oracle.com/en/java/javase/12/docs/api/java.base/java/nio/Buffer.html
> 
> use that pattern. So, while not super-ideal, I think that could be 
> justified.
> 
> As for the unsafe methods, I think we should rename them to
> getUnsafeOffset and getUnsafeBase
> 
> Maurizio
> 
> 
>> 
>> Another option worth looking into would be, IMHO, to explore 
>> alternatives for the offset(long) method. In a way it feels the use of 
>> offset is ambiguous here, it is both a noun (which justifies the 
>> getter semantics) and a verb (which justifies the setter/wither 
>> semantics). For instance, one possible renaming for offset(long) is 
>> advance/move
>> 
>> address.advance(42).offset();
>> 
>> I think that reads well? (also implies that negative offsets are kind 
>> of not accepted?)
>> 
>> Maurizio
>> 
>>> 
>>> Jorn
>>> 
>>>> Maurizio
>>>> 
>>>> 
>>>>> 
>>>>> Like you say, by adding a MemorySegment::asByteBuffer we introduce 
>>>>> a problem because some segments are too large, but this seems 
>>>>> pretty similar to being able to pass a size to 
>>>>> MemoryAddress::asByteBuffer that is too large, which is still 
>>>>> possible even if it's an `int`. At the same time we have one less 
>>>>> problem because we can no longer use a MemoryAddress that is out of 
>>>>> bounds of the segment.
>>>>> 
>>>>> It feels like a pretty much even trade.
>>>>> 
>>>>>> I'm less convinced on the
>>>>>> usefulness of turning an entire memory segment into a bytebuffer: 
>>>>>> if
>>>>>> you really wanted to view the entire memory segment as a BB, maybe 
>>>>>> you
>>>>>> just wanted to create a (direct) BB in the first place?
>>>>> 
>>>>> By that same line of thought, do we need 
>>>>> MemoryAddress::asByteBuffer? This is meant as an interop thing, 
>>>>> where you just have a MemorySegment that is given to you, and would 
>>>>> like to turn it into a ByteBuffer.
>>>>> 
>>>>> I agree with the concern of the API explosion. I was also looking 
>>>>> at removing the asByteBuffer method from MemoryAddress, and 
>>>>> replacing it with a more general 'sliceAt' method, that slices the 
>>>>> underlying MemorySegment at the offset denoted by the MemoryAddress 
>>>>> itself (instead of having to pass that manually as the offset to 
>>>>> MemorySegment::slice). I could imagine cases where a user has a 
>>>>> MemoryAddress and wants to do an offset and then slice, e.g. I have 
>>>>> a pointer to some struct, returned by some third party allocator 
>>>>> API, and want to give someone else a pointer to access just a 
>>>>> single field, I can try and call `address.segment().slice(...)`, 
>>>>> but the problem would be that I don't know what to pass as offset 
>>>>> (after all, the MemoryAddress I have might not be the base address 
>>>>> of the segment).
>>>>> 
>>>>> With a `sliceAt` method, the current use of asByteBuffer(int) could 
>>>>> be replaced with `address.sliceAt(newSize).asByteBuffer()`.
>>>>> 
>>>>> Prototype: 
>>>>> http://cr.openjdk.java.net/~jvernee/panama/webrevs/8227394/webrev.01/
>>>>> 
>>>>> What do you think? Of course, if you'd rather hold off on fiddling 
>>>>> with the API too much at this time that's fine with me :) (I'm 
>>>>> guessing we're trying to target JDK 14 as an incubating feature?)
>>>>> 
>>>>> Jorn
>>>>> 
>>>>> On 2019-07-11 14:00, Maurizio Cimadamore wrote:
>>>>>> Hi Jorn,
>>>>>> I gave some thought about this in the past, and my conclusion was
>>>>>> mixed - and I filed this in the "you ain't gonna need it" bucket
>>>>>> (YAGNI).
>>>>>> 
>>>>>> The motivations are that (i) it is really simple to emulate the 
>>>>>> API
>>>>>> with the existing methods (as the implementation of the new method
>>>>>> shows), and that (ii) the new API is, by necessity, a partial API 
>>>>>> -
>>>>>> not all segments can be mapped onto a byte buffer - some are too
>>>>>> large; while the existing API on memory address is total (thanks 
>>>>>> to
>>>>>> the explicit int parameter).
>>>>>> 
>>>>>> In other words, the existing API on MemoryAddress supports what I
>>>>>> believe to be the most important interop idiom: slicing a big 
>>>>>> memory
>>>>>> segment into multiple < 32bits buffer views. I'm less convinced on 
>>>>>> the
>>>>>> usefulness of turning an entire memory segment into a bytebuffer: 
>>>>>> if
>>>>>> you really wanted to view the entire memory segment as a BB, maybe 
>>>>>> you
>>>>>> just wanted to create a (direct) BB in the first place?
>>>>>> 
>>>>>> The current model is kind of easy to explain - the moves we allow 
>>>>>> are:
>>>>>> 
>>>>>> * ByteBuffer -> MemorySegment
>>>>>> 
>>>>>> * MemoryAddress + int -> ByteBuffer
>>>>>> 
>>>>>> This feels minimal. If we add another dimension, as per your 
>>>>>> patch:
>>>>>> 
>>>>>> * MemorySegment -> ByteBuffer
>>>>>> 
>>>>>> Then the next question is - should we allow also:
>>>>>> 
>>>>>> * ByteBuffer -> MemoryAddress? (e.g. create a big segment from the
>>>>>> byte buffer and then have a memory address point at the BB offset)
>>>>>> 
>>>>>> Which feels a bit like a slippery slope. I'd be more confident in
>>>>>> adding a facility like this after some real world validation.
>>>>>> 
>>>>>> What do you think?
>>>>>> 
>>>>>> Maurizio
>>>>>> 
>>>>>> On 11/07/2019 12:28, Jorn Vernee wrote:
>>>>>>> Hi,
>>>>>>> 
>>>>>>> Please review this small patch that adds an asByteBuffer 
>>>>>>> convenience method to MemorySegment that returns a buffer the 
>>>>>>> size of the segment.
>>>>>>> 
>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8227394
>>>>>>> Webrev: 
>>>>>>> http://cr.openjdk.java.net/~jvernee/panama/webrevs/8227394/webrev.00/ 
>>>>>>> (note that the javadoc is largely taken from 
>>>>>>> MemoryAddress::asByteBuffer).
>>>>>>> 
>>>>>>> My reason for adding this is that I wanted to reach for this 
>>>>>>> method a few times when playing with the API, but it didn't 
>>>>>>> exist. So, I thought it would be useful to have.
>>>>>>> 
>>>>>>> (Also, I originally wanted to upload this to GitHub [1] to test 
>>>>>>> out the Skara [2] tools, but apparently that's not operational 
>>>>>>> yet :) )
>>>>>>> 
>>>>>>> Thanks,
>>>>>>> Jorn
>>>>>>> 
>>>>>>> [1] : https://github.com/openjdk/panama/pull/1
>>>>>>> [2] : https://github.com/openjdk/skara#openjdk-project-skara