[foreign-memaccess] API refinements

[Maurizio Cimadamore]

On 06/01/2020 20:03, Paul Sandoz wrote:
>
>
>> On Dec 18, 2019, at 10:41 AM, Maurizio Cimadamore 
>> <maurizio.cimadamore at oracle.com 
>> <mailto:maurizio.cimadamore at oracle.com>> wrote:
>>
>> Hi,
>> during the code review for the foreign memory access API (see [1]), 
>> several issues were raised; these are now tracked in [2].
>>
>> Before doing any code change, I'd like to get some shared consensus 
>> on the direction we should take. Here is a list of the issues:
>>
>> * Should MemoryAddress implement Comparable?
>>
>> I believe the answer here should be NO.
>
> Ok.
>
>
>> In general comparing addresses is tricky - and it seems to me that, 
>> given that MemoryAddress has a well-defined equals() behavior, you 
>> can still implement e.g. loops where you want to keep offsetting an 
>> address until you reach a certain limit.
>>
>
> Yes. It’s possible to write your own comparators.
>
>
>> As to whether MemorySegment should implement Comparable, I believe 
>> answer should also be NO. I know that ByteBuffer implements 
>> Comparable, and it has method to compare the contents, and to find 
>> the first index at which two byte buffers mismatch. But I think this 
>> is a very marginal use case for the memory access API, and there's 
>> always an escape hatch: project segments down to a byte buffer and 
>> take it from there.
>>
>
> Agreed.  Useful comparison likely requires knowledge of the structure. 
>  A more useful lower abstraction is maybe mismatch, which identifies 
> the relative offset pointing to the first mismatching byte of two 
> segments.  We already have efficient building blocks for this. Perhaps 
> something to consider later on if there is demand.
>
>
>
>> * should we merge MemoryLayouts and MemoryLayout?
>>
>> I'm on the fence on this one. The fact that we have a MemoryLayouts 
>> is very much driven by the fact that the API lives in an incubating 
>> module, and that some layouts cannot live where they should (e.g. 
>> JAVA_INT should really be Integer::LAYOUT). Since we also have a 
>> MemoryHandles (which will be replaced by some methods inside 
>> MethodHandles), I really don't see a big reason as to why we should 
>> refactor the API at this point. E.g. this seems to be more a question 
>> that we should ask again when the API is finalized, and exits incubation.
>>
>
> Yes, hold off until suitably incubated.
>
>
>> * MemorySegment::acquire() -- replace with MemorySegment::fork?
>>
>> I came up with that - and I think I still like it. Acquire seems to 
>> suggest that it has something to do with threads and ownership - 
>> which it does, but in reality you can acquire a segment even from 
>> same owner. Fork seems a more neutral term, and, to me helps better 
>> visualizing what's really going on (e.g. a tree of segments is being 
>> created).
>>
>
> Not sure about fork, it reminds me of processes or fork-join, 
> something that is task oriented, overloaded term… but I think it could 
> grow on me.
>
> Re-reading the JavaDoc, we could refer to it as a confined view and 
> call the method asConfined and s/acquired memory segment/confined view 
> memory segment”.

Not too sure. For two reasons: (i) it seems to suggest that the segment 
it came from it wasn't confined (they all are, in a way) and, more 
importantly (ii) because the 'asXYZ' naming convention implies a 'dumb' 
view which, if closed, closes everything, which is not the case here 
(this is why slice() has been renamed to asSlice()).

So, while we could call it confine(), the problem is that this method is 
effectively doing two things:

1) create a new sub-segment whose life-cycle controls that of the parent
2) give a new confinement owner to the new sub-segment

Seems to me than names like acquire() and confine() suggest a string 
bias for (1), whereas fork() is probably biased for (2).

The problem with bias is that names like acquire() or confine() make 
perfect sense in a share context, but makes little sense when used in a 
single thread context, where you might want to use the acquire mechanism 
as a way to e.g. make sure that you can't close the parent segment 
before all the slices have been closed. In those case a name like fork() 
works better.

I'm fine with keeping acquire() if nobody comes up with a better 
alternative.

> I can convince myself that confinement also fits reasonably well with 
> regards to temporal bounds :-)
>
> The action of close on a confined view memory segment is temporally 
> confined since it does not close the source/original segment, further 
> the source is also temporally confined until all confined views are 
> all closed.  The view and source are both thread confined and 
> temporally confined.  That might make it easier to think about such 
> usages within the current thread to support 
> asConfined().asSlice(…).close();
>
> Separately, perhaps we still need the notion of thread ownership 
> transfer? e.g. hand off this segment to another thread. The name 
> “acquire” might be a better fit in that regard.  But I think this 
> needs more careful thought.
We have left that off for now, and see what concrete use cases are.
>
>
>> * should we have a way to ask a Layout if its size is specified ?
>>
>> This is a tricky one. On the one hand, if not all layouts have a 
>> size, one could argue that Layout::bitSize should return an optional. 
>> But that seems excessively punitive for the vast majority of layouts 
>> that are never going to use unbound sequences. This might leads down 
>> to two conclusions:
>>
>> - maybe unbound sequences do not carry enough weight - perhaps 
>> replace them with a zero-length sequence, and be done - or even get 
>> rid of zero-length sequences and let layout only be for things whose 
>> size is defined. (and maybe come back with a better mechanism to 
>> speak about "size holes" later, and variable-length layouts in general)
>>
>> - if we keep unbound sequences, let's just add an hasSize() predicate
>>
>> Of the two I'm warming up with just ditch unbound sequences period 
>> (e.g. all layouts must have a well-defined size).
>>
>
> It seems you have address this with your cleanup patch (RFR 8236267: 
> Cleanup support for layouts with undefined size)?
Yep
>
>
>> * There doesn't seem to be a predicate for "can I call close without 
>> an exception?" (isAlive && isAccessible) isn't sufficient.
>>
>> I think the answer here is - and that's FINE. I'm of the opinion that 
>> if an exception occurs when calling close(), 90% it signals a bug in 
>> the code, not just something that you want to proactively check 
>> against. E.g. somebody else forked your segment w/o you knowing. On 
>> top of that, I don't think that, even if we wanted to, there would be 
>> a way to do a 'canClose()' check which would mean anything - that is, 
>> the predicate could return true, but then, what if, just before you 
>> call close() another thread re-acquires the segment? I think this is 
>> a lost cause.
>>
>
> Agreed.
>
>
>> * MemoryAddress::offset() vs. MemoryAddress::offset(long) -- not much 
>> distance between these two semantically different operations
>>
>
> s/offset(long l)/relativeTo(long offset) ?
>
> "Returns a new address relative to the given offset from this address. 
>  The offset of the new address is the sum of the offset of this 
> address plus the given offset.  The given offset may be negative.”
>
>
>> I think the solution that I like here is to drop 
>> MemoryAddress::offset() completely, and to provide a new 
>> functionality to create a slice based on a given address (which is 
>> the primary motivation for having the offset() method). Here we have 
>> two options:
>>
>> - MemoryAddress::asSlice(long bytes) -> MemorySegment
>> - MemorySegment::asSlice(MemoryAddress base, long bytes)
>>
>> The former leads to more compact code, but it feels a bit dirtier - 
>> anyway, I don't feel too strongly here.
>>
>> * MemorySegment::isAccessible() -- as the A* word is overloaded, some 
>> other name should be found?
>>
>> Yes, the A-word is a mess - but some of the proposals I've see (e.g. 
>> "isOwnedByCurrentThread") look equally terrible. If somebody has a 
>> better alternative I'd be happy to hear it - if not, I think keeping 
>> the A-word is tolerable.
>>
>
> Seems ok to me.  Another alternative is to expose a method returning 
> the owner thread and the developer doing:
>
> seg.ownerThread() == Thread.currentThread()

Yeah - that is something that came up before - I'll go for that

Thanks
Maurizio

>
> Paul.
>
>>
>> Thoughts?
>>
>> Maurizio
>>
>> [1] - 
>> https://mail.openjdk.java.net/pipermail/core-libs-dev/2019-December/063836.html
>> [2] - https://bugs.openjdk.java.net/browse/JDK-8235837
>>
>