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

[Maurizio Cimadamore]

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