From 550422cdc25a6263dfa6dadef4803a7daf553203 Mon Sep 17 00:00:00 2001 From: Ketan Verma Date: Tue, 14 Nov 2023 15:21:20 +0530 Subject: [PATCH 01/11] Add experimental SIMD implementation of B-tree to round down dates Signed-off-by: Ketan Verma --- CHANGELOG.md | 1 + benchmarks/build.gradle | 5 + .../common/round/RoundableBenchmark.java | 18 ++- distribution/src/config/opensearch.yml | 5 + libs/common/build.gradle | 78 +++++++++++++ .../common/round/RoundableFactory.java | 2 +- .../common/round/BtreeSearcher.java | 110 ++++++++++++++++++ .../common/round/RoundableFactory.java | 55 +++++++++ .../BidirectionalLinearSearcherTests.java | 16 +++ .../common/round/BinarySearcherTests.java | 16 +++ .../common/round/BtreeSearcherTests.java | 20 ++++ .../java/org/opensearch/common/Rounding.java | 5 +- .../common/settings/FeatureFlagSettings.java | 3 +- .../opensearch/common/util/FeatureFlags.java | 8 ++ .../common/round/RoundableTestCase.java | 29 +++-- 15 files changed, 351 insertions(+), 20 deletions(-) create mode 100644 libs/common/src/main/java20/org/opensearch/common/round/BtreeSearcher.java create mode 100644 libs/common/src/main/java20/org/opensearch/common/round/RoundableFactory.java create mode 100644 libs/common/src/test/java/org/opensearch/common/round/BidirectionalLinearSearcherTests.java create mode 100644 libs/common/src/test/java/org/opensearch/common/round/BinarySearcherTests.java create mode 100644 libs/common/src/test/java20/org/opensearch/common/round/BtreeSearcherTests.java rename libs/common/src/test/java/org/opensearch/common/round/RoundableTests.java => test/framework/src/main/java/org/opensearch/common/round/RoundableTestCase.java (60%) diff --git a/CHANGELOG.md b/CHANGELOG.md index a75463ff16a08..6ce9b597f1dd3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -105,6 +105,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Adding slf4j license header to LoggerMessageFormat.java ([#11069](https://github.com/opensearch-project/OpenSearch/pull/11069)) - [Streaming Indexing] Introduce new experimental server HTTP transport based on Netty 4 and Project Reactor (Reactor Netty) ([#9672](https://github.com/opensearch-project/OpenSearch/pull/9672)) - Allowing pipeline processors to access index mapping info by passing ingest service ref as part of the processor factory parameters ([#10307](https://github.com/opensearch-project/OpenSearch/pull/10307)) +- Add experimental SIMD implementation of B-tree to round down dates ([#11194](https://github.com/opensearch-project/OpenSearch/issues/11194)) ### Dependencies - Bump `com.google.api.grpc:proto-google-common-protos` from 2.10.0 to 2.25.1 ([#10208](https://github.com/opensearch-project/OpenSearch/pull/10208), [#10298](https://github.com/opensearch-project/OpenSearch/pull/10298)) diff --git a/benchmarks/build.gradle b/benchmarks/build.gradle index 02aa9319cc583..d699c44ac8266 100644 --- a/benchmarks/build.gradle +++ b/benchmarks/build.gradle @@ -64,6 +64,11 @@ compileJava.options.compilerArgs.addAll(["-processor", "org.openjdk.jmh.generato run.executable = "${BuildParams.runtimeJavaHome}/bin/java" +// Add support for incubator modules on supported Java versions. +if (BuildParams.runtimeJavaVersion >= JavaVersion.VERSION_20) { + run.jvmArgs += ["--add-modules=jdk.incubator.vector"] +} + // classes generated by JMH can use all sorts of forbidden APIs but we have no influence at all and cannot exclude these classes disableTasks('forbiddenApisMain') diff --git a/benchmarks/src/main/java/org/opensearch/common/round/RoundableBenchmark.java b/benchmarks/src/main/java/org/opensearch/common/round/RoundableBenchmark.java index 4e07af452968b..3b0e5fbf1a7d5 100644 --- a/benchmarks/src/main/java/org/opensearch/common/round/RoundableBenchmark.java +++ b/benchmarks/src/main/java/org/opensearch/common/round/RoundableBenchmark.java @@ -20,6 +20,7 @@ import org.openjdk.jmh.annotations.Warmup; import org.openjdk.jmh.infra.Blackhole; +import java.lang.invoke.MethodHandles; import java.util.Random; import java.util.function.Supplier; @@ -83,7 +84,7 @@ public static class Options { "256" }) public Integer size; - @Param({ "binary", "linear" }) + @Param({ "linear", "binary", "btree" }) public String type; @Param({ "uniform", "skewed_edge", "skewed_center" }) @@ -93,7 +94,7 @@ public static class Options { public Supplier supplier; @Setup - public void setup() { + public void setup() throws ClassNotFoundException, IllegalAccessException { Random random = new Random(size); long[] values = new long[size]; for (int i = 1; i < values.length; i++) { @@ -135,6 +136,19 @@ public void setup() { case "linear": supplier = () -> new BidirectionalLinearSearcher(values, size); break; + case "btree": + // Not supported below Java 20. + // Using MethodHandles to reflectively look up the class (if available) in order to + // avoid setting up separate Java 20 sources with mostly duplicate code. + Class clz = MethodHandles.lookup().findClass("org.opensearch.common.round.BtreeSearcher"); + supplier = () -> { + try { + return (Roundable) clz.getDeclaredConstructor(long[].class, int.class).newInstance(values, size); + } catch (Exception e) { + throw new RuntimeException(e); + } + }; + break; default: throw new IllegalArgumentException("invalid type: " + type); } diff --git a/distribution/src/config/opensearch.yml b/distribution/src/config/opensearch.yml index b7ab2e1c2309b..1e2b01feea1ba 100644 --- a/distribution/src/config/opensearch.yml +++ b/distribution/src/config/opensearch.yml @@ -127,3 +127,8 @@ ${path.logs} # Once there is no observed impact on performance, this feature flag can be removed. # #opensearch.experimental.optimization.datetime_formatter_caching.enabled: false +# +# +# Gates the usage of SIMD for rounding down numbers. +# This is used extensively to round down timestamps in the date_histogram aggregation. +#opensearch.experimental.simd.rounding.enabled: false diff --git a/libs/common/build.gradle b/libs/common/build.gradle index 973fe30d09842..fe8ea9df9598f 100644 --- a/libs/common/build.gradle +++ b/libs/common/build.gradle @@ -43,3 +43,81 @@ tasks.named('forbiddenApisMain').configure { // TODO: Need to decide how we want to handle for forbidden signatures with the changes to server replaceSignatureFiles 'jdk-signatures' } + +// Add support for incubator modules on supported Java versions. +if (BuildParams.runtimeJavaVersion >= JavaVersion.VERSION_20) { + sourceSets { + java20 { + java { + srcDirs = ['src/main/java20'] + } + } + + java20Test { + java { + srcDirs = ['src/test/java20'] + } + } + } + + configurations { + java20Implementation.extendsFrom(implementation) + java20TestImplementation.extendsFrom(implementation) + } + + dependencies { + java20Implementation sourceSets.main.output + + // Adding Java 20 sources as compile-only to make them visible during compilation, + // but using the multi-release JAR output to provide the runtime functionality. + // This helps avoid JarHell problems when an overridden class is present with the same name. + java20TestCompileOnly sourceSets.java20.output + java20TestImplementation files(jar.archiveFile) + java20TestImplementation sourceSets.test.output + java20TestImplementation(project(':test:framework')) { + exclude group: 'org.opensearch', module: 'opensearch-common' + } + } + + compileJava20Java { + sourceCompatibility = JavaVersion.VERSION_20 + targetCompatibility = JavaVersion.VERSION_20 + options.compilerArgs += ['--add-modules', 'jdk.incubator.vector'] + options.compilerArgs -= '-Werror' // use of incubator modules is reported as a warning + } + + compileJava20TestJava { + sourceCompatibility = JavaVersion.VERSION_20 + targetCompatibility = JavaVersion.VERSION_20 + options.compilerArgs += ['--add-modules', 'jdk.incubator.vector'] + options.compilerArgs -= '-Werror' // use of incubator modules is reported as a warning + } + + jar { + metaInf { + into 'versions/20' + from sourceSets.java20.output + } + manifest.attributes('Multi-Release': 'true') + } + + forbiddenApisJava20 { + failOnMissingClasses = false + ignoreSignaturesOfMissingClasses = true + } + + forbiddenApisJava20Test { + failOnMissingClasses = false + ignoreSignaturesOfMissingClasses = true + } + + tasks.register('testJava20', Test) { + dependsOn jar + shouldRunAfter test + group = 'verification' + testClassesDirs = sourceSets.java20Test.output.classesDirs + classpath = sourceSets.java20Test.runtimeClasspath + } + + check.dependsOn('testJava20') +} diff --git a/libs/common/src/main/java/org/opensearch/common/round/RoundableFactory.java b/libs/common/src/main/java/org/opensearch/common/round/RoundableFactory.java index b7422694c3013..0ca1ae33d24bd 100644 --- a/libs/common/src/main/java/org/opensearch/common/round/RoundableFactory.java +++ b/libs/common/src/main/java/org/opensearch/common/round/RoundableFactory.java @@ -29,7 +29,7 @@ private RoundableFactory() {} /** * Creates and returns the fastest implementation of {@link Roundable}. */ - public static Roundable create(long[] values, int size) { + public static Roundable create(long[] values, int size, boolean useSimdIfAvailable) { if (size <= LINEAR_SEARCH_MAX_SIZE) { return new BidirectionalLinearSearcher(values, size); } else { diff --git a/libs/common/src/main/java20/org/opensearch/common/round/BtreeSearcher.java b/libs/common/src/main/java20/org/opensearch/common/round/BtreeSearcher.java new file mode 100644 index 0000000000000..624ffaaa4ba2a --- /dev/null +++ b/libs/common/src/main/java20/org/opensearch/common/round/BtreeSearcher.java @@ -0,0 +1,110 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.common.round; + +import org.opensearch.common.annotation.InternalApi; + +import jdk.incubator.vector.LongVector; +import jdk.incubator.vector.Vector; +import jdk.incubator.vector.VectorOperators; +import jdk.incubator.vector.VectorSpecies; + +/** + * It uses vectorized B-tree search to find the round-down point. + * + * @opensearch.internal + */ +@InternalApi +class BtreeSearcher implements Roundable { + private final VectorSpecies species; + private final int lanes; + private final int shift; + private final long[] values; + + BtreeSearcher(long[] values, int size) { + this(values, size, LongVector.SPECIES_PREFERRED); + } + + BtreeSearcher(long[] values, int size, VectorSpecies species) { + if (size <= 0) { + throw new IllegalArgumentException("at least one value must be present"); + } + + this.species = species; + this.lanes = species.length(); + this.shift = log2(lanes); + + int blocks = (size + lanes - 1) / lanes; // number of blocks + int length = 1 + blocks * lanes; // size of the backing array (1-indexed) + + this.values = new long[length]; + build(values, 0, size, this.values, 1); + } + + /** + * Builds the B-tree memory layout. + * It builds the tree recursively, following an in-order traversal. + * + *

+ * Each block stores 'lanes' values at indices {@code i, i + 1, ..., i + lanes - 1} where {@code i} is the + * starting offset. The starting offset of the root block is 1. The branching factor is (1 + lanes) so each + * block can have these many children. Given the starting offset {@code i} of a block, the starting offset + * of its k-th child (ranging from {@code 0, 1, ..., k}) can be computed as {@code i + ((i + k) << shift)}. + * + * @param src is the sorted input array + * @param i is the index in the input array to read the value from + * @param size the number of values in the input array + * @param dst is the output array + * @param j is the index in the output array to write the value to + * @return the next index 'i' + */ + private int build(long[] src, int i, int size, long[] dst, int j) { + if (j < dst.length) { + for (int k = 0; k < lanes; k++) { + i = build(src, i, size, dst, j + ((j + k) << shift)); + + // Fills the B-tree as a complete tree, i.e., all levels are completely filled, + // except the last level which is filled from left to right. + // The trick is to fill the destination array between indices 1...size (inclusive / 1-indexed) + // and pad the remaining array with +infinity. + dst[j + k] = (j + k <= size) ? src[i++] : Long.MAX_VALUE; + } + i = build(src, i, size, dst, j + ((j + lanes) << shift)); + } + return i; + } + + @Override + public long floor(long key) { + Vector keyVector = LongVector.broadcast(species, key); + int i = 1, result = 1; + + while (i < values.length) { + Vector valuesVector = LongVector.fromArray(species, values, i); + int j = i + valuesVector.compare(VectorOperators.GT, keyVector).firstTrue(); + result = (j > i) ? j : result; + i += (j << shift); + } + + assert result > 1 : "key must be greater than or equal to " + values[1]; + return values[result - 1]; + } + + private static int log2(int n) { + assert (n > 0) && ((n & (n - 1)) == 0) : n + " is not a positive power of 2"; + + int result = 0; + while (n > 1) { + n >>>= 1; + result += 1; + } + + return result; + } +} diff --git a/libs/common/src/main/java20/org/opensearch/common/round/RoundableFactory.java b/libs/common/src/main/java20/org/opensearch/common/round/RoundableFactory.java new file mode 100644 index 0000000000000..09d0299edac9f --- /dev/null +++ b/libs/common/src/main/java20/org/opensearch/common/round/RoundableFactory.java @@ -0,0 +1,55 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.common.round; + +import org.opensearch.common.annotation.InternalApi; + +import jdk.incubator.vector.LongVector; +import jdk.incubator.vector.VectorSpecies; + +/** + * Factory class to create and return the fastest implementation of {@link Roundable}. + * + * @opensearch.internal + */ +@InternalApi +public final class RoundableFactory { + /** + * The maximum limit up to which linear search is used, otherwise binary search is used. + * This is because linear search is much faster on small arrays. + * Benchmark results: PR #9727 + */ + private static final int LINEAR_SEARCH_MAX_SIZE = 64; + + /** + * The preferred LongVector species with the maximal bit-size supported on this platform. + */ + private static final VectorSpecies LONG_VECTOR_SPECIES = LongVector.SPECIES_PREFERRED; + + /** + * Indicates whether the vectorized (SIMD) B-tree search implementation is supported. + * This is true when the platform has a minimum of 4 long vector lanes. + */ + private static final boolean IS_BTREE_SEARCH_SUPPORTED = LONG_VECTOR_SPECIES.length() >= 4; + + private RoundableFactory() {} + + /** + * Creates and returns the fastest implementation of {@link Roundable}. + */ + public static Roundable create(long[] values, int size, boolean useSimdIfAvailable) { + if (size <= LINEAR_SEARCH_MAX_SIZE) { + return new BidirectionalLinearSearcher(values, size); + } else if (IS_BTREE_SEARCH_SUPPORTED && useSimdIfAvailable) { + return new BtreeSearcher(values, size, LONG_VECTOR_SPECIES); + } else { + return new BinarySearcher(values, size); + } + } +} diff --git a/libs/common/src/test/java/org/opensearch/common/round/BidirectionalLinearSearcherTests.java b/libs/common/src/test/java/org/opensearch/common/round/BidirectionalLinearSearcherTests.java new file mode 100644 index 0000000000000..29f6343b3ae50 --- /dev/null +++ b/libs/common/src/test/java/org/opensearch/common/round/BidirectionalLinearSearcherTests.java @@ -0,0 +1,16 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.common.round; + +public class BidirectionalLinearSearcherTests extends RoundableTestCase { + @Override + public Roundable newInstance(long[] values, int size) { + return new BidirectionalLinearSearcher(values, size); + } +} diff --git a/libs/common/src/test/java/org/opensearch/common/round/BinarySearcherTests.java b/libs/common/src/test/java/org/opensearch/common/round/BinarySearcherTests.java new file mode 100644 index 0000000000000..7134e5ab90202 --- /dev/null +++ b/libs/common/src/test/java/org/opensearch/common/round/BinarySearcherTests.java @@ -0,0 +1,16 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.common.round; + +public class BinarySearcherTests extends RoundableTestCase { + @Override + public Roundable newInstance(long[] values, int size) { + return new BinarySearcher(values, size); + } +} diff --git a/libs/common/src/test/java20/org/opensearch/common/round/BtreeSearcherTests.java b/libs/common/src/test/java20/org/opensearch/common/round/BtreeSearcherTests.java new file mode 100644 index 0000000000000..36bc111952dab --- /dev/null +++ b/libs/common/src/test/java20/org/opensearch/common/round/BtreeSearcherTests.java @@ -0,0 +1,20 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.common.round; + +import java.util.List; + +import jdk.incubator.vector.LongVector; + +public class BtreeSearcherTests extends RoundableTestCase { + @Override + public Roundable newInstance(long[] values, int size) { + return new BtreeSearcher(values, size, randomFrom(List.of(LongVector.SPECIES_128, LongVector.SPECIES_256, LongVector.SPECIES_512))); + } +} diff --git a/server/src/main/java/org/opensearch/common/Rounding.java b/server/src/main/java/org/opensearch/common/Rounding.java index 061934f9722f5..7f02dc8b7ca32 100644 --- a/server/src/main/java/org/opensearch/common/Rounding.java +++ b/server/src/main/java/org/opensearch/common/Rounding.java @@ -41,6 +41,7 @@ import org.opensearch.common.round.RoundableFactory; import org.opensearch.common.time.DateUtils; import org.opensearch.common.unit.TimeValue; +import org.opensearch.common.util.FeatureFlags; import org.opensearch.core.common.io.stream.StreamInput; import org.opensearch.core.common.io.stream.StreamOutput; import org.opensearch.core.common.io.stream.Writeable; @@ -81,6 +82,8 @@ public abstract class Rounding implements Writeable { private static final Logger logger = LogManager.getLogger(Rounding.class); + private static final boolean IS_SIMD_ROUNDING_ENABLED = FeatureFlags.isEnabled(FeatureFlags.SIMD_ROUNDING_SETTING); + /** * A Date Time Unit * @@ -444,7 +447,7 @@ protected Prepared maybeUseArray(long minUtcMillis, long maxUtcMillis, int max) values = ArrayUtil.grow(values, i + 1); values[i++] = rounded; } - return new ArrayRounding(RoundableFactory.create(values, i), this); + return new ArrayRounding(RoundableFactory.create(values, i, IS_SIMD_ROUNDING_ENABLED), this); } } diff --git a/server/src/main/java/org/opensearch/common/settings/FeatureFlagSettings.java b/server/src/main/java/org/opensearch/common/settings/FeatureFlagSettings.java index 387b0c9753574..bcf1b7e058da4 100644 --- a/server/src/main/java/org/opensearch/common/settings/FeatureFlagSettings.java +++ b/server/src/main/java/org/opensearch/common/settings/FeatureFlagSettings.java @@ -40,7 +40,8 @@ protected FeatureFlagSettings( FeatureFlags.IDENTITY_SETTING, FeatureFlags.CONCURRENT_SEGMENT_SEARCH_SETTING, FeatureFlags.TELEMETRY_SETTING, - FeatureFlags.DATETIME_FORMATTER_CACHING_SETTING + FeatureFlags.DATETIME_FORMATTER_CACHING_SETTING, + FeatureFlags.SIMD_ROUNDING_SETTING ) ) ); diff --git a/server/src/main/java/org/opensearch/common/util/FeatureFlags.java b/server/src/main/java/org/opensearch/common/util/FeatureFlags.java index 4e9b417e3433b..bae8c93bd6185 100644 --- a/server/src/main/java/org/opensearch/common/util/FeatureFlags.java +++ b/server/src/main/java/org/opensearch/common/util/FeatureFlags.java @@ -60,6 +60,12 @@ public class FeatureFlags { */ public static final String DATETIME_FORMATTER_CACHING = "opensearch.experimental.optimization.datetime_formatter_caching.enabled"; + /** + * Gates the usage of SIMD for rounding down numbers. + * This is used extensively to round down timestamps in the date_histogram aggregation. + */ + public static final String SIMD_ROUNDING = "opensearch.experimental.simd.rounding.enabled"; + /** * Should store the settings from opensearch.yml. */ @@ -122,4 +128,6 @@ public static boolean isEnabled(Setting featureFlag) { true, Property.NodeScope ); + + public static final Setting SIMD_ROUNDING_SETTING = Setting.boolSetting(SIMD_ROUNDING, false, Property.NodeScope); } diff --git a/libs/common/src/test/java/org/opensearch/common/round/RoundableTests.java b/test/framework/src/main/java/org/opensearch/common/round/RoundableTestCase.java similarity index 60% rename from libs/common/src/test/java/org/opensearch/common/round/RoundableTests.java rename to test/framework/src/main/java/org/opensearch/common/round/RoundableTestCase.java index ae9f629c59024..1d44e289612f2 100644 --- a/libs/common/src/test/java/org/opensearch/common/round/RoundableTests.java +++ b/test/framework/src/main/java/org/opensearch/common/round/RoundableTestCase.java @@ -10,16 +10,22 @@ import org.opensearch.test.OpenSearchTestCase; -public class RoundableTests extends OpenSearchTestCase { +/** + * Base class for testing {@link Roundable} implementations. + */ +public abstract class RoundableTestCase extends OpenSearchTestCase { + + public abstract Roundable newInstance(long[] values, int size); public void testFloor() { - int size = randomIntBetween(1, 256); - long[] values = new long[size]; - for (int i = 1; i < values.length; i++) { + int size = randomIntBetween(1, 256); // number of values present in the array + int capacity = size + randomIntBetween(0, 20); // capacity of the array can be larger + long[] values = new long[capacity]; + for (int i = 1; i < size; i++) { values[i] = values[i - 1] + (randomNonNegativeLong() % 200) + 1; } - Roundable[] impls = { new BinarySearcher(values, size), new BidirectionalLinearSearcher(values, size) }; + Roundable roundable = newInstance(values, size); for (int i = 0; i < 100000; i++) { // Index of the expected round-down point. @@ -35,23 +41,16 @@ public void testFloor() { // round-down point, which will still floor to the same value. long key = expected + (randomNonNegativeLong() % delta); - for (Roundable roundable : impls) { - assertEquals(expected, roundable.floor(key)); - } + assertEquals(expected, roundable.floor(key)); } } public void testFailureCases() { Throwable throwable; - throwable = assertThrows(IllegalArgumentException.class, () -> new BinarySearcher(new long[0], 0)); - assertEquals("at least one value must be present", throwable.getMessage()); - throwable = assertThrows(IllegalArgumentException.class, () -> new BidirectionalLinearSearcher(new long[0], 0)); + throwable = assertThrows(IllegalArgumentException.class, () -> newInstance(new long[0], 0)); assertEquals("at least one value must be present", throwable.getMessage()); - - throwable = assertThrows(AssertionError.class, () -> new BinarySearcher(new long[] { 100 }, 1).floor(50)); - assertEquals("key must be greater than or equal to 100", throwable.getMessage()); - throwable = assertThrows(AssertionError.class, () -> new BidirectionalLinearSearcher(new long[] { 100 }, 1).floor(50)); + throwable = assertThrows(AssertionError.class, () -> newInstance(new long[] { 100 }, 1).floor(50)); assertEquals("key must be greater than or equal to 100", throwable.getMessage()); } } From 8afaa1e6f00e82087ee8d0afda3291a939c2fc1d Mon Sep 17 00:00:00 2001 From: Ketan Verma Date: Mon, 20 Nov 2023 11:59:40 +0530 Subject: [PATCH 02/11] Use system properties in favor of feature flags to remove dependency on the server module Signed-off-by: Ketan Verma --- distribution/src/config/opensearch.yml | 5 ----- .../org/opensearch/common/round/RoundableFactory.java | 2 +- .../org/opensearch/common/round/BtreeSearcher.java | 4 +++- .../org/opensearch/common/round/RoundableFactory.java | 9 +++++---- server/src/main/java/org/opensearch/common/Rounding.java | 5 +---- .../opensearch/common/settings/FeatureFlagSettings.java | 3 +-- .../java/org/opensearch/common/util/FeatureFlags.java | 8 -------- 7 files changed, 11 insertions(+), 25 deletions(-) diff --git a/distribution/src/config/opensearch.yml b/distribution/src/config/opensearch.yml index 1e2b01feea1ba..b7ab2e1c2309b 100644 --- a/distribution/src/config/opensearch.yml +++ b/distribution/src/config/opensearch.yml @@ -127,8 +127,3 @@ ${path.logs} # Once there is no observed impact on performance, this feature flag can be removed. # #opensearch.experimental.optimization.datetime_formatter_caching.enabled: false -# -# -# Gates the usage of SIMD for rounding down numbers. -# This is used extensively to round down timestamps in the date_histogram aggregation. -#opensearch.experimental.simd.rounding.enabled: false diff --git a/libs/common/src/main/java/org/opensearch/common/round/RoundableFactory.java b/libs/common/src/main/java/org/opensearch/common/round/RoundableFactory.java index 0ca1ae33d24bd..b7422694c3013 100644 --- a/libs/common/src/main/java/org/opensearch/common/round/RoundableFactory.java +++ b/libs/common/src/main/java/org/opensearch/common/round/RoundableFactory.java @@ -29,7 +29,7 @@ private RoundableFactory() {} /** * Creates and returns the fastest implementation of {@link Roundable}. */ - public static Roundable create(long[] values, int size, boolean useSimdIfAvailable) { + public static Roundable create(long[] values, int size) { if (size <= LINEAR_SEARCH_MAX_SIZE) { return new BidirectionalLinearSearcher(values, size); } else { diff --git a/libs/common/src/main/java20/org/opensearch/common/round/BtreeSearcher.java b/libs/common/src/main/java20/org/opensearch/common/round/BtreeSearcher.java index 624ffaaa4ba2a..06fa2c7291ade 100644 --- a/libs/common/src/main/java20/org/opensearch/common/round/BtreeSearcher.java +++ b/libs/common/src/main/java20/org/opensearch/common/round/BtreeSearcher.java @@ -97,7 +97,9 @@ public long floor(long key) { } private static int log2(int n) { - assert (n > 0) && ((n & (n - 1)) == 0) : n + " is not a positive power of 2"; + if ((n <= 0) || ((n & (n - 1)) != 0)) { + throw new IllegalArgumentException(n + " is not a positive power of 2"); + } int result = 0; while (n > 1) { diff --git a/libs/common/src/main/java20/org/opensearch/common/round/RoundableFactory.java b/libs/common/src/main/java20/org/opensearch/common/round/RoundableFactory.java index 09d0299edac9f..955a42c13582e 100644 --- a/libs/common/src/main/java20/org/opensearch/common/round/RoundableFactory.java +++ b/libs/common/src/main/java20/org/opensearch/common/round/RoundableFactory.java @@ -34,19 +34,20 @@ public final class RoundableFactory { /** * Indicates whether the vectorized (SIMD) B-tree search implementation is supported. - * This is true when the platform has a minimum of 4 long vector lanes. + * This is true when the platform has a minimum of 4 long vector lanes and the feature flag is enabled. */ - private static final boolean IS_BTREE_SEARCH_SUPPORTED = LONG_VECTOR_SPECIES.length() >= 4; + private static final boolean IS_BTREE_SEARCH_SUPPORTED = LONG_VECTOR_SPECIES.length() >= 4 + && "true".equalsIgnoreCase(System.getProperty("opensearch.experimental.feature.simd.rounding.enabled")); private RoundableFactory() {} /** * Creates and returns the fastest implementation of {@link Roundable}. */ - public static Roundable create(long[] values, int size, boolean useSimdIfAvailable) { + public static Roundable create(long[] values, int size) { if (size <= LINEAR_SEARCH_MAX_SIZE) { return new BidirectionalLinearSearcher(values, size); - } else if (IS_BTREE_SEARCH_SUPPORTED && useSimdIfAvailable) { + } else if (IS_BTREE_SEARCH_SUPPORTED) { return new BtreeSearcher(values, size, LONG_VECTOR_SPECIES); } else { return new BinarySearcher(values, size); diff --git a/server/src/main/java/org/opensearch/common/Rounding.java b/server/src/main/java/org/opensearch/common/Rounding.java index 7f02dc8b7ca32..061934f9722f5 100644 --- a/server/src/main/java/org/opensearch/common/Rounding.java +++ b/server/src/main/java/org/opensearch/common/Rounding.java @@ -41,7 +41,6 @@ import org.opensearch.common.round.RoundableFactory; import org.opensearch.common.time.DateUtils; import org.opensearch.common.unit.TimeValue; -import org.opensearch.common.util.FeatureFlags; import org.opensearch.core.common.io.stream.StreamInput; import org.opensearch.core.common.io.stream.StreamOutput; import org.opensearch.core.common.io.stream.Writeable; @@ -82,8 +81,6 @@ public abstract class Rounding implements Writeable { private static final Logger logger = LogManager.getLogger(Rounding.class); - private static final boolean IS_SIMD_ROUNDING_ENABLED = FeatureFlags.isEnabled(FeatureFlags.SIMD_ROUNDING_SETTING); - /** * A Date Time Unit * @@ -447,7 +444,7 @@ protected Prepared maybeUseArray(long minUtcMillis, long maxUtcMillis, int max) values = ArrayUtil.grow(values, i + 1); values[i++] = rounded; } - return new ArrayRounding(RoundableFactory.create(values, i, IS_SIMD_ROUNDING_ENABLED), this); + return new ArrayRounding(RoundableFactory.create(values, i), this); } } diff --git a/server/src/main/java/org/opensearch/common/settings/FeatureFlagSettings.java b/server/src/main/java/org/opensearch/common/settings/FeatureFlagSettings.java index bcf1b7e058da4..387b0c9753574 100644 --- a/server/src/main/java/org/opensearch/common/settings/FeatureFlagSettings.java +++ b/server/src/main/java/org/opensearch/common/settings/FeatureFlagSettings.java @@ -40,8 +40,7 @@ protected FeatureFlagSettings( FeatureFlags.IDENTITY_SETTING, FeatureFlags.CONCURRENT_SEGMENT_SEARCH_SETTING, FeatureFlags.TELEMETRY_SETTING, - FeatureFlags.DATETIME_FORMATTER_CACHING_SETTING, - FeatureFlags.SIMD_ROUNDING_SETTING + FeatureFlags.DATETIME_FORMATTER_CACHING_SETTING ) ) ); diff --git a/server/src/main/java/org/opensearch/common/util/FeatureFlags.java b/server/src/main/java/org/opensearch/common/util/FeatureFlags.java index bae8c93bd6185..4e9b417e3433b 100644 --- a/server/src/main/java/org/opensearch/common/util/FeatureFlags.java +++ b/server/src/main/java/org/opensearch/common/util/FeatureFlags.java @@ -60,12 +60,6 @@ public class FeatureFlags { */ public static final String DATETIME_FORMATTER_CACHING = "opensearch.experimental.optimization.datetime_formatter_caching.enabled"; - /** - * Gates the usage of SIMD for rounding down numbers. - * This is used extensively to round down timestamps in the date_histogram aggregation. - */ - public static final String SIMD_ROUNDING = "opensearch.experimental.simd.rounding.enabled"; - /** * Should store the settings from opensearch.yml. */ @@ -128,6 +122,4 @@ public static boolean isEnabled(Setting featureFlag) { true, Property.NodeScope ); - - public static final Setting SIMD_ROUNDING_SETTING = Setting.boolSetting(SIMD_ROUNDING, false, Property.NodeScope); } From 5501806a9d6a10cbaa089020cc4121b42c7dbf89 Mon Sep 17 00:00:00 2001 From: Ketan Verma Date: Mon, 4 Dec 2023 17:19:43 +0530 Subject: [PATCH 03/11] Removed Java 20 test sources to simplify builds Signed-off-by: Ketan Verma --- benchmarks/build.gradle | 2 +- libs/common/build.gradle | 46 ++-------- .../common/round/BtreeSearcher.java | 52 +++++------ .../common/round/RoundableFactory.java | 11 +-- .../BidirectionalLinearSearcherTests.java | 16 ---- .../common/round/BinarySearcherTests.java | 16 ---- .../common/round/RoundableTests.java | 89 +++++++++++++++++++ .../common/round/BtreeSearcherTests.java | 20 ----- .../common/round/RoundableTestCase.java | 56 ------------ 9 files changed, 121 insertions(+), 187 deletions(-) delete mode 100644 libs/common/src/test/java/org/opensearch/common/round/BidirectionalLinearSearcherTests.java delete mode 100644 libs/common/src/test/java/org/opensearch/common/round/BinarySearcherTests.java create mode 100644 libs/common/src/test/java/org/opensearch/common/round/RoundableTests.java delete mode 100644 libs/common/src/test/java20/org/opensearch/common/round/BtreeSearcherTests.java delete mode 100644 test/framework/src/main/java/org/opensearch/common/round/RoundableTestCase.java diff --git a/benchmarks/build.gradle b/benchmarks/build.gradle index 39f27f7f66b54..3fb9bb5ace5fd 100644 --- a/benchmarks/build.gradle +++ b/benchmarks/build.gradle @@ -66,7 +66,7 @@ run.executable = "${BuildParams.runtimeJavaHome}/bin/java" // Add support for incubator modules on supported Java versions. if (BuildParams.runtimeJavaVersion >= JavaVersion.VERSION_20) { - run.jvmArgs += ["--add-modules=jdk.incubator.vector"] + run.jvmArgs += ['--add-modules=jdk.incubator.vector'] } // classes generated by JMH can use all sorts of forbidden APIs but we have no influence at all and cannot exclude these classes diff --git a/libs/common/build.gradle b/libs/common/build.gradle index 5f5bc036f2cc9..155b253ec08c2 100644 --- a/libs/common/build.gradle +++ b/libs/common/build.gradle @@ -52,31 +52,14 @@ if (BuildParams.runtimeJavaVersion >= JavaVersion.VERSION_20) { srcDirs = ['src/main/java20'] } } - - java20Test { - java { - srcDirs = ['src/test/java20'] - } - } } configurations { java20Implementation.extendsFrom(implementation) - java20TestImplementation.extendsFrom(implementation) } dependencies { java20Implementation sourceSets.main.output - - // Adding Java 20 sources as compile-only to make them visible during compilation, - // but using the multi-release JAR output to provide the runtime functionality. - // This helps avoid JarHell problems when an overridden class is present with the same name. - java20TestCompileOnly sourceSets.java20.output - java20TestImplementation files(jar.archiveFile) - java20TestImplementation sourceSets.test.output - java20TestImplementation(project(':test:framework')) { - exclude group: 'org.opensearch', module: 'opensearch-common' - } } compileJava20Java { @@ -86,13 +69,6 @@ if (BuildParams.runtimeJavaVersion >= JavaVersion.VERSION_20) { options.compilerArgs -= '-Werror' // use of incubator modules is reported as a warning } - compileJava20TestJava { - sourceCompatibility = JavaVersion.VERSION_20 - targetCompatibility = JavaVersion.VERSION_20 - options.compilerArgs += ['--add-modules', 'jdk.incubator.vector'] - options.compilerArgs -= '-Werror' // use of incubator modules is reported as a warning - } - jar { metaInf { into 'versions/20' @@ -101,23 +77,17 @@ if (BuildParams.runtimeJavaVersion >= JavaVersion.VERSION_20) { manifest.attributes('Multi-Release': 'true') } - forbiddenApisJava20 { - failOnMissingClasses = false - ignoreSignaturesOfMissingClasses = true + test { + // Adds the multi-release JAR to the classpath when executing tests. + // This allows newer sources to be picked up at test runtime (if supported). + classpath += files(jar.archiveFile) + // Removes the "main" sources from the classpath to avoid JarHell problems as + // the multi-release JAR already contains those classes. + classpath -= sourceSets.main.output } - forbiddenApisJava20Test { + forbiddenApisJava20 { failOnMissingClasses = false ignoreSignaturesOfMissingClasses = true } - - tasks.register('testJava20', Test) { - dependsOn jar - shouldRunAfter test - group = 'verification' - testClassesDirs = sourceSets.java20Test.output.classesDirs - classpath = sourceSets.java20Test.runtimeClasspath - } - - check.dependsOn('testJava20') } diff --git a/libs/common/src/main/java20/org/opensearch/common/round/BtreeSearcher.java b/libs/common/src/main/java20/org/opensearch/common/round/BtreeSearcher.java index 06fa2c7291ade..626fb6e6b810e 100644 --- a/libs/common/src/main/java20/org/opensearch/common/round/BtreeSearcher.java +++ b/libs/common/src/main/java20/org/opensearch/common/round/BtreeSearcher.java @@ -22,27 +22,22 @@ */ @InternalApi class BtreeSearcher implements Roundable { - private final VectorSpecies species; - private final int lanes; - private final int shift; + private static final VectorSpecies LONG_VECTOR_SPECIES = LongVector.SPECIES_PREFERRED; + private static final int LANES = LONG_VECTOR_SPECIES.length(); + private static final int SHIFT = log2(LANES); + private final long[] values; + private final long minValue; BtreeSearcher(long[] values, int size) { - this(values, size, LongVector.SPECIES_PREFERRED); - } - - BtreeSearcher(long[] values, int size, VectorSpecies species) { if (size <= 0) { throw new IllegalArgumentException("at least one value must be present"); } - this.species = species; - this.lanes = species.length(); - this.shift = log2(lanes); - - int blocks = (size + lanes - 1) / lanes; // number of blocks - int length = 1 + blocks * lanes; // size of the backing array (1-indexed) + int blocks = (size + LANES - 1) / LANES; // number of blocks + int length = 1 + blocks * LANES; // size of the backing array (1-indexed) + this.minValue = values[0]; this.values = new long[length]; build(values, 0, size, this.values, 1); } @@ -64,10 +59,10 @@ class BtreeSearcher implements Roundable { * @param j is the index in the output array to write the value to * @return the next index 'i' */ - private int build(long[] src, int i, int size, long[] dst, int j) { + private static int build(long[] src, int i, int size, long[] dst, int j) { if (j < dst.length) { - for (int k = 0; k < lanes; k++) { - i = build(src, i, size, dst, j + ((j + k) << shift)); + for (int k = 0; k < LANES; k++) { + i = build(src, i, size, dst, j + ((j + k) << SHIFT)); // Fills the B-tree as a complete tree, i.e., all levels are completely filled, // except the last level which is filled from left to right. @@ -75,38 +70,31 @@ private int build(long[] src, int i, int size, long[] dst, int j) { // and pad the remaining array with +infinity. dst[j + k] = (j + k <= size) ? src[i++] : Long.MAX_VALUE; } - i = build(src, i, size, dst, j + ((j + lanes) << shift)); + i = build(src, i, size, dst, j + ((j + LANES) << SHIFT)); } return i; } @Override public long floor(long key) { - Vector keyVector = LongVector.broadcast(species, key); + Vector keyVector = LongVector.broadcast(LONG_VECTOR_SPECIES, key); int i = 1, result = 1; while (i < values.length) { - Vector valuesVector = LongVector.fromArray(species, values, i); + Vector valuesVector = LongVector.fromArray(LONG_VECTOR_SPECIES, values, i); int j = i + valuesVector.compare(VectorOperators.GT, keyVector).firstTrue(); result = (j > i) ? j : result; - i += (j << shift); + i += (j << SHIFT); } - assert result > 1 : "key must be greater than or equal to " + values[1]; + assert result > 1 : "key must be greater than or equal to " + minValue; return values[result - 1]; } - private static int log2(int n) { - if ((n <= 0) || ((n & (n - 1)) != 0)) { - throw new IllegalArgumentException(n + " is not a positive power of 2"); + private static int log2(int num) { + if ((num <= 0) || ((num & (num - 1)) != 0)) { + throw new IllegalArgumentException(num + " is not a positive power of 2"); } - - int result = 0; - while (n > 1) { - n >>>= 1; - result += 1; - } - - return result; + return 32 - Integer.numberOfLeadingZeros(num - 1); } } diff --git a/libs/common/src/main/java20/org/opensearch/common/round/RoundableFactory.java b/libs/common/src/main/java20/org/opensearch/common/round/RoundableFactory.java index 955a42c13582e..4afc033408ca4 100644 --- a/libs/common/src/main/java20/org/opensearch/common/round/RoundableFactory.java +++ b/libs/common/src/main/java20/org/opensearch/common/round/RoundableFactory.java @@ -11,7 +11,6 @@ import org.opensearch.common.annotation.InternalApi; import jdk.incubator.vector.LongVector; -import jdk.incubator.vector.VectorSpecies; /** * Factory class to create and return the fastest implementation of {@link Roundable}. @@ -27,16 +26,12 @@ public final class RoundableFactory { */ private static final int LINEAR_SEARCH_MAX_SIZE = 64; - /** - * The preferred LongVector species with the maximal bit-size supported on this platform. - */ - private static final VectorSpecies LONG_VECTOR_SPECIES = LongVector.SPECIES_PREFERRED; - /** * Indicates whether the vectorized (SIMD) B-tree search implementation is supported. * This is true when the platform has a minimum of 4 long vector lanes and the feature flag is enabled. + * Package-private for testing. */ - private static final boolean IS_BTREE_SEARCH_SUPPORTED = LONG_VECTOR_SPECIES.length() >= 4 + static final boolean IS_BTREE_SEARCH_SUPPORTED = LongVector.SPECIES_PREFERRED.length() >= 4 && "true".equalsIgnoreCase(System.getProperty("opensearch.experimental.feature.simd.rounding.enabled")); private RoundableFactory() {} @@ -48,7 +43,7 @@ public static Roundable create(long[] values, int size) { if (size <= LINEAR_SEARCH_MAX_SIZE) { return new BidirectionalLinearSearcher(values, size); } else if (IS_BTREE_SEARCH_SUPPORTED) { - return new BtreeSearcher(values, size, LONG_VECTOR_SPECIES); + return new BtreeSearcher(values, size); } else { return new BinarySearcher(values, size); } diff --git a/libs/common/src/test/java/org/opensearch/common/round/BidirectionalLinearSearcherTests.java b/libs/common/src/test/java/org/opensearch/common/round/BidirectionalLinearSearcherTests.java deleted file mode 100644 index 29f6343b3ae50..0000000000000 --- a/libs/common/src/test/java/org/opensearch/common/round/BidirectionalLinearSearcherTests.java +++ /dev/null @@ -1,16 +0,0 @@ -/* - * SPDX-License-Identifier: Apache-2.0 - * - * The OpenSearch Contributors require contributions made to - * this file be licensed under the Apache-2.0 license or a - * compatible open source license. - */ - -package org.opensearch.common.round; - -public class BidirectionalLinearSearcherTests extends RoundableTestCase { - @Override - public Roundable newInstance(long[] values, int size) { - return new BidirectionalLinearSearcher(values, size); - } -} diff --git a/libs/common/src/test/java/org/opensearch/common/round/BinarySearcherTests.java b/libs/common/src/test/java/org/opensearch/common/round/BinarySearcherTests.java deleted file mode 100644 index 7134e5ab90202..0000000000000 --- a/libs/common/src/test/java/org/opensearch/common/round/BinarySearcherTests.java +++ /dev/null @@ -1,16 +0,0 @@ -/* - * SPDX-License-Identifier: Apache-2.0 - * - * The OpenSearch Contributors require contributions made to - * this file be licensed under the Apache-2.0 license or a - * compatible open source license. - */ - -package org.opensearch.common.round; - -public class BinarySearcherTests extends RoundableTestCase { - @Override - public Roundable newInstance(long[] values, int size) { - return new BinarySearcher(values, size); - } -} diff --git a/libs/common/src/test/java/org/opensearch/common/round/RoundableTests.java b/libs/common/src/test/java/org/opensearch/common/round/RoundableTests.java new file mode 100644 index 0000000000000..ef4b6100e1676 --- /dev/null +++ b/libs/common/src/test/java/org/opensearch/common/round/RoundableTests.java @@ -0,0 +1,89 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.common.round; + +import org.opensearch.common.SuppressForbidden; +import org.opensearch.test.OpenSearchTestCase; +import org.junit.BeforeClass; + +public class RoundableTests extends OpenSearchTestCase { + + @BeforeClass + @SuppressForbidden(reason = "Sets the feature flag system property to a random value") + public static void setupClass() { + System.setProperty("opensearch.experimental.feature.simd.rounding.enabled", String.valueOf(randomBoolean())); + } + + public void testRoundingEmptyArray() { + Throwable throwable = assertThrows(IllegalArgumentException.class, () -> RoundableFactory.create(new long[0], 0)); + assertEquals("at least one value must be present", throwable.getMessage()); + } + + public void testRoundingSmallArray() { + int size = randomIntBetween(1, 64); + long[] values = randomArrayOfSortedValues(size); + Roundable roundable = RoundableFactory.create(values, size); + + assertEquals("BidirectionalLinearSearcher", roundable.getClass().getSimpleName()); + assertRounding(roundable, values, size); + } + + @SuppressForbidden(reason = "Reads a package-private static field using reflection as it's only available on Java 20 and above") + public void testRoundingLargeArray() { + int size = randomIntBetween(65, 256); + long[] values = randomArrayOfSortedValues(size); + Roundable roundable = RoundableFactory.create(values, size); + + boolean isBtreeSearchSupported; + try { + // Not supported below Java 20. + isBtreeSearchSupported = RoundableFactory.class.getDeclaredField("IS_BTREE_SEARCH_SUPPORTED").getBoolean(null); + } catch (NoSuchFieldException e) { + isBtreeSearchSupported = false; + } catch (IllegalAccessException e) { + throw new RuntimeException(e); + } + + assertEquals(isBtreeSearchSupported ? "BtreeSearcher" : "BinarySearcher", roundable.getClass().getSimpleName()); + assertRounding(roundable, values, size); + } + + private void assertRounding(Roundable roundable, long[] values, int size) { + for (int i = 0; i < 100000; i++) { + // Index of the expected round-down point. + int idx = randomIntBetween(0, size - 1); + + // Value of the expected round-down point. + long expected = values[idx]; + + // Delta between the expected and the next round-down point. + long delta = (idx < size - 1) ? (values[idx + 1] - values[idx]) : 200; + + // Adding a random delta between 0 (inclusive) and delta (exclusive) to the expected + // round-down point, which will still floor to the same value. + long key = expected + (randomNonNegativeLong() % delta); + + assertEquals(expected, roundable.floor(key)); + } + + Throwable throwable = assertThrows(AssertionError.class, () -> roundable.floor(values[0] - 1)); + assertEquals("key must be greater than or equal to " + values[0], throwable.getMessage()); + } + + private static long[] randomArrayOfSortedValues(int size) { + int capacity = size + randomInt(20); // May be slightly more than the size. + long[] values = new long[capacity]; + + for (int i = 1; i < size; i++) { + values[i] = values[i - 1] + (randomNonNegativeLong() % 200) + 1; + } + + return values; + } +} diff --git a/libs/common/src/test/java20/org/opensearch/common/round/BtreeSearcherTests.java b/libs/common/src/test/java20/org/opensearch/common/round/BtreeSearcherTests.java deleted file mode 100644 index 36bc111952dab..0000000000000 --- a/libs/common/src/test/java20/org/opensearch/common/round/BtreeSearcherTests.java +++ /dev/null @@ -1,20 +0,0 @@ -/* - * SPDX-License-Identifier: Apache-2.0 - * - * The OpenSearch Contributors require contributions made to - * this file be licensed under the Apache-2.0 license or a - * compatible open source license. - */ - -package org.opensearch.common.round; - -import java.util.List; - -import jdk.incubator.vector.LongVector; - -public class BtreeSearcherTests extends RoundableTestCase { - @Override - public Roundable newInstance(long[] values, int size) { - return new BtreeSearcher(values, size, randomFrom(List.of(LongVector.SPECIES_128, LongVector.SPECIES_256, LongVector.SPECIES_512))); - } -} diff --git a/test/framework/src/main/java/org/opensearch/common/round/RoundableTestCase.java b/test/framework/src/main/java/org/opensearch/common/round/RoundableTestCase.java deleted file mode 100644 index 1d44e289612f2..0000000000000 --- a/test/framework/src/main/java/org/opensearch/common/round/RoundableTestCase.java +++ /dev/null @@ -1,56 +0,0 @@ -/* - * SPDX-License-Identifier: Apache-2.0 - * - * The OpenSearch Contributors require contributions made to - * this file be licensed under the Apache-2.0 license or a - * compatible open source license. - */ - -package org.opensearch.common.round; - -import org.opensearch.test.OpenSearchTestCase; - -/** - * Base class for testing {@link Roundable} implementations. - */ -public abstract class RoundableTestCase extends OpenSearchTestCase { - - public abstract Roundable newInstance(long[] values, int size); - - public void testFloor() { - int size = randomIntBetween(1, 256); // number of values present in the array - int capacity = size + randomIntBetween(0, 20); // capacity of the array can be larger - long[] values = new long[capacity]; - for (int i = 1; i < size; i++) { - values[i] = values[i - 1] + (randomNonNegativeLong() % 200) + 1; - } - - Roundable roundable = newInstance(values, size); - - for (int i = 0; i < 100000; i++) { - // Index of the expected round-down point. - int idx = randomIntBetween(0, size - 1); - - // Value of the expected round-down point. - long expected = values[idx]; - - // Delta between the expected and the next round-down point. - long delta = (idx < size - 1) ? (values[idx + 1] - values[idx]) : 200; - - // Adding a random delta between 0 (inclusive) and delta (exclusive) to the expected - // round-down point, which will still floor to the same value. - long key = expected + (randomNonNegativeLong() % delta); - - assertEquals(expected, roundable.floor(key)); - } - } - - public void testFailureCases() { - Throwable throwable; - - throwable = assertThrows(IllegalArgumentException.class, () -> newInstance(new long[0], 0)); - assertEquals("at least one value must be present", throwable.getMessage()); - throwable = assertThrows(AssertionError.class, () -> newInstance(new long[] { 100 }, 1).floor(50)); - assertEquals("key must be greater than or equal to 100", throwable.getMessage()); - } -} From 2cb97a565e0f039293344f1ee57b69f7d258deea Mon Sep 17 00:00:00 2001 From: Ketan Verma Date: Thu, 7 Dec 2023 13:55:35 +0530 Subject: [PATCH 04/11] Remove the use of forbidden APIs in unit-tests Signed-off-by: Ketan Verma --- libs/common/build.gradle | 11 +++++++-- .../common/round/RoundableTests.java | 23 +++---------------- 2 files changed, 12 insertions(+), 22 deletions(-) diff --git a/libs/common/build.gradle b/libs/common/build.gradle index 155b253ec08c2..f8dc8159b36de 100644 --- a/libs/common/build.gradle +++ b/libs/common/build.gradle @@ -63,7 +63,6 @@ if (BuildParams.runtimeJavaVersion >= JavaVersion.VERSION_20) { } compileJava20Java { - sourceCompatibility = JavaVersion.VERSION_20 targetCompatibility = JavaVersion.VERSION_20 options.compilerArgs += ['--add-modules', 'jdk.incubator.vector'] options.compilerArgs -= '-Werror' // use of incubator modules is reported as a warning @@ -77,7 +76,7 @@ if (BuildParams.runtimeJavaVersion >= JavaVersion.VERSION_20) { manifest.attributes('Multi-Release': 'true') } - test { + tasks.withType(Test).configureEach { // Adds the multi-release JAR to the classpath when executing tests. // This allows newer sources to be picked up at test runtime (if supported). classpath += files(jar.archiveFile) @@ -86,6 +85,14 @@ if (BuildParams.runtimeJavaVersion >= JavaVersion.VERSION_20) { classpath -= sourceSets.main.output } + tasks.register('roundableSimdTest', Test) { + group 'verification' + include '**/RoundableTests.class' + systemProperty 'opensearch.experimental.feature.simd.rounding.enabled', 'true' + } + + check.dependsOn(roundableSimdTest) + forbiddenApisJava20 { failOnMissingClasses = false ignoreSignaturesOfMissingClasses = true diff --git a/libs/common/src/test/java/org/opensearch/common/round/RoundableTests.java b/libs/common/src/test/java/org/opensearch/common/round/RoundableTests.java index ef4b6100e1676..1c0a36fe28ee8 100644 --- a/libs/common/src/test/java/org/opensearch/common/round/RoundableTests.java +++ b/libs/common/src/test/java/org/opensearch/common/round/RoundableTests.java @@ -8,17 +8,11 @@ package org.opensearch.common.round; -import org.opensearch.common.SuppressForbidden; import org.opensearch.test.OpenSearchTestCase; -import org.junit.BeforeClass; -public class RoundableTests extends OpenSearchTestCase { +import java.util.List; - @BeforeClass - @SuppressForbidden(reason = "Sets the feature flag system property to a random value") - public static void setupClass() { - System.setProperty("opensearch.experimental.feature.simd.rounding.enabled", String.valueOf(randomBoolean())); - } +public class RoundableTests extends OpenSearchTestCase { public void testRoundingEmptyArray() { Throwable throwable = assertThrows(IllegalArgumentException.class, () -> RoundableFactory.create(new long[0], 0)); @@ -34,23 +28,12 @@ public void testRoundingSmallArray() { assertRounding(roundable, values, size); } - @SuppressForbidden(reason = "Reads a package-private static field using reflection as it's only available on Java 20 and above") public void testRoundingLargeArray() { int size = randomIntBetween(65, 256); long[] values = randomArrayOfSortedValues(size); Roundable roundable = RoundableFactory.create(values, size); - boolean isBtreeSearchSupported; - try { - // Not supported below Java 20. - isBtreeSearchSupported = RoundableFactory.class.getDeclaredField("IS_BTREE_SEARCH_SUPPORTED").getBoolean(null); - } catch (NoSuchFieldException e) { - isBtreeSearchSupported = false; - } catch (IllegalAccessException e) { - throw new RuntimeException(e); - } - - assertEquals(isBtreeSearchSupported ? "BtreeSearcher" : "BinarySearcher", roundable.getClass().getSimpleName()); + assertTrue(List.of("BtreeSearcher", "BinarySearcher").contains(roundable.getClass().getSimpleName())); assertRounding(roundable, values, size); } From 1e455aaa008f34ad271edc33f89b38e471174a3e Mon Sep 17 00:00:00 2001 From: Ketan Verma Date: Thu, 7 Dec 2023 14:31:52 +0530 Subject: [PATCH 05/11] Migrate to the recommended usage for custom test task classpath Signed-off-by: Ketan Verma --- libs/common/build.gradle | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/libs/common/build.gradle b/libs/common/build.gradle index f8dc8159b36de..5dd25fde56a0f 100644 --- a/libs/common/build.gradle +++ b/libs/common/build.gradle @@ -77,6 +77,12 @@ if (BuildParams.runtimeJavaVersion >= JavaVersion.VERSION_20) { } tasks.withType(Test).configureEach { + // Relying on the convention for Test.classpath in custom Test tasks has been deprecated + // and scheduled to be removed in Gradle 9.0. Below lines are added from the migration guide: + // https://docs.gradle.org/8.5/userguide/upgrading_version_8.html#test_task_default_classpath + testClassesDirs = testing.suites.test.sources.output.classesDirs + classpath = testing.suites.test.sources.runtimeClasspath + // Adds the multi-release JAR to the classpath when executing tests. // This allows newer sources to be picked up at test runtime (if supported). classpath += files(jar.archiveFile) From 2969070f7df7e3d0c7abdaf2e53ab8f80f9bc360 Mon Sep 17 00:00:00 2001 From: Andriy Redko Date: Fri, 8 Dec 2023 11:45:38 -0500 Subject: [PATCH 06/11] Switch benchmarks module to multi-release one Signed-off-by: Andriy Redko --- benchmarks/build.gradle | 40 ++++- .../common/round/RoundableBenchmark.java | 18 +-- .../common/round/RoundableBenchmark.java | 150 ++++++++++++++++++ 3 files changed, 187 insertions(+), 21 deletions(-) create mode 100644 benchmarks/src/main/java20/org/opensearch/common/round/RoundableBenchmark.java diff --git a/benchmarks/build.gradle b/benchmarks/build.gradle index 3fb9bb5ace5fd..238fdda4dd0a6 100644 --- a/benchmarks/build.gradle +++ b/benchmarks/build.gradle @@ -64,11 +64,6 @@ compileJava.options.compilerArgs.addAll(["-processor", "org.openjdk.jmh.generato run.executable = "${BuildParams.runtimeJavaHome}/bin/java" -// Add support for incubator modules on supported Java versions. -if (BuildParams.runtimeJavaVersion >= JavaVersion.VERSION_20) { - run.jvmArgs += ['--add-modules=jdk.incubator.vector'] -} - // classes generated by JMH can use all sorts of forbidden APIs but we have no influence at all and cannot exclude these classes disableTasks('forbiddenApisMain') @@ -89,3 +84,38 @@ spotless { targetExclude 'src/main/generated/**/*.java' } } + +if (BuildParams.runtimeJavaVersion >= JavaVersion.VERSION_20) { + // Add support for incubator modules on supported Java versions. + run.jvmArgs += ['--add-modules=jdk.incubator.vector'] + evaluationDependsOn(':libs:opensearch-common') + + sourceSets { + java20 { + java { + srcDirs = ['src/main/java20'] + } + } + } + + configurations { + java20Implementation.extendsFrom(implementation) + } + + dependencies { + java20Implementation sourceSets.main.output + java20Implementation project(':libs:opensearch-common').sourceSets.java20.output + } + + compileJava20Java { + targetCompatibility = JavaVersion.VERSION_20 + } + + jar { + metaInf { + into 'versions/20' + from sourceSets.java20.output + } + manifest.attributes('Multi-Release': 'true') + } +} diff --git a/benchmarks/src/main/java/org/opensearch/common/round/RoundableBenchmark.java b/benchmarks/src/main/java/org/opensearch/common/round/RoundableBenchmark.java index 3b0e5fbf1a7d5..4e07af452968b 100644 --- a/benchmarks/src/main/java/org/opensearch/common/round/RoundableBenchmark.java +++ b/benchmarks/src/main/java/org/opensearch/common/round/RoundableBenchmark.java @@ -20,7 +20,6 @@ import org.openjdk.jmh.annotations.Warmup; import org.openjdk.jmh.infra.Blackhole; -import java.lang.invoke.MethodHandles; import java.util.Random; import java.util.function.Supplier; @@ -84,7 +83,7 @@ public static class Options { "256" }) public Integer size; - @Param({ "linear", "binary", "btree" }) + @Param({ "binary", "linear" }) public String type; @Param({ "uniform", "skewed_edge", "skewed_center" }) @@ -94,7 +93,7 @@ public static class Options { public Supplier supplier; @Setup - public void setup() throws ClassNotFoundException, IllegalAccessException { + public void setup() { Random random = new Random(size); long[] values = new long[size]; for (int i = 1; i < values.length; i++) { @@ -136,19 +135,6 @@ public void setup() throws ClassNotFoundException, IllegalAccessException { case "linear": supplier = () -> new BidirectionalLinearSearcher(values, size); break; - case "btree": - // Not supported below Java 20. - // Using MethodHandles to reflectively look up the class (if available) in order to - // avoid setting up separate Java 20 sources with mostly duplicate code. - Class clz = MethodHandles.lookup().findClass("org.opensearch.common.round.BtreeSearcher"); - supplier = () -> { - try { - return (Roundable) clz.getDeclaredConstructor(long[].class, int.class).newInstance(values, size); - } catch (Exception e) { - throw new RuntimeException(e); - } - }; - break; default: throw new IllegalArgumentException("invalid type: " + type); } diff --git a/benchmarks/src/main/java20/org/opensearch/common/round/RoundableBenchmark.java b/benchmarks/src/main/java20/org/opensearch/common/round/RoundableBenchmark.java new file mode 100644 index 0000000000000..bccbd758b3e29 --- /dev/null +++ b/benchmarks/src/main/java20/org/opensearch/common/round/RoundableBenchmark.java @@ -0,0 +1,150 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.common.round; + +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.BenchmarkMode; +import org.openjdk.jmh.annotations.Fork; +import org.openjdk.jmh.annotations.Measurement; +import org.openjdk.jmh.annotations.Mode; +import org.openjdk.jmh.annotations.Param; +import org.openjdk.jmh.annotations.Scope; +import org.openjdk.jmh.annotations.Setup; +import org.openjdk.jmh.annotations.State; +import org.openjdk.jmh.annotations.Warmup; +import org.openjdk.jmh.infra.Blackhole; + +import java.util.Random; +import java.util.function.Supplier; + +@Fork(value = 3) +@Warmup(iterations = 3, time = 1) +@Measurement(iterations = 1, time = 1) +@BenchmarkMode(Mode.Throughput) +public class RoundableBenchmark { + + @Benchmark + public void floor(Blackhole bh, Options opts) { + Roundable roundable = opts.supplier.get(); + for (long key : opts.queries) { + bh.consume(roundable.floor(key)); + } + } + + @State(Scope.Benchmark) + public static class Options { + @Param({ + "1", + "2", + "3", + "4", + "5", + "6", + "7", + "8", + "9", + "10", + "12", + "14", + "16", + "18", + "20", + "22", + "24", + "26", + "29", + "32", + "37", + "41", + "45", + "49", + "54", + "60", + "64", + "74", + "83", + "90", + "98", + "108", + "118", + "128", + "144", + "159", + "171", + "187", + "204", + "229", + "256" }) + public Integer size; + + @Param({ "linear", "binary", "btree" }) + public String type; + + @Param({ "uniform", "skewed_edge", "skewed_center" }) + public String distribution; + + public long[] queries; + public Supplier supplier; + + @Setup + public void setup() throws ClassNotFoundException, IllegalAccessException { + Random random = new Random(size); + long[] values = new long[size]; + for (int i = 1; i < values.length; i++) { + values[i] = values[i - 1] + 100; + } + + long range = values[values.length - 1] - values[0] + 100; + long mean, stddev; + queries = new long[1000000]; + + switch (distribution) { + case "uniform": // all values equally likely. + for (int i = 0; i < queries.length; i++) { + queries[i] = values[0] + (nextPositiveLong(random) % range); + } + break; + case "skewed_edge": // distribution centered at p90 with ± 5% stddev. + mean = values[0] + (long) (range * 0.9); + stddev = (long) (range * 0.05); + for (int i = 0; i < queries.length; i++) { + queries[i] = Math.max(values[0], mean + (long) (random.nextGaussian() * stddev)); + } + break; + case "skewed_center": // distribution centered at p50 with ± 5% stddev. + mean = values[0] + (long) (range * 0.5); + stddev = (long) (range * 0.05); + for (int i = 0; i < queries.length; i++) { + queries[i] = Math.max(values[0], mean + (long) (random.nextGaussian() * stddev)); + } + break; + default: + throw new IllegalArgumentException("invalid distribution: " + distribution); + } + + switch (type) { + case "binary": + supplier = () -> new BinarySearcher(values, size); + break; + case "linear": + supplier = () -> new BidirectionalLinearSearcher(values, size); + break; + case "btree": + supplier = () -> new BtreeSearcher(values, size); + break; + default: + throw new IllegalArgumentException("invalid type: " + type); + } + } + + private static long nextPositiveLong(Random random) { + return random.nextLong() & Long.MAX_VALUE; + } + } +} From 2286af19603c0f3c2d8c3b15a7617837d46a44c0 Mon Sep 17 00:00:00 2001 From: Andriy Redko Date: Mon, 11 Dec 2023 12:23:47 -0500 Subject: [PATCH 07/11] Add JMH annotation processing for JDK-20+ sources Signed-off-by: Andriy Redko --- benchmarks/build.gradle | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/benchmarks/build.gradle b/benchmarks/build.gradle index 238fdda4dd0a6..c1cc8438faa46 100644 --- a/benchmarks/build.gradle +++ b/benchmarks/build.gradle @@ -105,10 +105,12 @@ if (BuildParams.runtimeJavaVersion >= JavaVersion.VERSION_20) { dependencies { java20Implementation sourceSets.main.output java20Implementation project(':libs:opensearch-common').sourceSets.java20.output + java20AnnotationProcessor "org.openjdk.jmh:jmh-generator-annprocess:$versions.jmh" } compileJava20Java { targetCompatibility = JavaVersion.VERSION_20 + options.compilerArgs.addAll(["-processor", "org.openjdk.jmh.generators.BenchmarkProcessor"]) } jar { @@ -118,4 +120,7 @@ if (BuildParams.runtimeJavaVersion >= JavaVersion.VERSION_20) { } manifest.attributes('Multi-Release': 'true') } + + // classes generated by JMH can use all sorts of forbidden APIs but we have no influence at all and cannot exclude these classes + disableTasks('forbiddenApisJava20') } From b7cb42af0c64366ee0f9cdd4517a0e9344b101d2 Mon Sep 17 00:00:00 2001 From: Ketan Verma Date: Wed, 13 Dec 2023 14:02:08 +0530 Subject: [PATCH 08/11] Make JMH annotations consistent across sources Signed-off-by: Ketan Verma --- benchmarks/build.gradle | 2 + .../common/round/RoundableBenchmark.java | 18 +-- .../common/round/RoundableSupplier.java | 35 ++++ .../common/round/RoundableBenchmark.java | 150 ------------------ .../common/round/RoundableSupplier.java | 36 +++++ 5 files changed, 77 insertions(+), 164 deletions(-) create mode 100644 benchmarks/src/main/java/org/opensearch/common/round/RoundableSupplier.java delete mode 100644 benchmarks/src/main/java20/org/opensearch/common/round/RoundableBenchmark.java create mode 100644 benchmarks/src/main/java20/org/opensearch/common/round/RoundableSupplier.java diff --git a/benchmarks/build.gradle b/benchmarks/build.gradle index c1cc8438faa46..be4579b4e5324 100644 --- a/benchmarks/build.gradle +++ b/benchmarks/build.gradle @@ -88,6 +88,8 @@ spotless { if (BuildParams.runtimeJavaVersion >= JavaVersion.VERSION_20) { // Add support for incubator modules on supported Java versions. run.jvmArgs += ['--add-modules=jdk.incubator.vector'] + run.classpath += files(jar.archiveFile) + run.classpath -= sourceSets.main.output evaluationDependsOn(':libs:opensearch-common') sourceSets { diff --git a/benchmarks/src/main/java/org/opensearch/common/round/RoundableBenchmark.java b/benchmarks/src/main/java/org/opensearch/common/round/RoundableBenchmark.java index 4e07af452968b..3909a3f4eb8fc 100644 --- a/benchmarks/src/main/java/org/opensearch/common/round/RoundableBenchmark.java +++ b/benchmarks/src/main/java/org/opensearch/common/round/RoundableBenchmark.java @@ -21,7 +21,6 @@ import org.openjdk.jmh.infra.Blackhole; import java.util.Random; -import java.util.function.Supplier; @Fork(value = 3) @Warmup(iterations = 3, time = 1) @@ -83,17 +82,17 @@ public static class Options { "256" }) public Integer size; - @Param({ "binary", "linear" }) + @Param({ "binary", "linear", "btree" }) public String type; @Param({ "uniform", "skewed_edge", "skewed_center" }) public String distribution; public long[] queries; - public Supplier supplier; + public RoundableSupplier supplier; @Setup - public void setup() { + public void setup() throws ClassNotFoundException { Random random = new Random(size); long[] values = new long[size]; for (int i = 1; i < values.length; i++) { @@ -128,16 +127,7 @@ public void setup() { throw new IllegalArgumentException("invalid distribution: " + distribution); } - switch (type) { - case "binary": - supplier = () -> new BinarySearcher(values, size); - break; - case "linear": - supplier = () -> new BidirectionalLinearSearcher(values, size); - break; - default: - throw new IllegalArgumentException("invalid type: " + type); - } + supplier = new RoundableSupplier(type, values, size); } private static long nextPositiveLong(Random random) { diff --git a/benchmarks/src/main/java/org/opensearch/common/round/RoundableSupplier.java b/benchmarks/src/main/java/org/opensearch/common/round/RoundableSupplier.java new file mode 100644 index 0000000000000..44ac42810996f --- /dev/null +++ b/benchmarks/src/main/java/org/opensearch/common/round/RoundableSupplier.java @@ -0,0 +1,35 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.common.round; + +import java.util.function.Supplier; + +public class RoundableSupplier implements Supplier { + private final Supplier delegate; + + RoundableSupplier(String type, long[] values, int size) throws ClassNotFoundException { + switch (type) { + case "binary": + delegate = () -> new BinarySearcher(values, size); + break; + case "linear": + delegate = () -> new BidirectionalLinearSearcher(values, size); + break; + case "btree": + throw new ClassNotFoundException("BtreeSearcher is not supported below JDK 20"); + default: + throw new IllegalArgumentException("invalid type: " + type); + } + } + + @Override + public Roundable get() { + return delegate.get(); + } +} diff --git a/benchmarks/src/main/java20/org/opensearch/common/round/RoundableBenchmark.java b/benchmarks/src/main/java20/org/opensearch/common/round/RoundableBenchmark.java deleted file mode 100644 index bccbd758b3e29..0000000000000 --- a/benchmarks/src/main/java20/org/opensearch/common/round/RoundableBenchmark.java +++ /dev/null @@ -1,150 +0,0 @@ -/* - * SPDX-License-Identifier: Apache-2.0 - * - * The OpenSearch Contributors require contributions made to - * this file be licensed under the Apache-2.0 license or a - * compatible open source license. - */ - -package org.opensearch.common.round; - -import org.openjdk.jmh.annotations.Benchmark; -import org.openjdk.jmh.annotations.BenchmarkMode; -import org.openjdk.jmh.annotations.Fork; -import org.openjdk.jmh.annotations.Measurement; -import org.openjdk.jmh.annotations.Mode; -import org.openjdk.jmh.annotations.Param; -import org.openjdk.jmh.annotations.Scope; -import org.openjdk.jmh.annotations.Setup; -import org.openjdk.jmh.annotations.State; -import org.openjdk.jmh.annotations.Warmup; -import org.openjdk.jmh.infra.Blackhole; - -import java.util.Random; -import java.util.function.Supplier; - -@Fork(value = 3) -@Warmup(iterations = 3, time = 1) -@Measurement(iterations = 1, time = 1) -@BenchmarkMode(Mode.Throughput) -public class RoundableBenchmark { - - @Benchmark - public void floor(Blackhole bh, Options opts) { - Roundable roundable = opts.supplier.get(); - for (long key : opts.queries) { - bh.consume(roundable.floor(key)); - } - } - - @State(Scope.Benchmark) - public static class Options { - @Param({ - "1", - "2", - "3", - "4", - "5", - "6", - "7", - "8", - "9", - "10", - "12", - "14", - "16", - "18", - "20", - "22", - "24", - "26", - "29", - "32", - "37", - "41", - "45", - "49", - "54", - "60", - "64", - "74", - "83", - "90", - "98", - "108", - "118", - "128", - "144", - "159", - "171", - "187", - "204", - "229", - "256" }) - public Integer size; - - @Param({ "linear", "binary", "btree" }) - public String type; - - @Param({ "uniform", "skewed_edge", "skewed_center" }) - public String distribution; - - public long[] queries; - public Supplier supplier; - - @Setup - public void setup() throws ClassNotFoundException, IllegalAccessException { - Random random = new Random(size); - long[] values = new long[size]; - for (int i = 1; i < values.length; i++) { - values[i] = values[i - 1] + 100; - } - - long range = values[values.length - 1] - values[0] + 100; - long mean, stddev; - queries = new long[1000000]; - - switch (distribution) { - case "uniform": // all values equally likely. - for (int i = 0; i < queries.length; i++) { - queries[i] = values[0] + (nextPositiveLong(random) % range); - } - break; - case "skewed_edge": // distribution centered at p90 with ± 5% stddev. - mean = values[0] + (long) (range * 0.9); - stddev = (long) (range * 0.05); - for (int i = 0; i < queries.length; i++) { - queries[i] = Math.max(values[0], mean + (long) (random.nextGaussian() * stddev)); - } - break; - case "skewed_center": // distribution centered at p50 with ± 5% stddev. - mean = values[0] + (long) (range * 0.5); - stddev = (long) (range * 0.05); - for (int i = 0; i < queries.length; i++) { - queries[i] = Math.max(values[0], mean + (long) (random.nextGaussian() * stddev)); - } - break; - default: - throw new IllegalArgumentException("invalid distribution: " + distribution); - } - - switch (type) { - case "binary": - supplier = () -> new BinarySearcher(values, size); - break; - case "linear": - supplier = () -> new BidirectionalLinearSearcher(values, size); - break; - case "btree": - supplier = () -> new BtreeSearcher(values, size); - break; - default: - throw new IllegalArgumentException("invalid type: " + type); - } - } - - private static long nextPositiveLong(Random random) { - return random.nextLong() & Long.MAX_VALUE; - } - } -} diff --git a/benchmarks/src/main/java20/org/opensearch/common/round/RoundableSupplier.java b/benchmarks/src/main/java20/org/opensearch/common/round/RoundableSupplier.java new file mode 100644 index 0000000000000..e81c1b137bd30 --- /dev/null +++ b/benchmarks/src/main/java20/org/opensearch/common/round/RoundableSupplier.java @@ -0,0 +1,36 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.common.round; + +import java.util.function.Supplier; + +public class RoundableSupplier implements Supplier { + private final Supplier delegate; + + RoundableSupplier(String type, long[] values, int size) { + switch (type) { + case "binary": + delegate = () -> new BinarySearcher(values, size); + break; + case "linear": + delegate = () -> new BidirectionalLinearSearcher(values, size); + break; + case "btree": + delegate = () -> new BtreeSearcher(values, size); + break; + default: + throw new IllegalArgumentException("invalid type: " + type); + } + } + + @Override + public Roundable get() { + return delegate.get(); + } +} From 2e82d0aa7b114000bcf58f9485377eb5705ba3b1 Mon Sep 17 00:00:00 2001 From: Ketan Verma Date: Mon, 18 Dec 2023 16:13:21 +0530 Subject: [PATCH 09/11] Improve execution of Roundable unit-tests Signed-off-by: Ketan Verma --- libs/common/build.gradle | 8 --- .../common/round/RoundableTests.java | 68 +++++++++++++------ 2 files changed, 49 insertions(+), 27 deletions(-) diff --git a/libs/common/build.gradle b/libs/common/build.gradle index 5dd25fde56a0f..704fe1b3d63da 100644 --- a/libs/common/build.gradle +++ b/libs/common/build.gradle @@ -91,14 +91,6 @@ if (BuildParams.runtimeJavaVersion >= JavaVersion.VERSION_20) { classpath -= sourceSets.main.output } - tasks.register('roundableSimdTest', Test) { - group 'verification' - include '**/RoundableTests.class' - systemProperty 'opensearch.experimental.feature.simd.rounding.enabled', 'true' - } - - check.dependsOn(roundableSimdTest) - forbiddenApisJava20 { failOnMissingClasses = false ignoreSignaturesOfMissingClasses = true diff --git a/libs/common/src/test/java/org/opensearch/common/round/RoundableTests.java b/libs/common/src/test/java/org/opensearch/common/round/RoundableTests.java index 1c0a36fe28ee8..3873a599ad8c7 100644 --- a/libs/common/src/test/java/org/opensearch/common/round/RoundableTests.java +++ b/libs/common/src/test/java/org/opensearch/common/round/RoundableTests.java @@ -8,36 +8,61 @@ package org.opensearch.common.round; +import org.opensearch.common.SuppressForbidden; import org.opensearch.test.OpenSearchTestCase; -import java.util.List; +import java.lang.invoke.MethodHandles; +import java.lang.reflect.InvocationTargetException; public class RoundableTests extends OpenSearchTestCase { - public void testRoundingEmptyArray() { - Throwable throwable = assertThrows(IllegalArgumentException.class, () -> RoundableFactory.create(new long[0], 0)); - assertEquals("at least one value must be present", throwable.getMessage()); + public void testBidirectionalLinearSearcher() { + assertRounding(BidirectionalLinearSearcher::new); } - public void testRoundingSmallArray() { - int size = randomIntBetween(1, 64); - long[] values = randomArrayOfSortedValues(size); - Roundable roundable = RoundableFactory.create(values, size); - - assertEquals("BidirectionalLinearSearcher", roundable.getClass().getSimpleName()); - assertRounding(roundable, values, size); + public void testBinarySearcher() { + assertRounding(BinarySearcher::new); } - public void testRoundingLargeArray() { - int size = randomIntBetween(65, 256); - long[] values = randomArrayOfSortedValues(size); - Roundable roundable = RoundableFactory.create(values, size); + @SuppressForbidden(reason = "Reflective construction of BtreeSearcher since it's not supported below Java 20") + public void testBtreeSearcher() { + RoundableSupplier supplier; + + try { + Class clz = MethodHandles.lookup().findClass("org.opensearch.common.round.BtreeSearcher"); + supplier = (values, size) -> { + try { + return (Roundable) clz.getDeclaredConstructor(long[].class, int.class).newInstance(values, size); + } catch (InvocationTargetException e) { + // Failed to instantiate the class. Unwrap if the nested exception is already a runtime exception, + // say due to an IllegalArgumentException due to bad constructor arguments. + if (e.getCause() instanceof RuntimeException) { + throw (RuntimeException) e.getCause(); + } else { + throw new RuntimeException(e); + } + } catch (Exception e) { + throw new RuntimeException(e); + } + }; + } catch (ClassNotFoundException e) { + assumeTrue("BtreeSearcher is not supported below Java 20", false); + return; + } catch (IllegalAccessException e) { + throw new RuntimeException(e); + } - assertTrue(List.of("BtreeSearcher", "BinarySearcher").contains(roundable.getClass().getSimpleName())); - assertRounding(roundable, values, size); + assertRounding(supplier); } - private void assertRounding(Roundable roundable, long[] values, int size) { + private void assertRounding(RoundableSupplier supplier) { + Throwable throwable = assertThrows(IllegalArgumentException.class, () -> supplier.get(new long[0], 0)); + assertEquals("at least one value must be present", throwable.getMessage()); + + int size = randomIntBetween(1, 256); + long[] values = randomArrayOfSortedValues(size); + Roundable roundable = supplier.get(values, size); + for (int i = 0; i < 100000; i++) { // Index of the expected round-down point. int idx = randomIntBetween(0, size - 1); @@ -55,7 +80,7 @@ private void assertRounding(Roundable roundable, long[] values, int size) { assertEquals(expected, roundable.floor(key)); } - Throwable throwable = assertThrows(AssertionError.class, () -> roundable.floor(values[0] - 1)); + throwable = assertThrows(AssertionError.class, () -> roundable.floor(values[0] - 1)); assertEquals("key must be greater than or equal to " + values[0], throwable.getMessage()); } @@ -69,4 +94,9 @@ private static long[] randomArrayOfSortedValues(int size) { return values; } + + @FunctionalInterface + private interface RoundableSupplier { + Roundable get(long[] values, int size); + } } From ef85c47cc1fb54c1145ae82f634b3543477d279e Mon Sep 17 00:00:00 2001 From: Ketan Verma Date: Mon, 18 Dec 2023 20:25:23 +0530 Subject: [PATCH 10/11] Revert "Improve execution of Roundable unit-tests" This reverts commit 2e82d0aa7b114000bcf58f9485377eb5705ba3b1. Signed-off-by: Ketan Verma --- libs/common/build.gradle | 8 +++ .../common/round/RoundableTests.java | 68 ++++++------------- 2 files changed, 27 insertions(+), 49 deletions(-) diff --git a/libs/common/build.gradle b/libs/common/build.gradle index 704fe1b3d63da..5dd25fde56a0f 100644 --- a/libs/common/build.gradle +++ b/libs/common/build.gradle @@ -91,6 +91,14 @@ if (BuildParams.runtimeJavaVersion >= JavaVersion.VERSION_20) { classpath -= sourceSets.main.output } + tasks.register('roundableSimdTest', Test) { + group 'verification' + include '**/RoundableTests.class' + systemProperty 'opensearch.experimental.feature.simd.rounding.enabled', 'true' + } + + check.dependsOn(roundableSimdTest) + forbiddenApisJava20 { failOnMissingClasses = false ignoreSignaturesOfMissingClasses = true diff --git a/libs/common/src/test/java/org/opensearch/common/round/RoundableTests.java b/libs/common/src/test/java/org/opensearch/common/round/RoundableTests.java index 3873a599ad8c7..1c0a36fe28ee8 100644 --- a/libs/common/src/test/java/org/opensearch/common/round/RoundableTests.java +++ b/libs/common/src/test/java/org/opensearch/common/round/RoundableTests.java @@ -8,61 +8,36 @@ package org.opensearch.common.round; -import org.opensearch.common.SuppressForbidden; import org.opensearch.test.OpenSearchTestCase; -import java.lang.invoke.MethodHandles; -import java.lang.reflect.InvocationTargetException; +import java.util.List; public class RoundableTests extends OpenSearchTestCase { - public void testBidirectionalLinearSearcher() { - assertRounding(BidirectionalLinearSearcher::new); - } - - public void testBinarySearcher() { - assertRounding(BinarySearcher::new); + public void testRoundingEmptyArray() { + Throwable throwable = assertThrows(IllegalArgumentException.class, () -> RoundableFactory.create(new long[0], 0)); + assertEquals("at least one value must be present", throwable.getMessage()); } - @SuppressForbidden(reason = "Reflective construction of BtreeSearcher since it's not supported below Java 20") - public void testBtreeSearcher() { - RoundableSupplier supplier; - - try { - Class clz = MethodHandles.lookup().findClass("org.opensearch.common.round.BtreeSearcher"); - supplier = (values, size) -> { - try { - return (Roundable) clz.getDeclaredConstructor(long[].class, int.class).newInstance(values, size); - } catch (InvocationTargetException e) { - // Failed to instantiate the class. Unwrap if the nested exception is already a runtime exception, - // say due to an IllegalArgumentException due to bad constructor arguments. - if (e.getCause() instanceof RuntimeException) { - throw (RuntimeException) e.getCause(); - } else { - throw new RuntimeException(e); - } - } catch (Exception e) { - throw new RuntimeException(e); - } - }; - } catch (ClassNotFoundException e) { - assumeTrue("BtreeSearcher is not supported below Java 20", false); - return; - } catch (IllegalAccessException e) { - throw new RuntimeException(e); - } + public void testRoundingSmallArray() { + int size = randomIntBetween(1, 64); + long[] values = randomArrayOfSortedValues(size); + Roundable roundable = RoundableFactory.create(values, size); - assertRounding(supplier); + assertEquals("BidirectionalLinearSearcher", roundable.getClass().getSimpleName()); + assertRounding(roundable, values, size); } - private void assertRounding(RoundableSupplier supplier) { - Throwable throwable = assertThrows(IllegalArgumentException.class, () -> supplier.get(new long[0], 0)); - assertEquals("at least one value must be present", throwable.getMessage()); - - int size = randomIntBetween(1, 256); + public void testRoundingLargeArray() { + int size = randomIntBetween(65, 256); long[] values = randomArrayOfSortedValues(size); - Roundable roundable = supplier.get(values, size); + Roundable roundable = RoundableFactory.create(values, size); + + assertTrue(List.of("BtreeSearcher", "BinarySearcher").contains(roundable.getClass().getSimpleName())); + assertRounding(roundable, values, size); + } + private void assertRounding(Roundable roundable, long[] values, int size) { for (int i = 0; i < 100000; i++) { // Index of the expected round-down point. int idx = randomIntBetween(0, size - 1); @@ -80,7 +55,7 @@ private void assertRounding(RoundableSupplier supplier) { assertEquals(expected, roundable.floor(key)); } - throwable = assertThrows(AssertionError.class, () -> roundable.floor(values[0] - 1)); + Throwable throwable = assertThrows(AssertionError.class, () -> roundable.floor(values[0] - 1)); assertEquals("key must be greater than or equal to " + values[0], throwable.getMessage()); } @@ -94,9 +69,4 @@ private static long[] randomArrayOfSortedValues(int size) { return values; } - - @FunctionalInterface - private interface RoundableSupplier { - Roundable get(long[] values, int size); - } } From 42eac9a3cafa66ec30be9e60f6942b2b9882682f Mon Sep 17 00:00:00 2001 From: Ketan Verma Date: Mon, 18 Dec 2023 20:51:34 +0530 Subject: [PATCH 11/11] Add 'forced' as a possible feature flag value to simplify the execution of unit-tests Signed-off-by: Ketan Verma --- libs/common/build.gradle | 2 +- .../common/round/RoundableFactory.java | 20 ++++++++++++------- .../common/round/RoundableTests.java | 5 ++--- 3 files changed, 16 insertions(+), 11 deletions(-) diff --git a/libs/common/build.gradle b/libs/common/build.gradle index 5dd25fde56a0f..60bf488833393 100644 --- a/libs/common/build.gradle +++ b/libs/common/build.gradle @@ -94,7 +94,7 @@ if (BuildParams.runtimeJavaVersion >= JavaVersion.VERSION_20) { tasks.register('roundableSimdTest', Test) { group 'verification' include '**/RoundableTests.class' - systemProperty 'opensearch.experimental.feature.simd.rounding.enabled', 'true' + systemProperty 'opensearch.experimental.feature.simd.rounding.enabled', 'forced' } check.dependsOn(roundableSimdTest) diff --git a/libs/common/src/main/java20/org/opensearch/common/round/RoundableFactory.java b/libs/common/src/main/java20/org/opensearch/common/round/RoundableFactory.java index 4afc033408ca4..c5ec45bceb5bd 100644 --- a/libs/common/src/main/java20/org/opensearch/common/round/RoundableFactory.java +++ b/libs/common/src/main/java20/org/opensearch/common/round/RoundableFactory.java @@ -20,19 +20,25 @@ @InternalApi public final class RoundableFactory { /** - * The maximum limit up to which linear search is used, otherwise binary search is used. + * The maximum limit up to which linear search is used, otherwise binary or B-tree search is used. * This is because linear search is much faster on small arrays. * Benchmark results: PR #9727 */ private static final int LINEAR_SEARCH_MAX_SIZE = 64; /** - * Indicates whether the vectorized (SIMD) B-tree search implementation is supported. - * This is true when the platform has a minimum of 4 long vector lanes and the feature flag is enabled. - * Package-private for testing. + * Indicates whether the vectorized (SIMD) B-tree search implementation is to be used. + * It is true when either: + * 1. The feature flag is set to "forced", or + * 2. The platform has a minimum of 4 long vector lanes and the feature flag is set to "true". */ - static final boolean IS_BTREE_SEARCH_SUPPORTED = LongVector.SPECIES_PREFERRED.length() >= 4 - && "true".equalsIgnoreCase(System.getProperty("opensearch.experimental.feature.simd.rounding.enabled")); + private static final boolean USE_BTREE_SEARCHER; + + static { + String simdRoundingFeatureFlag = System.getProperty("opensearch.experimental.feature.simd.rounding.enabled"); + USE_BTREE_SEARCHER = "forced".equalsIgnoreCase(simdRoundingFeatureFlag) + || (LongVector.SPECIES_PREFERRED.length() >= 4 && "true".equalsIgnoreCase(simdRoundingFeatureFlag)); + } private RoundableFactory() {} @@ -42,7 +48,7 @@ private RoundableFactory() {} public static Roundable create(long[] values, int size) { if (size <= LINEAR_SEARCH_MAX_SIZE) { return new BidirectionalLinearSearcher(values, size); - } else if (IS_BTREE_SEARCH_SUPPORTED) { + } else if (USE_BTREE_SEARCHER) { return new BtreeSearcher(values, size); } else { return new BinarySearcher(values, size); diff --git a/libs/common/src/test/java/org/opensearch/common/round/RoundableTests.java b/libs/common/src/test/java/org/opensearch/common/round/RoundableTests.java index 1c0a36fe28ee8..ad19f456b0df4 100644 --- a/libs/common/src/test/java/org/opensearch/common/round/RoundableTests.java +++ b/libs/common/src/test/java/org/opensearch/common/round/RoundableTests.java @@ -10,8 +10,6 @@ import org.opensearch.test.OpenSearchTestCase; -import java.util.List; - public class RoundableTests extends OpenSearchTestCase { public void testRoundingEmptyArray() { @@ -33,7 +31,8 @@ public void testRoundingLargeArray() { long[] values = randomArrayOfSortedValues(size); Roundable roundable = RoundableFactory.create(values, size); - assertTrue(List.of("BtreeSearcher", "BinarySearcher").contains(roundable.getClass().getSimpleName())); + boolean useBtreeSearcher = "forced".equalsIgnoreCase(System.getProperty("opensearch.experimental.feature.simd.rounding.enabled")); + assertEquals(useBtreeSearcher ? "BtreeSearcher" : "BinarySearcher", roundable.getClass().getSimpleName()); assertRounding(roundable, values, size); }