-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enable MemorySegment in MMapDirectory for Java 22+ and Vectorization (incubation) for exact Java 22 #12706
Conversation
… do not need any new version; fallback to 21 version
I updated the Jenkins jobs running mmap tests to use this branch: https://jenkins.thetaphi.de/job/Lucene-MMAPv2-Linux/, https://jenkins.thetaphi.de/job/Lucene-MMAPv2-Windows/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation.LGTM.
It's awesome that we can see the final version of this (when we move to JDK 21+ as the minimum) .
…o dev/java22_mmap
After openjdk/jdk#16792 was fixed, I added the better isAlive check to the Java 21+ code. |
…o dev/java22_mmap
I tested the new code - and at the same time commenting out the Java 21 fallback code to check for "closed" in the lucene/lucene/core/src/java21/org/apache/lucene/store/MemorySegmentIndexInput.java Lines 116 to 121 in 5c91529
With JDK 22 build 23 the test fails because it rethrows the lucene/lucene/core/src/java21/org/apache/lucene/store/MemorySegmentIndexInput.java Lines 112 to 114 in 5c91529
With build 26 beasting the test worked for hours:
(build 23 or earlier Java versions like 21 failed on first or second build) |
No no no, I am not stale! |
I also committed support for incubator SIMD vectorization in Java 22. According to Java's change logs there were no changes to API at all, so code runs as is. @ChrisHegarty just have a look, but to me it looks like the changes are the only ones needed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I did a quick benchmark of Vector API and compared Java 22-ea+31 vs. Java 21.0.1 (AMD Ryzen 7 3700X 8-Core Processor):
So all looks fine. |
@uschindler This looks like it is in good shape. I think that it is ready for merging, right? |
Hey, yes. I wanted to wait till RC phase and check everything before it. In case we want to release a new Lucene version we can merge this earlier, I just want to be on safe side. |
I don't think there will be any changes till release, but my plan was to merge this on 2024-02-08. |
That sounds fine to me. And I agree with merging this at the JDK 22 RC date. |
@uschindler As per our in-person conversation, are you ok to merge this PR so that it can be incorporated into the next Lucene bugfix version. |
Yes. To be sure I wanted to wait till Thu/Fri this week. But yes in general I am happy to have this in. Do you mean another 9.9.3 with bugfix, or do you mean next minor version 9.10? |
Apologies, I mean the next minor - 9.10 (not 9.9.3). Sorry for the confusion. |
Yes. If we want to start the release process next week I want this in. Risk is low, it is already tested by Jenkins for weeks now. I just want to make sure there are no API glitches. |
I targeted it to milestone 9.10.0. I will add the CHANGES.txt entry shortly before merging. |
Thanks @uschindler. |
The release candidate is still missing and wasn't announced by Mark Reinhold: https://jdk.java.net/22/ |
JDK release candidate was announced (it is build 35). I downloaded and tested it with vector and foreign API, no API breaks detected. Tests work. I will merge this now. |
…(incubation) for exact Java 22 (#12706)
This is a very simple PR to add support for Java 22 and later for the MMapDirectory using MemorySegment. The Panama Foreign API is no longer in preview mode it is fully supported backwards-compatible public API.
I initially extracted the APIJAR file for Java 22, but actually the code compiled unmodified with that version. This means for the parts of the API we use in MemorySegment does not differ from Java 21 and is byte code compatible.
This has several positive effects:
The current API is this: https://download.java.net/java/early_access/jdk22/docs/api/java.base/java/lang/foreign/package-summary.html
The PR is quite simple:
As it is still subject to change, I declare this PR as draft and update it. I will merge it when the JDK 22 goes into rampdown phases (after 2023-12-07). If all looks stable, I will merge at this time, at latest I will merge on the initial release candidate of Java 22 at 2024-02-08,
I extended the PR to also cover the vector incubator (see comment below). Combining that into one PR makes testing easier, as we have a separate Jenkins job on this branch.