Remove ByteBufferIndexInput and update all Panama implementations (MMap and Vector) to Java 21 #13146
Conversation
…ap and Vector) to Java 21
|
Because this also updated ASM to correct versions, I changed the JavascriptCompiler to use Java 21 class file format. |
|
Hi, tasks.named('jar').configure {
boolean needMRJAR = false;
mrjarJavaVersions.each { jdkVersion ->
boolean isBaseVersion = (jdkVersion.toString() == rootProject.minJavaVersion.toString())
into(isBaseVersion ? '' : "META-INF/versions/${jdkVersion}") {
from sourceSets["main${jdkVersion}"].output
}
needMRJAR |= !isBaseVersion
}
if (needMRJAR) {
manifest.attributes(
'Multi-Release': 'true'
)
}
}This copies the files from the |
| @@ -50,9 +50,6 @@ grant { | |||
| permission java.lang.RuntimePermission "getStackTrace"; | |||
| // needed for mock filesystems in tests | |||
| permission java.lang.RuntimePermission "fileSystemProvider"; | |||
| // needed to test unmap hack on platforms that support it | |||
| permission java.lang.RuntimePermission "accessClassInPackage.sun.misc"; | |||
There was a problem hiding this comment.
This is my favourite change of the whole PR!
| public void testAceWithThreads() throws Exception { | ||
| assumeTrue("Test requires MemorySegmentIndexInput", isMemorySegmentImpl()); |
There was a problem hiding this comment.
this is my favorite part of the whole PR
| @@ -157,7 +157,7 @@ public static FSDirectory open(Path path) throws IOException { | |||
|
|
|||
| /** Just like {@link #open(Path)}, but allows you to also specify a custom {@link LockFactory}. */ | |||
| public static FSDirectory open(Path path, LockFactory lockFactory) throws IOException { | |||
There was a problem hiding this comment.
FYI: there are still some references to the unmap-hack in the class javadocs of this file. It's gotta feel great nuking these warnings :)
There was a problem hiding this comment.
will grep through the code :-)
There was a problem hiding this comment.
Fixed in 0f146ac. Thanks Robert!
|
I'm not sure how you debugged that! I tried to debug what gradle is doing with this tests counter (hey, a stacktrace of the offending setAccessible would be nice), but I think @dweiss has seen this before, it is not really possible. As soon as you set java.security.debug on the test JVM, gradle test runner dies because it doesn't like the stderr prints or something? I tried to just add something like this to Trying to debug just crashes the tests instead, it hits StackOverflowError... |
|
It's this issue - they closed the issue but it's still not working as expected. |
I did not debug that; it was an observation and then try/error. I was a bit annoyed yesterday so here is my observations:
No idea. I was shortly before jumping out of the windows last night. I did not try to debug, I just reverted test changes step by step until the policy file came to my mind.... After reverting the changes in policy, the test counter was incrementing again. I did not do any further investigations or debugging. I was just annoyed and angry and very tired (2:30 in the morning). This is all bad, no debugging possible. |
| @@ -50,9 +50,6 @@ grant { | |||
| permission java.lang.RuntimePermission "getStackTrace"; | |||
| // needed for mock filesystems in tests | |||
| permission java.lang.RuntimePermission "fileSystemProvider"; | |||
| // needed to test unmap hack on platforms that support it | |||
| permission java.lang.RuntimePermission "accessClassInPackage.sun.misc"; | |||
| permission java.lang.reflect.ReflectPermission "suppressAccessChecks"; | |||
There was a problem hiding this comment.
unfortunately this one had to be reverted :-(
|
I did track it down to gradle's test listeners - it looks like a bug to me. Whatever this delegation is doing, even a simple check on whether the target method is already accessible (or even public) would be sufficient for this to work.
Oh well. |
And on top of this, as usual Oh well! puke P.S.: Where is the exception swallowed? |
|
I had trouble identifying the root cause of the need for the security permission grant, and setting the java security debug property just made things fail in a different and unhelpful way, so I reproduced with a "modified" JDK - that emits the security debugging output to a temp file (rather than |
|
Adding explicit grants to the above 3 identified codebases (randomizedtesting-runner, junit, and lucene-test-framework), allows the test count to work (for me), e.g. (quickly hacked with hardcoded paths) |
|
Cool. So it is not gradle, which breaks. Does counting while the tests are running works then? |
|
Basically we would need one more AccessController #doPrivileged for the third stack trace. The first two would make it enough to have the randomized runner be whitelisted. |
|
Have you looked at the test output xmls, Chris? Did the tests actually execute? There is a doPrivileged wrapper in RR - I'm not sure why it requires permissions, it shouldn't. |
|
I can reproduce your results (tests do execute, even if you exclude randomizedtesting jar - the count doesn't show up in gradle properly then though). |
|
Things like this: will be very difficult to predict - the runner interacts closely with junit in so many places that I think granting it a permission for reflection will be an easier workaround than trying to figure out where such calls can be made from within junit... I'm also surprised none of the gradle callbacks violate the security permissions. Could it be that I stopped on that breakpoint in gradle's server-side code?! it is confusing. |
|
Ok, I forgot we actually allow gradle to do anything: I think I could try to locate places in RandomizedRunner where it calls into JUnit without doPrivileged... not sure if it's worth the hassle though - maybe adding permissions for just those three jars is fine (we can compute the URLs and pass them as properties)? |
|
There are also some |
Die, die, die. 🤬😱🤨 |
|
We should ban it in forbidden-apis if not already... also some of the usages in tests should be reviewed. Two of them are the JVM-crashing tests. The other is in RAMUsageTester, I don't understand it enough. |
Putting aside the test specific usages mentioned above, then... so long as there is no test code on the stack when setAccessible is called, those three jars should be sufficient. At least from what I see on my machine. I do see the test count working fine with just those three jars (as well as the global grants removed). Here's an example from 
|
There was a problem hiding this comment.
I don't see CHANGES.txt updated; was it forgotten? Some tuning settings have disappeared now. org.apache.lucene.store.MMapDirectory.enableMemorySegments=false in particular is one I'm using in 9.8 to recover a big performance loss from an 8x upgrade. JDK 21. We'll eventually do more analysis to see what's going on. Perhaps we'll chat at Berlin Buzzwords soon.
|
Do you have highly concurrent close of index files? Die to the new features regarding safe close, the close is more expensive (especially for other threads concurrently accessing index files. This comes from a safe point for a thread local handshake to prevent access to the unmapped memory after close. This is a known limitation. |
|
Changes entry is here: Line 87 in 750a7c4 It is Lucene 10 only. |
|
See also discussion here: dacapobench/dacapobench#264 (comment) |
|
Thanks Uwe! I suppose I blame the JDK then :-) |
It was removed because it is unsafe (it can crash your JVM) and with current JEPs in development it will no longer work in JDK 24 because sun.misc.Unsafe will go away and therefor unmapping won't work anymore. So basically we get rid of unsafe and code which is no longer supported. I am planning to open JDK issues because from the code, it should not slow too much. But it looks like of a side effect that it sometimes deoptimizes code on concurrent access on the safepoint. I will check about this with Maurizio. It could possibly only a bug in Java 21/22. So actually this is the reason why we have the sysprop in 9.x so we can detect such bugs, report it to JDK and let it fix those bugs. So please share more information or a small bench showing the problem. In typical Elasticsearch use cases we have seen no slowdown, but more a speedup. But on the other hand we did not close the IndexInput 10 times per second while punching on them from 64 threads. |
|
Hi @dsmiley, My idea is:
Theoretically, the benchmark should not slow if the few threads close Arenas which are not bound to and hot workers. What we have seen in dacapo bench is that it seems to affect all MemorySegments negatively, not only those which are affected by the close. of Arena. Once I have the benchmark I will open issue at OpenJDK. In the meantime, we could "preserve" the old Lucene 9.1 (the version without MemorySegments) in the "misc" module of Lucene as LegacyMMapDirectory. This would allow people to use it, but it is strinly discouraged. It won't work anymore with Java 24 (or around that time). |
|
I reopened #13325. |
This PR updates the MR-JAR parts to only have implementations of Java 21.
This PR does not remove the sourceSets for Java 21, although this is also our base version:
Because compiling against the APIJAR is a hack, we do not want to do this for the main sourceset. So this one still has a separate sourceset with the Java 21 classes of vector and memorysegment.
In the current state the Java 21 classes are still put into a MR-JAR part. I don't want to remove this for now:
We could merge the Java 21 classes into the main part of the JAR file. The Gradle code could just compare the base version with the MR-JAR sourceset and if the version is identical (minJavaVersion==sourcesetVersion) it could copy the files into the main part of the JAR.
I will try this in a separate commit.