Revert "Improve performance of rounding dates in date_histogram aggre…

…gation (#9727) (#9930)"

This reverts commit 6f1311b.

CHANGELOG.md

@@ -115,7 +115,6 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 - Use non-concurrent path for sort request on timeseries index and field([#9562](https://github.com/opensearch-project/OpenSearch/pull/9562))
 - Added sampler based on `Blanket Probabilistic Sampling rate` and `Override for on demand` ([#9621](https://github.com/opensearch-project/OpenSearch/issues/9621))
 - Decouple replication lag from logic to fail stale replicas ([#9507](https://github.com/opensearch-project/OpenSearch/pull/9507))
-- Improve performance of rounding dates in date_histogram aggregation ([#9727](https://github.com/opensearch-project/OpenSearch/pull/9727))
 - [Remote Store] Add support for Remote Translog Store stats in `_remotestore/stats/` API ([#9263](https://github.com/opensearch-project/OpenSearch/pull/9263))
 - Removing the vec file extension from INDEX_STORE_HYBRID_NIO_EXTENSIONS, to ensure the no performance degradation for vector search via Lucene Engine.([#9528](https://github.com/opensearch-project/OpenSearch/pull/9528)))
 - Cleanup Unreferenced file on segment merge failure ([#9503](https://github.com/opensearch-project/OpenSearch/pull/9503))

server/src/main/java/org/opensearch/common/Rounding.java

@@ -38,7 +38,6 @@
 import org.opensearch.OpenSearchException;
 import org.opensearch.common.LocalTimeOffset.Gap;
 import org.opensearch.common.LocalTimeOffset.Overlap;
-import org.opensearch.common.annotation.InternalApi;
 import org.opensearch.common.time.DateUtils;
 import org.opensearch.common.unit.TimeValue;
 import org.opensearch.core.common.io.stream.StreamInput;
@@ -414,21 +413,6 @@ public Rounding build() {
     }
 
     private abstract class PreparedRounding implements Prepared {
-        /**
-         * The maximum limit up to which array-based prepared rounding is used.
-         * 128 is a power of two that isn't huge. We might be able to do
-         * better if the limit was based on the actual type of prepared
-         * rounding but this'll do for now.
-         */
-        private static final int DEFAULT_ARRAY_ROUNDING_MAX_THRESHOLD = 128;
-
-        /**
-         * 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: <a href="https://github.com/opensearch-project/OpenSearch/pull/9727">PR #9727</a>
-         */
-        private static final int LINEAR_SEARCH_ARRAY_ROUNDING_MAX_THRESHOLD = 64;
-
         /**
          * Attempt to build a {@link Prepared} implementation that relies on pre-calcuated
          * "round down" points. If there would be more than {@code max} points then return
@@ -452,9 +436,7 @@ protected Prepared maybeUseArray(long minUtcMillis, long maxUtcMillis, int max)
                 values = ArrayUtil.grow(values, i + 1);
                 values[i++] = rounded;
             }
-            return i <= LINEAR_SEARCH_ARRAY_ROUNDING_MAX_THRESHOLD
-                ? new BidirectionalLinearSearchArrayRounding(values, i, this)
-                : new BinarySearchArrayRounding(values, i, this);
+            return new ArrayRounding(values, i, this);
         }
     }
 
@@ -547,11 +529,12 @@ private LocalDateTime truncateLocalDateTime(LocalDateTime localDateTime) {
 
         @Override
         public Prepared prepare(long minUtcMillis, long maxUtcMillis) {
-            return prepareOffsetOrJavaTimeRounding(minUtcMillis, maxUtcMillis).maybeUseArray(
-                minUtcMillis,
-                maxUtcMillis,
-                PreparedRounding.DEFAULT_ARRAY_ROUNDING_MAX_THRESHOLD
-            );
+            /*
+             * 128 is a power of two that isn't huge. We might be able to do
+             * better if the limit was based on the actual type of prepared
+             * rounding but this'll do for now.
+             */
+            return prepareOffsetOrJavaTimeRounding(minUtcMillis, maxUtcMillis).maybeUseArray(minUtcMillis, maxUtcMillis, 128);
         }
 
         private TimeUnitPreparedRounding prepareOffsetOrJavaTimeRounding(long minUtcMillis, long maxUtcMillis) {
@@ -1362,19 +1345,14 @@ public static Rounding read(StreamInput in) throws IOException {
     /**
      * Implementation of {@link Prepared} using pre-calculated "round down" points.
      *
-     * <p>
-     * It uses binary search to find the greatest round-down point less than or equal to the given timestamp.
-     *
      * @opensearch.internal
      */
-    @InternalApi
-    static class BinarySearchArrayRounding implements Prepared {
+    private static class ArrayRounding implements Prepared {
         private final long[] values;
         private final int max;
         private final Prepared delegate;
 
-        BinarySearchArrayRounding(long[] values, int max, Prepared delegate) {
-            assert max > 0 : "at least one round-down point must be present";
+        private ArrayRounding(long[] values, int max, Prepared delegate) {
             this.values = values;
             this.max = max;
             this.delegate = delegate;
@@ -1402,64 +1380,4 @@ public double roundingSize(long utcMillis, DateTimeUnit timeUnit) {
             return delegate.roundingSize(utcMillis, timeUnit);
         }
     }
-
-    /**
-     * Implementation of {@link Prepared} using pre-calculated "round down" points.
-     *
-     * <p>
-     * It uses linear search to find the greatest round-down point less than or equal to the given timestamp.
-     * For small inputs (&le; 64 elements), this can be much faster than binary search as it avoids the penalty of
-     * branch mispredictions and pipeline stalls, and accesses memory sequentially.
-     *
-     * <p>
-     * It uses "meet in the middle" linear search to avoid the worst case scenario when the desired element is present
-     * at either side of the array. This is helpful for time-series data where velocity increases over time, so more
-     * documents are likely to find a greater timestamp which is likely to be present on the right end of the array.
-     *
-     * @opensearch.internal
-     */
-    @InternalApi
-    static class BidirectionalLinearSearchArrayRounding implements Prepared {
-        private final long[] ascending;
-        private final long[] descending;
-        private final Prepared delegate;
-
-        BidirectionalLinearSearchArrayRounding(long[] values, int max, Prepared delegate) {
-            assert max > 0 : "at least one round-down point must be present";
-            this.delegate = delegate;
-            int len = (max + 1) >>> 1; // rounded-up to handle odd number of values
-            ascending = new long[len];
-            descending = new long[len];
-
-            for (int i = 0; i < len; i++) {
-                ascending[i] = values[i];
-                descending[i] = values[max - i - 1];
-            }
-        }
-
-        @Override
-        public long round(long utcMillis) {
-            int i = 0;
-            for (; i < ascending.length; i++) {
-                if (descending[i] <= utcMillis) {
-                    return descending[i];
-                }
-                if (ascending[i] > utcMillis) {
-                    assert i > 0 : "utcMillis must be after " + ascending[0];
-                    return ascending[i - 1];
-                }
-            }
-            return ascending[i - 1];
-        }
-
-        @Override
-        public long nextRoundingValue(long utcMillis) {
-            return delegate.nextRoundingValue(utcMillis);
-        }
-
-        @Override
-        public double roundingSize(long utcMillis, DateTimeUnit timeUnit) {
-            return delegate.roundingSize(utcMillis, timeUnit);
-        }
-    }
 }

server/src/test/java/org/opensearch/common/RoundingTests.java

@@ -1143,28 +1143,6 @@ public void testNonMillisecondsBasedUnitCalendarRoundingSize() {
         assertThat(prepared.roundingSize(thirdQuarter, Rounding.DateTimeUnit.HOUR_OF_DAY), closeTo(2208.0, 0.000001));
     }
 
-    public void testArrayRoundingImplementations() {
-        int length = randomIntBetween(1, 256);
-        long[] values = new long[length];
-        for (int i = 1; i < values.length; i++) {
-            values[i] = values[i - 1] + (randomNonNegativeLong() % 100);
-        }
-
-        Rounding.Prepared binarySearchImpl = new Rounding.BinarySearchArrayRounding(values, length, null);
-        Rounding.Prepared linearSearchImpl = new Rounding.BidirectionalLinearSearchArrayRounding(values, length, null);
-
-        for (int i = 0; i < 100000; i++) {
-            long key = values[0] + (randomNonNegativeLong() % (100 + values[length - 1] - values[0]));
-            assertEquals(binarySearchImpl.round(key), linearSearchImpl.round(key));
-        }
-
-        AssertionError exception = expectThrows(AssertionError.class, () -> { binarySearchImpl.round(values[0] - 1); });
-        assertEquals("utcMillis must be after " + values[0], exception.getMessage());
-
-        exception = expectThrows(AssertionError.class, () -> { linearSearchImpl.round(values[0] - 1); });
-        assertEquals("utcMillis must be after " + values[0], exception.getMessage());
-    }
-
     private void assertInterval(long rounded, long nextRoundingValue, Rounding rounding, int minutes, ZoneId tz) {
         assertInterval(rounded, dateBetween(rounded, nextRoundingValue), nextRoundingValue, rounding, tz);
         long millisPerMinute = 60_000;