[foreign-memaccess] API refinements

[Paul Sandoz]

> On Dec 18, 2019, at 10:41 AM, Maurizio Cimadamore <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”.

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.


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


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

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
>