Skip to content
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

Merged
merged 16 commits into from Feb 9, 2024

Conversation

uschindler
Copy link
Contributor

@uschindler uschindler commented Oct 22, 2023

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:

  • we need no additional APIJAR to compile and no MR-JAR folder (this may change when we need an update for vectors)
  • If we change minimum Java version for main branch to 21, we can compile against the JDK 21 APIJARS (with patch-module in the main sourceSet) and only have a single version that will be compatible for all Java versions supported. For branch 9.x we need to stay with MR-JAR. Compiling against the APIJAR is needed because of the stupid preview flag on all class files in Java 21 (that is removed from APIJAR). We can also remove the (Mapped-)ByteBufferIndexInput impl completely.

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:

  • It removes the upper version limitation in the provider interface lookup
  • It removes the upper version limitation in one test
  • Fixes a message

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.

… do not need any new version; fallback to 21 version
@uschindler uschindler self-assigned this Oct 22, 2023
@uschindler uschindler marked this pull request as draft October 22, 2023 07:48
@uschindler
Copy link
Contributor Author

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/

Copy link
Contributor

@ChrisHegarty ChrisHegarty left a 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) .

@uschindler
Copy link
Contributor Author

uschindler commented Nov 30, 2023

After openjdk/jdk#16792 was fixed, I added the better isAlive check to the Java 21+ code.

@uschindler
Copy link
Contributor Author

uschindler commented Dec 1, 2023

I tested the new code - and at the same time commenting out the Java 21 fallback code to check for "closed" in the IllegalStateException message here:

if (e instanceof IllegalStateException
&& e.getMessage() != null
&& e.getMessage().contains("closed")) {
// the check is on message only, so preserve original cause for debugging:
return new AlreadyClosedException("Already closed: " + this, e);
}

With JDK 22 build 23 the test fails because it rethrows the IllegalStateException due to the isAlive() check didn't work:

if (Arrays.stream(segments).allMatch(s -> s.scope().isAlive()) == false) {
return new AlreadyClosedException("Already closed: " + this);
}

With build 26 beasting the test worked for hours:

$ ./gradlew :lucene:core:beast --tests TestMMapDirectory.testAceWithThreads -Ptests.multiplier=500 -Ptests.dups=100

(build 23 or earlier Java versions like 21 failed on first or second build)

@uschindler
Copy link
Contributor Author

No no no, I am not stale!

@uschindler
Copy link
Contributor Author

uschindler commented Jan 13, 2024

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.

@uschindler uschindler changed the title Initial impl of MMapDirectory for Java 22 Initial impl of MMapDirectory and Vectorization (incubation) for Java 22 Jan 13, 2024
Copy link
Contributor

@ChrisHegarty ChrisHegarty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@uschindler
Copy link
Contributor Author

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):

$ ~jenkins/tools/java/64bit/hotspot/jdk-22-ea+31/bin/java --module-path lucene/benchmark-jmh/build/benchmarks --module org.apache.lucene.benchmark.jmh VectorUtilBenchmark -p size=1024

Jan 14, 2024 11:12:14 AM org.apache.lucene.internal.vectorization.PanamaVectorizationProvider <init>
INFO: Java vector incubator API enabled; uses preferredBitSize=256; FMA enabled

Benchmark                                   (size)   Mode  Cnt   Score   Error   Units
VectorUtilBenchmark.binaryCosineScalar        1024  thrpt   15   0.632 ± 0.004  ops/us
VectorUtilBenchmark.binaryCosineVector        1024  thrpt   15   4.635 ± 0.049  ops/us
VectorUtilBenchmark.binaryDotProductScalar    1024  thrpt   15   1.904 ± 0.030  ops/us
VectorUtilBenchmark.binaryDotProductVector    1024  thrpt   15  12.080 ± 0.101  ops/us
VectorUtilBenchmark.binarySquareScalar        1024  thrpt   15   1.566 ± 0.029  ops/us
VectorUtilBenchmark.binarySquareVector        1024  thrpt   15  11.060 ± 0.141  ops/us
VectorUtilBenchmark.floatCosineScalar         1024  thrpt   15   1.464 ± 0.019  ops/us
VectorUtilBenchmark.floatCosineVector         1024  thrpt   75  10.497 ± 0.042  ops/us
VectorUtilBenchmark.floatDotProductScalar     1024  thrpt   15   3.887 ± 0.036  ops/us
VectorUtilBenchmark.floatDotProductVector     1024  thrpt   75  20.890 ± 0.446  ops/us
VectorUtilBenchmark.floatSquareScalar         1024  thrpt   15   3.063 ± 0.034  ops/us
VectorUtilBenchmark.floatSquareVector         1024  thrpt   75  20.096 ± 0.198  ops/us


$ ~jenkins/tools/java/64bit/hotspot/jdk-21.0.1/bin/java --module-path lucene/benchmark-jmh/build/benchmarks --module org.apache.lucene.benchmark.jmh VectorUtilBenchmark -p size=1024

Jan 14, 2024 11:28:42 AM org.apache.lucene.internal.vectorization.PanamaVectorizationProvider <init>
INFO: Java vector incubator API enabled; uses preferredBitSize=256; FMA enabled

Benchmark                                   (size)   Mode  Cnt   Score   Error   Units
VectorUtilBenchmark.binaryCosineScalar        1024  thrpt   15   0.627 ± 0.005  ops/us
VectorUtilBenchmark.binaryCosineVector        1024  thrpt   15   4.684 ± 0.044  ops/us
VectorUtilBenchmark.binaryDotProductScalar    1024  thrpt   15   1.936 ± 0.026  ops/us
VectorUtilBenchmark.binaryDotProductVector    1024  thrpt   15  12.235 ± 0.117  ops/us
VectorUtilBenchmark.binarySquareScalar        1024  thrpt   15   1.590 ± 0.014  ops/us
VectorUtilBenchmark.binarySquareVector        1024  thrpt   15  11.114 ± 0.143  ops/us
VectorUtilBenchmark.floatCosineScalar         1024  thrpt   15   1.442 ± 0.025  ops/us
VectorUtilBenchmark.floatCosineVector         1024  thrpt   75  10.370 ± 0.049  ops/us
VectorUtilBenchmark.floatDotProductScalar     1024  thrpt   15   3.866 ± 0.028  ops/us
VectorUtilBenchmark.floatDotProductVector     1024  thrpt   75  19.050 ± 0.547  ops/us
VectorUtilBenchmark.floatSquareScalar         1024  thrpt   15   3.094 ± 0.028  ops/us
VectorUtilBenchmark.floatSquareVector         1024  thrpt   75  18.836 ± 0.343  ops/us

So all looks fine.

@ChrisHegarty
Copy link
Contributor

@uschindler This looks like it is in good shape. I think that it is ready for merging, right?

@uschindler
Copy link
Contributor Author

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.

@uschindler
Copy link
Contributor Author

I don't think there will be any changes till release, but my plan was to merge this on 2024-02-08.

@uschindler uschindler changed the title Initial impl of MMapDirectory and Vectorization (incubation) for Java 22 Enable MemorySegment in MMapDirectory for Java 22+ and Vectorization (incubation) for exact Java 22 Jan 23, 2024
@ChrisHegarty
Copy link
Contributor

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.

@ChrisHegarty
Copy link
Contributor

@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.

@uschindler
Copy link
Contributor Author

uschindler commented Feb 6, 2024

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?

@uschindler uschindler marked this pull request as ready for review February 6, 2024 15:52
@ChrisHegarty
Copy link
Contributor

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.

@uschindler
Copy link
Contributor Author

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.

@uschindler uschindler added this to the 9.10.0 milestone Feb 6, 2024
@uschindler
Copy link
Contributor Author

I targeted it to milestone 9.10.0. I will add the CHANGES.txt entry shortly before merging.

@ChrisHegarty
Copy link
Contributor

Thanks @uschindler.

@uschindler
Copy link
Contributor Author

The release candidate is still missing and wasn't announced by Mark Reinhold: https://jdk.java.net/22/

@uschindler
Copy link
Contributor Author

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.

@uschindler uschindler merged commit 178f5a7 into apache:main Feb 9, 2024
4 checks passed
@uschindler uschindler deleted the dev/java22_mmap branch February 9, 2024 22:02
asfgit pushed a commit that referenced this pull request Feb 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants