Add MemorySegment::mismatch #180
Conversation
|
|
Webrevs
|
|
Looks good - I've added some comments. Test is very comprehensive - thanks! |
...incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegment.java
Outdated
Show resolved
Hide resolved
...oreign/share/classes/jdk/internal/foreign/AbstractMemorySegmentImpl.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| for (; off < minSize; off++) { | ||
| if (UNSAFE.getByte(this.base(), this.min() + off) != UNSAFE.getByte(that.base(), that.min() + off)) { |
mcimadamore
May 20, 2020
Collaborator
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 MemoryAddresslocal variable in the loop instead of theoffsetand keep increasing that address on each iteration ofArraySupport::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.
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 MemoryAddresslocal variable in the loop instead of theoffsetand keep increasing that address on each iteration ofArraySupport::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(dataProvider = "slices") | ||
| public void testSameValues(MemorySegment ss1, MemorySegment ss2) { | ||
| out.format("testSameValues s1:%s, s2:%s\n", ss1, ss2); | ||
| MemorySegment s1 = initializeSegment(ss1); | ||
| MemorySegment s2 = initializeSegment(ss2); | ||
|
|
||
| if (s1.byteSize() == s2.byteSize()) { | ||
| assertEquals(s1.mismatch(s2), -1); // identical | ||
| assertEquals(s2.mismatch(s1), -1); | ||
| } else if (s1.byteSize() > s2.byteSize()) { | ||
| assertEquals(s1.mismatch(s2), s2.byteSize()); // proper prefix | ||
| assertEquals(s2.mismatch(s1), s2.byteSize()); | ||
| } else { | ||
| assert s1.byteSize() < s2.byteSize(); | ||
| assertEquals(s1.mismatch(s2), s1.byteSize()); // proper prefix | ||
| assertEquals(s2.mismatch(s1), s1.byteSize()); | ||
| } | ||
| } | ||
|
|
||
| @Test(dataProvider = "slices") | ||
| public void testDifferentValues(MemorySegment s1, MemorySegment s2) { | ||
| out.format("testDifferentValues s1:%s, s2:%s\n", s1, s2); | ||
| s1 = initializeSegment(s1); | ||
| s2 = initializeSegment(s2); | ||
|
|
||
| for (long i = s2.byteSize() -1 ; i >= 0; i--) { | ||
| long expectedMismatchOffset = i; | ||
| BYTE_HANDLE.set(s2.baseAddress().addOffset(i), (byte) 0xFF); | ||
|
|
||
| if (s1.byteSize() == s2.byteSize()) { | ||
| assertEquals(s1.mismatch(s2), expectedMismatchOffset); | ||
| assertEquals(s2.mismatch(s1), expectedMismatchOffset); | ||
| } else if (s1.byteSize() > s2.byteSize()) { | ||
| assertEquals(s1.mismatch(s2), expectedMismatchOffset); | ||
| assertEquals(s2.mismatch(s1), expectedMismatchOffset); | ||
| } else { | ||
| assert s1.byteSize() < s2.byteSize(); | ||
| var off = Math.min(s1.byteSize(), expectedMismatchOffset); | ||
| assertEquals(s1.mismatch(s2), off); // proper prefix | ||
| assertEquals(s2.mismatch(s1), off); | ||
| } | ||
| } | ||
| } |
mcimadamore
May 20, 2020
Collaborator
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).
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).
ChrisHegarty
May 20, 2020
Author
Member
Originally I had a version of the test that did compare specific segments, but it didn't scale well to different sizes and kinds ( we need to test both above and below the 8 byte threshold ). I removed a number of slice sizes, which greatly reduces the combinations. This may be enough, or I can certainly revisit the test's structure.
Originally I had a version of the test that did compare specific segments, but it didn't scale well to different sizes and kinds ( we need to test both above and below the 8 byte threshold ). I removed a number of slice sizes, which greatly reduces the combinations. This may be enough, or I can certainly revisit the test's structure.
|
Very nice. Improving the JDK implementation is good. In fact i think you could so that now. In Then you can specialize mismatch of memory segments for length threshold and type (the threshold only really makes sense for the first check, once you go over |
|
@ChrisHegarty This change now passes all automated pre-integration checks, type
Since the source branch of this PR was last updated there have been 143 commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid automatic rebasing, please merge As you do not have Committer status in this project, an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@mcimadamore, @PaulSandoz) but any other Committer may sponsor as well.
|
|
Mailing list message from Maurizio Cimadamore on panama-dev: On 20/05/2020 16:36, Paul Sandoz wrote:
To be clear - are you suggesting to add vectorizedMismatchLarge inside Maurizio |
|
Mailing list message from Paul Sandoz on panama-dev: Yes, add the ?large? strip mining implementation to ArraysSupport. I think it unlikely this method ever needs to be made intrinsic. For large loop bounds it's important that we don?t starve the system (allow for safe points), so it?s easier to do that from Java than in the intrinsic stub or in C2. Then the memory segment code can use the those primitives based on thresholds. I suspect it can get away with just calling the large version, after a check for a small threshold value and a scalar loop. Paul.
|
src/java.base/share/classes/jdk/internal/util/ArraysSupport.java
Outdated
Show resolved
Hide resolved
|
Looks good |
| return i; | ||
| } | ||
| } | ||
| return thisSize != thatSize ? length : -1; |
mcimadamore
May 20, 2020
Collaborator
I guess I'm a bit confused - shouldn't this method return (as per javadoc), -1 if there's no mismatch? In this code path we found no mismatch, and yet, if sizes are different we return length, suggesting there's a mismatch. I now realize that mismatch is taken quite literally - e.g. no mismatch really means the two things are identical in contents and size --- which is, I realize, what Arrays::mismatch also does.
IMHO the javadoc of the various mismatch routines could use more clarity around what a mismatch is. But maybe that's something for another day.
I guess I'm a bit confused - shouldn't this method return (as per javadoc), -1 if there's no mismatch? In this code path we found no mismatch, and yet, if sizes are different we return length, suggesting there's a mismatch. I now realize that mismatch is taken quite literally - e.g. no mismatch really means the two things are identical in contents and size --- which is, I realize, what Arrays::mismatch also does.
IMHO the javadoc of the various mismatch routines could use more clarity around what a mismatch is. But maybe that's something for another day.
|
Mailing list message from Paul Sandoz on panama-dev: If the two segments are are different lengths and one segment is a proper prefix of the other (all elements are equal) there is a mismatch in the length. It?s described more formally in the JavaDoc of the Arrays.mismatch methods. e.g * <p>If the two arrays share a common prefix then the returned index is the Paul. |
|
The proposed |
|
/integrate |
|
@ChrisHegarty |
|
Looks good! |
|
/sponsor |
|
@mcimadamore The PR has been updated since the change author (@ChrisHegarty) issued the |
|
/integrate |
|
@ChrisHegarty |
|
/sponsor |
|
@mcimadamore @ChrisHegarty The following commits have been pushed to foreign-memaccess since your change was applied:
Your commit was automatically rebased without conflicts. Pushed as commit cd39750. |
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.
Progress
Reviewers
Download
$ git fetch https://git.openjdk.java.net/panama-foreign pull/180/head:pull/180$ git checkout pull/180