[foreign-memaccess] RFR: Add MemorySegment::mismatch

[Maurizio Cimadamore]

On Wed, 20 May 2020 14:06:48 GMT, Chris Hegarty <chegar at openjdk.org> wrote:

> Hi,
> 
> As part of feedback on the Foreign Memory API (when experimenting with its usage internally in the JDK), a small number
> of potential usability enhancements could be made to the API. This is the fourth such, and last on my current todo
> list.  This change proposes to add a new method:
> MemorySegment::mismatch
> 
> The mismatch semantic is very useful for building equality and comparison logic on top of segments. I found that I
> needed such when modeling and comparing native socket address in the JDK implementation. It is possible to write your
> own, but requires a non-trivial amount of not-trivial code - to do it right! I also think that we can provide a more
> efficient implementation building on top of the JDK's internal mismatch support.  I still need to do some perf testing
> and add a micro-benchmake ( Maurizio suggested possibly amending TestBulkOps ). There is also the question about
> possibly improving the JDK's internal implementation to work on long sizes (which could be done separately). For now, I
> just want to share the idea, along with the proposed specification and initial implementation.  Comments welcome.

Looks good - I've added some comments. Test is very comprehensive - thanks!

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

> 346:      * the smaller of the segment sizes, and it follows that the offset is only
> 347:      * valid for the larger segment. Otherwise, there is no mismatch.
> 348:      *

there is no mismatch and the returned value is (I think you say `-1` below, perhaps would be good to state that here
also).

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

> 141:
> 142:         this.checkRange(0, minSize, false);
> 143:         that.checkRange(0, minSize, false);

This is of course correct but also a bit redundant (there's a similar issue, although to a smaller extent in
`copyFrom`). Let's see how performance numbers are - basically this code can be simplified by omitting the bound check
on both segments (since you already picked a size that must work on both) - so that we just need confinement + read
access check.

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

> 174:         for (; off < minSize; off++) {
> 175:             if (UNSAFE.getByte(this.base(), this.min() + off) != UNSAFE.getByte(that.base(), that.min() + off)) {
> 176:                 return off;

This code could be simplified/rewritten to use `MemoryAddress' and VH, instead of unsafe access with object/offset
addressing. E.g. you could maintain a `MemoryAddress` local variable in the loop instead of the `offset` and keep
increasing that address on each iteration of `ArraySupport::vectorizedMismatch`. Then, when you get out of the loop,
the address already points at the base of the region to compare, and a simple for loop with an indexed VH should do the
job.

test/jdk/java/foreign/TestMismatch.java line 99:

> 98:         }
> 99:     }
> 100:

How important is it that these tests operate on slices? Looking at the test code, it could have worked equally well if
the input parameters were just two sizes, and then you did an explicit allocation (or maybe also receive a segment
factory from the provider, so that you can test different segment kinds).

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

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