8323552: AbstractMemorySegmentImpl#mismatch returns -1 when comparing distinct areas of the same instance of MemorySegment #17354

plevart · 2024-01-10T20:54:20Z

I belive there is a bug in AbstractMemorySegmentImpl#mismatch method. It returns -1 (meaning that regions are equal) when passing the same instance of MemorySegment as both srcSegment and dstSegment parameters regardless of whether srcFromOffset and dstFromOffset as well as srcToOffset and dstToOffset are also equal.

Am I right?

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Error

⚠️ This PR only contains changes already present in the target

Issue

JDK-8323552: AbstractMemorySegmentImpl#mismatch returns -1 when comparing distinct areas of the same instance of MemorySegment (Bug - P4) ⚠️ Issue is already resolved. Consider making this a "backport pull request" by setting the PR title to Backport <hash> with the hash of the original commit. See Backports.

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/17354/head:pull/17354
$ git checkout pull/17354

Update a local copy of the PR:
$ git checkout pull/17354
$ git pull https://git.openjdk.org/jdk.git pull/17354/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 17354

View PR using the GUI difftool:
$ git pr show -t 17354

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/17354.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2024-01-10T20:55:55Z

👋 Welcome back plevart! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-01-10T20:57:01Z

@plevart The following label will be automatically applied to this pull request:

core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2024-01-10T21:12:32Z

Webrevs

mcimadamore · 2024-01-10T22:12:08Z

Note there's another bug for this: https://bugs.openjdk.org/browse/JDK-8323524

mcimadamore · 2024-01-10T22:12:55Z

src/java.base/share/classes/jdk/internal/foreign/AbstractMemorySegmentImpl.java

@@ -681,7 +681,7 @@ public static long mismatch(MemorySegment srcSegment, long srcFromOffset, long s
        long dstBytes = dstToOffset - dstFromOffset;
        srcImpl.checkAccess(srcFromOffset, srcBytes, true);
        dstImpl.checkAccess(dstFromOffset, dstBytes, true);
-        if (dstImpl == srcImpl) {
+        if (dstImpl.equals(srcImpl) && srcFromOffset == dstFromOffset && srcBytes == dstBytes) {


I'm also ok to completely drop this check (or move it into the instance version of the mismatch method). This also needs a test.

…re probable)

JornVernee · 2024-01-11T15:23:14Z

src/java.base/share/classes/jdk/internal/foreign/AbstractMemorySegmentImpl.java

+        if (
+            other instanceof AbstractMemorySegmentImpl that &&
+            unsafeGetBase() == that.unsafeGetBase() &&
+            unsafeGetOffset() == that.unsafeGetOffset() &&
+            byteSize() == that.byteSize()
+        ) {
+            this.checkAccess(0, this.byteSize(), true);
+            that.checkAccess(0, that.byteSize(), true);
+            checkValidState();
+            return -1;
+        }


I'm skeptical of trying to optimize this without a more thorough performance investigation. We shouldn't add this complexity on a whim.

I'm more in favor of just dropping the check from the static mismatch method and leaving it at that. (And adding a test as Maurizio mentioned).

I have similar thoughts. This is low-level API. Code using it has more context knowledge and is more fit to do optimisations like that or not, depending if they are worth it. I would want such low level API to do as little checks as possible to accommodate faster common case scenario.

bridgekeeper · 2024-02-08T16:54:45Z

@plevart This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

JornVernee · 2024-02-08T17:47:15Z

@plevart Are you planning to finish this? (add a test). Or maybe someone else can take over and do the rest?

openjdk · 2024-03-13T20:16:01Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

minborg · 2024-03-25T10:36:38Z

This PR can be closed @plevart as the issue has been fixed via #18426

bridgekeeper · 2024-04-22T13:55:53Z

@plevart This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

bridgekeeper · 2024-05-20T15:46:17Z

@plevart This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

fix mismatch avoidance of memory comparison

7bcf027

openjdk bot added the core-libs core-libs-dev@openjdk.org label Jan 10, 2024

plevart changed the title ~~Fix: AbstractMemorySegmentImpl#mismatch returns -1 when comparing distinct areas of the same instance of MemorySegment~~ 8323552: AbstractMemorySegmentImpl#mismatch returns -1 when comparing distinct areas of the same instance of MemorySegment Jan 10, 2024

openjdk bot added the rfr Pull request is ready for review label Jan 10, 2024

mcimadamore reviewed Jan 10, 2024

View reviewed changes

move special-case check for equal segments to the instance method (mo…

5112839

…re probable)

JornVernee reviewed Jan 11, 2024

View reviewed changes

remove special-case check from instance method - test pending

22417ea

openjdk bot removed the rfr Pull request is ready for review label Mar 25, 2024

bridgekeeper bot closed this May 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8323552: AbstractMemorySegmentImpl#mismatch returns -1 when comparing distinct areas of the same instance of MemorySegment #17354

8323552: AbstractMemorySegmentImpl#mismatch returns -1 when comparing distinct areas of the same instance of MemorySegment #17354

plevart commented Jan 10, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jan 10, 2024

openjdk bot commented Jan 10, 2024

mlbridge bot commented Jan 10, 2024 •

edited

Loading

mcimadamore commented Jan 10, 2024 •

edited by bridgekeeper bot

Loading

mcimadamore Jan 10, 2024 •

edited by bridgekeeper bot

Loading

JornVernee Jan 11, 2024

plevart Jan 11, 2024

bridgekeeper bot commented Feb 8, 2024

JornVernee commented Feb 8, 2024

openjdk bot commented Mar 13, 2024

minborg commented Mar 25, 2024

bridgekeeper bot commented Apr 22, 2024

bridgekeeper bot commented May 20, 2024

8323552: AbstractMemorySegmentImpl#mismatch returns -1 when comparing distinct areas of the same instance of MemorySegment #17354

8323552: AbstractMemorySegmentImpl#mismatch returns -1 when comparing distinct areas of the same instance of MemorySegment #17354

Conversation

plevart commented Jan 10, 2024 • edited by openjdk bot Loading

Progress

Error

Issue

Reviewing

Webrev

bridgekeeper bot commented Jan 10, 2024

openjdk bot commented Jan 10, 2024

mlbridge bot commented Jan 10, 2024 • edited Loading

Webrevs

mcimadamore commented Jan 10, 2024 • edited by bridgekeeper bot Loading

mcimadamore Jan 10, 2024 • edited by bridgekeeper bot Loading

Choose a reason for hiding this comment

JornVernee Jan 11, 2024

Choose a reason for hiding this comment

plevart Jan 11, 2024

Choose a reason for hiding this comment

bridgekeeper bot commented Feb 8, 2024

JornVernee commented Feb 8, 2024

openjdk bot commented Mar 13, 2024

minborg commented Mar 25, 2024

bridgekeeper bot commented Apr 22, 2024

bridgekeeper bot commented May 20, 2024

plevart commented Jan 10, 2024 •

edited by openjdk bot

Loading

mlbridge bot commented Jan 10, 2024 •

edited

Loading

mcimadamore commented Jan 10, 2024 •

edited by bridgekeeper bot

Loading

mcimadamore Jan 10, 2024 •

edited by bridgekeeper bot

Loading