Patch class files for Java 19 code to no longer have the "preview" flag (this enables Java 19 memory segments by default) #12033
Conversation
…ag. This enables Java 19 memory segments by default
|
As cleanup I also moved all MR-JAR special cases into a separate gradle file to have |
|
Maybe have a look, @mcimadamore |
|
theoretically i am fine with the change, the less |
There was a problem hiding this comment.
This addresses the concerns that I had regarding the wider enablement of preview features.
There is a tradeoff here, in the form of ongoing maintenance and tinkering, until the Foreign Memory API is finalised. I don't see this as too onerous, since there has already been several iterations and further changes are mostly "cosmetic". This can be largely mitigated with sufficient testing of early access JDK builds.
I assume that older JDK support will be eagerly cleaned up? Say only provide support for JDK N and JDK N+1. So the currently shipping JDK and the one in development. Thus limiting the maintenance cost.
I'm not particularly a fan of patching the class files, but it's quite straightforward and is built upon well specified behaviour. Taken together, this change (if you squint) is no worse than depending upon the unsupported Unsafe API (and as Robert says, the less SIGSEGV the better) . 👍
That's exactly my plan. I will soon add JDK 20 (the code here already prepares that by some code refactoring to enable features only on specific jdk feature versions. I will keep 19 and 20. When LTS 21 comes out hopefully with final release, I will remove 19. At some point we will change main branch on 21 and could therefore remove ByteBuffer support.
Yes. That's exactly my thoughts. Performance wise there's no risk as we have benchmark and patching the class files here is quite simple and risk less. I would not preprocess class files with ASM or similar. Btw, the idea here with the "doLast" was inspired by ASM 's Gradle build. This works well with caching, as the output of JavaCompile task is still well defined, we just apply the change inside the task. |
There was a problem hiding this comment.
At first I was a bit worried about messing with the minor classfile version but it is only used for preview these days (https://docs.oracle.com/javase/specs/jvms/se19/html/jvms-4.html)
I think it is better to do this trickiness and prevent crashes in client applications as much as possible. We have to do some sneaky stuff, but it isn't our fault, openjdk incubates and previews features indefinitely...
…ag (this enables Java 19 memory segments by default) (#12033)
|
After some discussions on twitter (https://twitter.com/jpountz/status/1607308789417492480), vendors of JDK runtimes are allowed to exclude preview features and still pass TCK tests. With current code this is a problem, but to fix it, we can do a check if the A followup PR is coming soon. |
|
Here is the followup PR: #12043 |
|
But I think we should not add those sanity checks, because JEP 12 states:
Comments? |
|
This all looks like a false alarm. Every JDK vendor is required to have the "preview features" available. There is no discussion. So the sainity checks are not needed. I will close the linked PR unmerged. We can use it at a later stage if somebody from IBM/OpenJ9 complains, but I see no reason to take action. In short: all fine. |
|
IMHO it would still be useful to have some kind of flag to disable this. Because of this patch it is effectively no longer possible to run without memory segment in JDK 19 (or later, for future releases). This means that it will be quite tricky to diagnose performance differences caused by the byte buffer <-> memory segment change vs. JDK N <-> JDK N + 1. I also thought that the whole point of only enabling the memory segment index with IMHO, if there was discomfort in using --enable-preview before because of enabling of unwanted features (btw, this concern is not baseless (**)), this patch seems to just kick the can down the road: now the discomfort will be directed at whether using JDK 19 as a whole or not. (**) - Java 19 has other preview features as well, namely Loom. In Java 19 there were some startup regression when enabling Loom. To mitigate these regression, Loom support is only enabled if the |
|
Thanks Maurizio, |
Yes and no, basically the flag in the class files was mainly added to prevent loading classes with preview API features in a JVM that has a different major version (JEP 12: "A JVM implementation for Java SE $N must not define a class file that depends on the preview features of a different Java SE release, even if the JVM implementation would otherwise understand the class file's version. Specifically, ClassLoader.defineClass must fail for the bytes of the class file. This prevents such class files from running years into the future, long after the preview features that were enjoyed by the developer have been removed or finalized in a different form. In essence, Java SE $N+1 does not claim backwards compatibility with the preview features of Java SE $N."). I understand this as "there is a new preview API that may change, so class files may not work anymore, but functionality can be used in production already". It must also be "high quality", "not experimental" and "universally available" (see above). I think another important part is: "A preview API may add public members to classes and interfaces in java.* and javax.; add public classes and interfaces to java. and javax.; add and export packages in the java. and javax.* namespaces; and add modules in the java.* namespace. A preview API may modify the narrative specifications (though not the signatures) of pre-existing methods, fields, classes, interfaces, packages, and modules. A preview API typically resides in the java.base module, but may reside additionally or exclusively in other java.* modules, including those introduced just for the preview API." Lucene is here the downstream consumer ("consumer of this new API"); the downstream user of Lucene never get in contact with Panama APIs so, Elasticsearch, Solr, or Lucene users can't give any feedback on the APIs. Lucene already provides feedback about the API. Lucene 9.4 was our "test balloon", now we think now that it works and performence is fine. We ca still add a sysprop to disable, for sure. Project Loom is different as it has big changes in the underlying JVM, not only the API. Panama added its stuff already in Java 16 (memory scopes) and mixed it into the inner implementation on ByteBuffers and other performance critical classes.
I think this is special, but also according to spec JEP 12: Because Loom has two types of preview features: One inside the JVM and the other one co-developed inside the class library. For the Panama this is not the case, the code to access memory segments is always available because it also shares code with many other features like ByteBuffers (that also use the memory scopes). So ByteBuffers wont work if you would disable the scoped memory access. |
I think the difference between Loom and Panama is just accidental, rather than deliberate. Both are preview features - but since Loom goes deep into the JVM, choices were made to tie Loom closer to the preview command-line - that is, parts of support for continuations/virtual threads will be disabled if (It is of course debatable as to whether all preview API should behave like this - as I stated, the discrepancies so far are more accidental than anything else, due to preview APIs being something rather "new"). As for the FFM API, I'm not planning to make things stricter (unless it is decided as a more global policy for preview APIs) - I don't think it will make sense given the point where the API is. But I wouldn't be surprised if checks like the ones in Loom would become the norm for any new preview API (of course that doesn't affect this PR directly). |
|
I prepared a followup PR #12062: You can pass |
|
Nice side effect: This would also allow to disable in case OpenJ9 breaks or disables their Panama impl (see above). |
Java 20 is also done and it still works there. And I hope in Java 21 we will have a LTS release with Panama enabled by default. According to the spec, there should be "two preview cycles". We would really be happy to use Panama without any hacks in 21, as we are planning to raise the "main" Lucene branch to Java 21 when it comes out, if Panama would not be inside, it would be....... very bad! Looking forward to 21! |
I believe this is mentioned as a guidance, rather than a strict rule. Note that for other features we had more preview rounds (see JEP 433). So, FFM API will be finalized when it's ready :-) a more complete description of the state at the time of writing was published here: https://mail.openjdk.org/pipermail/panama-dev/2022-December/018182.html |
Let me still hope! To me the API looks complete and removing/replacing VaList could also be done without another preview round. You collected feedback and you may slightly revise the API as final step. I will keep an eye on the mailing lists, but maybe ping me when some decissions need to be taken! |
When we upgraded to lucene 9.5 (snapshot) with elastic#92957 we initially disable panama-based mmap directory through a system property. With this commit we remove the system property and enable java 19 memory segments by default (based on apache/lucene#12033)
When we upgraded to lucene 9.5 (snapshot) with #92957 we initially disable panama-based mmap directory through a system property. With this commit we remove the system property and enable java 19 memory segments by default (based on apache/lucene#12033)
With the merging of apache/lucene#12033 this testing is no longer necessary as Lucene no longer requires the --enable-preview flag for enabling the project panama mmap implementation.
With the merging of apache/lucene#12033 this testing is no longer necessary as Lucene no longer requires the --enable-preview flag for enabling the project panama mmap implementation.
No longer required with Lucene 9.5: apache/lucene#12033
No longer required with Lucene 9.5: apache/lucene#12033
After many complaints that you need to use
--enable-previewto use the new MMapDirectory with Java 19 (especially Elasticsearch peope where afraid that passing this flag enables other "unwanted features" in the JDK), this PR makes the class file exposed by javac "real java 19 production class files", by removing the preview flag from the class file.Actually this is exactly according to spec:
Runtime.version().feature()==19(later also 20). The implementation classes are also not public, so they can't be instantiated without our prechecks.With this PR we use MMapDirectory by default on exactly Java 19. On Java 20 it prints at moment a warning (like before). I will soon provide another MR-JAR variant for Java 20 (there were some significant API changes, so we need a separate SourceSet and compiler).
If nobody objects, I will merge this around Xmas. I will add a CHANGES.txt entry mentioning this.