From a6f70ad2bb0b682eb65feb522358ee6d16cad766 Mon Sep 17 00:00:00 2001 From: Chris Hegarty <62058229+ChrisHegarty@users.noreply.github.com> Date: Mon, 11 Dec 2023 20:10:03 +0000 Subject: [PATCH] Reflow computeCommonPrefixLengthAndBuildHistogram to avoid crash (#12905) 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. --- .../randomization/policies/tests.policy | 3 ++ .../apache/lucene/util/MSBRadixSorter.java | 21 +++++++++++++ .../org/apache/lucene/util/RadixSelector.java | 21 +++++++++++++ .../lucene/util/bkd/TestDocIdsWriter.java | 31 +++++++++++++++++++ 4 files changed, 76 insertions(+) diff --git a/gradle/testing/randomization/policies/tests.policy b/gradle/testing/randomization/policies/tests.policy index 150fa970b97..ce23b31bb27 100644 --- a/gradle/testing/randomization/policies/tests.policy +++ b/gradle/testing/randomization/policies/tests.policy @@ -60,6 +60,9 @@ grant { permission java.lang.RuntimePermission "getFileStoreAttributes"; 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"; + // TestLockFactoriesMultiJVM opens a random port on 127.0.0.1 (port 0 = ephemeral port range): permission java.net.SocketPermission "127.0.0.1:0", "accept,listen,resolve"; diff --git a/lucene/core/src/java/org/apache/lucene/util/MSBRadixSorter.java b/lucene/core/src/java/org/apache/lucene/util/MSBRadixSorter.java index f471a3f53fc..70948d3c30b 100644 --- a/lucene/core/src/java/org/apache/lucene/util/MSBRadixSorter.java +++ b/lucene/core/src/java/org/apache/lucene/util/MSBRadixSorter.java @@ -213,7 +213,16 @@ public abstract class MSBRadixSorter extends Sorter { * * @see #buildHistogram */ + // This method, and its namesakes, have been manually split to work around a JVM crash. + // See https://github.com/apache/lucene/issues/12898 private int computeCommonPrefixLengthAndBuildHistogram(int from, int to, int k, int[] histogram) { + int commonPrefixLength = computeInitialCommonPrefixLength(from, k); + return computeCommonPrefixLengthAndBuildHistogramPart1( + from, to, k, histogram, commonPrefixLength); + } + + // This method, and its namesakes, have been manually split to work around a JVM crash. + private int computeInitialCommonPrefixLength(int from, int k) { final int[] commonPrefix = this.commonPrefix; int commonPrefixLength = Math.min(commonPrefix.length, maxLength - k); for (int j = 0; j < commonPrefixLength; ++j) { @@ -224,7 +233,13 @@ public abstract class MSBRadixSorter extends Sorter { break; } } + return commonPrefixLength; + } + // This method, and its namesakes, have been manually split to work around a JVM crash. + private int computeCommonPrefixLengthAndBuildHistogramPart1( + int from, int to, int k, int[] histogram, int commonPrefixLength) { + final int[] commonPrefix = this.commonPrefix; int i; outer: for (i = from + 1; i < to; ++i) { @@ -239,7 +254,13 @@ public abstract class MSBRadixSorter extends Sorter { } } } + return computeCommonPrefixLengthAndBuildHistogramPart2( + from, to, k, histogram, commonPrefixLength, i); + } + // This method, and its namesakes, have been manually split to work around a JVM crash. + private int computeCommonPrefixLengthAndBuildHistogramPart2( + int from, int to, int k, int[] histogram, int commonPrefixLength, int i) { if (i < to) { // the loop got broken because there is no common prefix assert commonPrefixLength == 0; diff --git a/lucene/core/src/java/org/apache/lucene/util/RadixSelector.java b/lucene/core/src/java/org/apache/lucene/util/RadixSelector.java index 6a916ed8f85..47c3ca6eaf1 100644 --- a/lucene/core/src/java/org/apache/lucene/util/RadixSelector.java +++ b/lucene/core/src/java/org/apache/lucene/util/RadixSelector.java @@ -198,7 +198,16 @@ public abstract class RadixSelector extends Selector { * * @see #buildHistogram */ + // This method, and its namesakes, have been manually split to work around a JVM crash. + // See https://github.com/apache/lucene/issues/12898 private int computeCommonPrefixLengthAndBuildHistogram(int from, int to, int k, int[] histogram) { + int commonPrefixLength = computeInitialCommonPrefixLength(from, k); + return computeCommonPrefixLengthAndBuildHistogramPart1( + from, to, k, histogram, commonPrefixLength); + } + + // This method, and its namesakes, have been manually split to work around a JVM crash. + private int computeInitialCommonPrefixLength(int from, int k) { final int[] commonPrefix = this.commonPrefix; int commonPrefixLength = Math.min(commonPrefix.length, maxLength - k); for (int j = 0; j < commonPrefixLength; ++j) { @@ -209,7 +218,13 @@ public abstract class RadixSelector extends Selector { break; } } + return commonPrefixLength; + } + // This method, and its namesakes, have been manually split to work around a JVM crash. + private int computeCommonPrefixLengthAndBuildHistogramPart1( + int from, int to, int k, int[] histogram, int commonPrefixLength) { + final int[] commonPrefix = this.commonPrefix; int i; outer: for (i = from + 1; i < to; ++i) { @@ -226,7 +241,13 @@ public abstract class RadixSelector extends Selector { } } } + return computeCommonPrefixLengthAndBuildHistogramPart2( + from, to, k, histogram, commonPrefixLength, i); + } + // This method, and its namesakes, have been manually split to work around a JVM crash. + private int computeCommonPrefixLengthAndBuildHistogramPart2( + int from, int to, int k, int[] histogram, int commonPrefixLength, int i) { if (i < to) { // the loop got broken because there is no common prefix assert commonPrefixLength == 0; diff --git a/lucene/core/src/test/org/apache/lucene/util/bkd/TestDocIdsWriter.java b/lucene/core/src/test/org/apache/lucene/util/bkd/TestDocIdsWriter.java index 6610bdb620d..6ddcabb03ae 100644 --- a/lucene/core/src/test/org/apache/lucene/util/bkd/TestDocIdsWriter.java +++ b/lucene/core/src/test/org/apache/lucene/util/bkd/TestDocIdsWriter.java @@ -17,8 +17,14 @@ package org.apache.lucene.util.bkd; import java.io.IOException; +import java.security.AccessController; +import java.security.PrivilegedAction; import java.util.Arrays; +import java.util.List; import java.util.Set; +import org.apache.lucene.document.IntPoint; +import org.apache.lucene.document.SortedNumericDocValuesField; +import org.apache.lucene.index.IndexWriter; import org.apache.lucene.index.PointValues.IntersectVisitor; import org.apache.lucene.index.PointValues.Relation; import org.apache.lucene.store.Directory; @@ -28,6 +34,7 @@ import org.apache.lucene.store.IndexOutput; import org.apache.lucene.tests.util.LuceneTestCase; import org.apache.lucene.tests.util.TestUtil; import org.apache.lucene.util.CollectionUtil; +import org.apache.lucene.util.SuppressForbidden; public class TestDocIdsWriter extends LuceneTestCase { @@ -150,4 +157,28 @@ public class TestDocIdsWriter extends LuceneTestCase { } dir.deleteFile("tmp"); } + + // This simple test tickles a JVM C2 JIT crash on JDK's less than 21.0.1 + // 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); + int itrs = atLeast(100); + for (int i = 0; i < itrs; i++) { + try (Directory dir = newDirectory(); + IndexWriter iw = new IndexWriter(dir, newIndexWriterConfig(null))) { + for (int d = 0; d < 20_000; d++) { + iw.addDocument( + List.of(new IntPoint("foo", 0), new SortedNumericDocValuesField("bar", 0))); + } + } + } + } + + @SuppressForbidden(reason = "needed to check if C2 is enabled") + @SuppressWarnings("removal") + private static String getCIEnv() { + PrivilegedAction pa = () -> System.getenv("CI"); + return AccessController.doPrivileged(pa); + } }