Reflow computeCommonPrefixLengthAndBuildHistogram to avoid crash#12905
Reflow computeCommonPrefixLengthAndBuildHistogram to avoid crash#12905ChrisHegarty merged 4 commits intoapache:mainfrom
Conversation
uschindler
left a comment
uschindler
left a comment
There was a problem hiding this comment.
Some minor thing, please fix in main and 9.x (remove permission again). Sorry, I was late to review this.
| permission java.lang.RuntimePermission "writeFileDescriptor"; | ||
|
|
||
| // needed to check if C2 (implied by the presence of the CI env) is enabled | ||
| permission java.lang.RuntimePermission "getenv.CI"; |
There was a problem hiding this comment.
Hi Chris,
this env var isn't too useful as Jenkins does not set it. The logic to detect the CI availability is harder, but actually please remove the permission again, see below!
There was a problem hiding this comment.
Didn't know about it either. Will this work?
#12953
There was a problem hiding this comment.
Yes this works. Gradle enables Tiered compiler if Jenkins or GitHub is detected.
| // Crashes only when run with C2, so with the environment variable `CI` set | ||
| // Regardless of whether C2 is enabled or not, the test should never fail. | ||
| public void testCrash() throws IOException { | ||
| assumeTrue("Requires C2, which is only enabled when CI env is set", getCIEnv() != null); |
There was a problem hiding this comment.
There's a much easier way to detect if CI is enabled. Use assumeFalse(oal.util.Constants.IS_CLIENT_VM) (this was added for vector API detection: https://lucene.apache.org/core/9_9_1/core/org/apache/lucene/util/Constants.html#IS_CLIENT_VM)
…ash (apache#12905)" This reverts commit a6f70ad.
3 participants
This commit reflows the code in the method body of computeCommonPrefixLengthAndBuildHistogram, so as to avoid a JVM JIT crash. The purpose of this change is to workaround the JVM bug which is somewhat fragile, but the best that we can do for now and appears to be working well. Further details of the JDK issue can be found in https://bugs.openjdk.org/browse/JDK-8321370 and linked issues.
The newly added test fails around 9 out of 10 times for me, before the fix. With the fix the test has not been seen to fail in several hundreds of runs. Though, further test runs are on going.
The JDK issue is already fixed in the currently unreleased JDK 21.0.2 and JDK 22, but we need this workaround to avoid the crash on current released JDKs.
relates #12898