You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by GitBox <gi...@apache.org> on 2022/12/23 14:08:55 UTC
[GitHub] [lucene] uschindler opened a new pull request, #12033: Patch class files for Java 19 code to no longer have the "preview" flag (this enables Java 19 memory segments by default)
uschindler opened a new pull request, #12033:
URL: https://github.com/apache/lucene/pull/12033
After many complaints that you need to use `--enable-preview` to 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:
- the class files are only marked with the flag to prevent them from being used in Java versions that do not suport that exact API level. This is no issue, because I also changed our provider code to load the new class files only on `Runtime.version().feature()==19`
- there is nothing in the JDK that needs to be enabled. The preview features are always available (e.g. with reflection), the runtime flag is only to allow classloader to accept the "marked" class files.
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on pull request #12033: Patch class files for Java 19 code to no longer have the "preview" flag (this enables Java 19 memory segments by default)
Posted by GitBox <gi...@apache.org>.
uschindler commented on PR #12033:
URL: https://github.com/apache/lucene/pull/12033#issuecomment-1370002638
Nice side effect: This would also allow to disable in case OpenJ9 breaks or disables their Panama impl (see above).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] rmuir commented on pull request #12033: Patch class files for Java 19 code to no longer have the "preview" flag (this enables Java 19 memory segments by default)
Posted by GitBox <gi...@apache.org>.
rmuir commented on PR #12033:
URL: https://github.com/apache/lucene/pull/12033#issuecomment-1364453343
theoretically i am fine with the change, the less `SIGSEGV` the better. the hoops we jump through though :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on pull request #12033: Patch class files for Java 19 code to no longer have the "preview" flag (this enables Java 19 memory segments by default)
Posted by GitBox <gi...@apache.org>.
uschindler commented on PR #12033:
URL: https://github.com/apache/lucene/pull/12033#issuecomment-1364028457
Maybe have a look, @mcimadamore
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] mcimadamore commented on pull request #12033: Patch class files for Java 19 code to no longer have the "preview" flag (this enables Java 19 memory segments by default)
Posted by GitBox <gi...@apache.org>.
mcimadamore commented on PR #12033:
URL: https://github.com/apache/lucene/pull/12033#issuecomment-1369859675
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 `--enable-preview` was to give a choice to downstream clients whether to enable or not, and to test this in the real world (so as to provide feedback). Forcing downstream clients to use (indirectly) a preview API is a pragmatic solution, for sure, but also one that loses some of the benefits of the previous approach.
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 `--enable-preview` flag is detected. (AFAIK, this is no longer the case in 20, as the underlying issues have been resolved). So, enabling preview features in JDK 19 *can* result in some performance differences, even if _none_ of the Java 19 experimental features is used.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on pull request #12033: Patch class files for Java 19 code to no longer have the "preview" flag (this enables Java 19 memory segments by default)
Posted by GitBox <gi...@apache.org>.
uschindler commented on PR #12033:
URL: https://github.com/apache/lucene/pull/12033#issuecomment-1364516210
> 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.
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.
> 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) . 👍
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on pull request #12033: Patch class files for Java 19 code to no longer have the "preview" flag (this enables Java 19 memory segments by default)
Posted by GitBox <gi...@apache.org>.
uschindler commented on PR #12033:
URL: https://github.com/apache/lucene/pull/12033#issuecomment-1366554649
Here is the followup PR: #12043
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on pull request #12033: Patch class files for Java 19 code to no longer have the "preview" flag (this enables Java 19 memory segments by default)
Posted by GitBox <gi...@apache.org>.
uschindler commented on PR #12033:
URL: https://github.com/apache/lucene/pull/12033#issuecomment-1366589535
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on pull request #12033: Patch class files for Java 19 code to no longer have the "preview" flag (this enables Java 19 memory segments by default)
Posted by GitBox <gi...@apache.org>.
uschindler commented on PR #12033:
URL: https://github.com/apache/lucene/pull/12033#issuecomment-1370084709
> > According to the spec, there should be "two preview cycles".
>
> 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 revised the API.
I will keep an eye on the mailing lists, but maybe ping me when some decissions need to be taken!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] mcimadamore commented on pull request #12033: Patch class files for Java 19 code to no longer have the "preview" flag (this enables Java 19 memory segments by default)
Posted by GitBox <gi...@apache.org>.
mcimadamore commented on PR #12033:
URL: https://github.com/apache/lucene/pull/12033#issuecomment-1370077469
> According to the spec, there should be "two preview cycles".
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler merged pull request #12033: Patch class files for Java 19 code to no longer have the "preview" flag (this enables Java 19 memory segments by default)
Posted by GitBox <gi...@apache.org>.
uschindler merged PR #12033:
URL: https://github.com/apache/lucene/pull/12033
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on pull request #12033: Patch class files for Java 19 code to no longer have the "preview" flag (this enables Java 19 memory segments by default)
Posted by GitBox <gi...@apache.org>.
uschindler commented on PR #12033:
URL: https://github.com/apache/lucene/pull/12033#issuecomment-1366519363
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 `MemorySegment` class is available before loading our implementations. Currently the code produces a LinkageError on problems, so this would be good to have.
A followup PR is coming soon.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on pull request #12033: Patch class files for Java 19 code to no longer have the "preview" flag (this enables Java 19 memory segments by default)
Posted by GitBox <gi...@apache.org>.
uschindler commented on PR #12033:
URL: https://github.com/apache/lucene/pull/12033#issuecomment-1369993371
I prepared a followup PR #12062: You can pass `-Dorg.apache.lucene.store.MMapDirectory.enableMemorySegments=false` to disable Panama's MemorySegments in case of problems.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on pull request #12033: Patch class files for Java 19 code to no longer have the "preview" flag (this enables Java 19 memory segments by default)
Posted by GitBox <gi...@apache.org>.
uschindler commented on PR #12033:
URL: https://github.com/apache/lucene/pull/12033#issuecomment-1369908501
Thanks Maurizio,
there could be the option to pass a system property to disable (this was discussed on the Elasticsearch issue already). I can do that as a separate PR.
Actually, I do not want to add some public setting to the API (like the current unmap hack enabler), but a system property may help.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on pull request #12033: Patch class files for Java 19 code to no longer have the "preview" flag (this enables Java 19 memory segments by default)
Posted by GitBox <gi...@apache.org>.
uschindler commented on PR #12033:
URL: https://github.com/apache/lucene/pull/12033#issuecomment-1369931777
> I also thought that the whole point of only enabling the memory segment index with --enable-preview was to give a choice to downstream clients whether to enable or not, and to test this in the real world (so as to provide feedback).
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 it is stable".
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.
> 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 --enable-preview flag is detected
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on pull request #12033: Patch class files for Java 19 code to no longer have the "preview" flag (this enables Java 19 memory segments by default)
Posted by GitBox <gi...@apache.org>.
uschindler commented on PR #12033:
URL: https://github.com/apache/lucene/pull/12033#issuecomment-1363998405
As cleanup I also moved all MR-JAR special cases into a separate gradle file to have `javac.gradle` clean and only contain the defaults.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on pull request #12033: Patch class files for Java 19 code to no longer have the "preview" flag (this enables Java 19 memory segments by default)
Posted by GitBox <gi...@apache.org>.
uschindler commented on PR #12033:
URL: https://github.com/apache/lucene/pull/12033#issuecomment-1366557538
But I think we should not add those sanity checks, because JEP 12 states:
> Universally available. The Umbrella JSR for the Java SE $N Platform enumerates the preview features of the platform. The desire for feedback and the expectation of quality means that these features are not "optional"; they must all be supported in full by every implementation of Java SE $N. In particular, all preview features must be disabled by default in a Java SE implementation, and an implementation must offer a mechanism to enable all preview features. An implementation must not allow preview features to be enabled on an individual basis, since all preview features have equal status in the Java SE Platform.
Comments?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] mcimadamore commented on pull request #12033: Patch class files for Java 19 code to no longer have the "preview" flag (this enables Java 19 memory segments by default)
Posted by GitBox <gi...@apache.org>.
mcimadamore commented on PR #12033:
URL: https://github.com/apache/lucene/pull/12033#issuecomment-1369960003
> 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 `enable-preview` is not there. Moreover, Loom does a lot more checks than Panama - for instance, when you use a virtual thread, Loom checks that `--enable-preview` is there - e.g. it doesn't trust the classfile version to be the source of truth (see the PreviewFeatures class in the JDK). If we added a similar check for the Panama API, the trick described in this PR would no longer work.
(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).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on pull request #12033: Patch class files for Java 19 code to no longer have the "preview" flag (this enables Java 19 memory segments by default)
Posted by GitBox <gi...@apache.org>.
uschindler commented on PR #12033:
URL: https://github.com/apache/lucene/pull/12033#issuecomment-1370008349
> 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).
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!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org