[foreign-memaccess+abi] RFR: 8274157: java.foreign: Add method MemorySegment::isOverlapping

[Maurizio Cimadamore]

On Tue, 28 Sep 2021 12:59:19 GMT, Julia Boes <jboes at openjdk.org> wrote:

> This change adds a method to the MemorySegment API that detects if two segments are overlapping.

Good work. As I was going through the changes, it occurred to me that another way to stack this API is to return the slice that is common to both segments (or `null` if none exist). I don't know which version would be more interesting; any comment on this @leerho?

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegment.java line 324:

> 322: 
> 323:     /**
> 324:      * Returns true if this segment overlaps the {@code other} segment.

We should probably explain what "overlaps" mean. E.g. "Two segments S1 and S2 are said to overlap if it is possible to find at least two slices L1 (from S1) and L2 (from S2) such that L1 and L2 are backed by the same memory region".

This description is vague enough (as it is defined in terms of slicing). At the same time it makes it clear as to why an heap segment can never overlap with an heap segment (and vice-versa).

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegment.java line 326:

> 324:      * Returns true if this segment overlaps the {@code other} segment.
> 325:      *
> 326:      * If the two segments are not either both backed by native or heap memory, {@code false} is returned.

If you like the above suggestion, I'd replace this para with:

"As such, it is never possible for a native (javadoc link) segment to overlap with an heap segment".

We can drop the sentence about offset and sizes - as that seems more of an impl detail? But perhaps, if you want to explain more, a couple of examples would be better? E.g.


MemorySegment s1 = MemorySegment.allocateNative(100, scope);
MemorySegment s2 = MemorySegment.allocateNative(100, scope);
s1.overlap(s2); // false
MemorySegment s3 = s1.slice(0, 50);
s1.overlap(slice); // true
s2.overlap(slice); // false

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegment.java line 330:

> 328:      *
> 329:      * @param other the segment to be tested for an overlap with this segment.
> 330:      * @return {@code true} if this segment overlaps the other segment.

Suggestion:

     * @return {@code true} if this segment overlaps with the other segment.

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/AbstractMemorySegmentImpl.java line 257:

> 255: 
> 256:     @Override
> 257:     public boolean isOverlapping(MemorySegment other) {

maybe add final, so that other segment subclasses cannot accidentally override?

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/AbstractMemorySegmentImpl.java line 260:

> 258:         AbstractMemorySegmentImpl that = (AbstractMemorySegmentImpl)Objects.requireNonNull(other);
> 259:         if (base() == that.base()) {  // both are either native or heap
> 260:             this.checkAccess(0, this.length, true);

I don't think `checkAccess` is needed? That operation is typically called when accessing a segment to dereference, to verify that:
* the accessed location is within the bounds of the segment
* the access mode (read/write) is compatible with the segment
Here, you don't do dereference, so these checks are not necessary, `checkValidState()` is what you want.

test/jdk/java/foreign/TestSegmentsOverlap.java line 42:

> 40: 
> 41:     @Test
> 42:     public void testNativeBasic() {

there are two kinds of native segments (or even three): segments created via `MemorySegment::allocateNative` and segments created via `MemorySegment.mapFile`. Perhaps it could be worth testing both (I believe `TestByteBuffer` has an example of a mapped segment).

Additionally, we have native segments which come from a direct buffer:


ByteBuffer b = ByteBuffer.allocateDirect(10);
MemorySegment s = MemorySegment.ofBuffer(b); // native segment!

-------------

PR: https://git.openjdk.java.net/panama-foreign/pull/585