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

[Jorn Vernee]

On 2019-07-11 16:31, Maurizio Cimadamore wrote:
> So, on second thought, maybe just moving the method from MA to MS is
> all we need? (in the name of a better abstraction mapping)

Yeah OK, that sounds good. FWIW, I think moving the method makes the 
implementation of asByteBuffer a bit cleaner as well, since it was 
already mostly operating on the MemoryAddress' segment.

> As for sliceAt - I'm not sure. You can always get to the base address
> of a segment of any given memory address. To solve the cases you are
> thinking of, you can go two ways: add sliceAt - which allows you to
> remain in the MA world - or add a MemoryAddress::offset() method which
> returns a long (this should be a safe operation, as the offset is
> relative to the segment anyway), and then feed that back into
> MemorySegment::slice
> 
> Of the two, the latter feels simpler?

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()?

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