Speed up time interval arounding around dst #56371

nik9000 · 2020-05-07T17:22:04Z

When an index spans a daylight savings time transition we can't use our
optimization that rewrites the requested time zone to a fixed time zone
and instead we used to fall back to a java.util.time based rounding
implementation. In #55559 we optimized "time unit" rounding. This
optimizes "time interval" rounding.

The java.util.time based implementation is about 1650% slower than the
rounding implementation for a fixed time zone. This replaces it with a
similar optimization that is only about 30% slower than the fixed time
zone. The java.util.time implementation allocates a ton of short lived
objects but the optimized implementation doesn't. So it might end up
being faster than the microbenchmarks imply.

When an index spans a daylight savings time transition we can't use our optimization that rewrites the requested time zone to a fixed time zone and instead we used to fall back to a java.util.time based rounding implementation. In elastic#55559 we optimized "time unit" rounding. This optimizes "time interval" rounding. The java.util.time based implementation is about 1650% slower than the rounding implementation for a fixed time zone. This replaces it with a similar optimization that is only about 30% slower than the fixed time zone. The java.util.time implementation allocates a ton of short lived objects but the optimized implementation doesn't. So it *might* end up being faster than the microbenchmarks imply.

elasticmachine · 2020-05-07T17:22:06Z

Pinging @elastic/es-analytics-geo (:Analytics/Aggregations)

nik9000 · 2020-05-07T17:22:30Z

server/src/main/java/org/elasticsearch/common/LocalTimeOffset.java

@@ -91,7 +91,7 @@ public static Lookup lookup(ZoneId zone, long minUtcMillis, long maxUtcMillis) {
     *
     * @return a lookup function of {@code null} if none could be built 
     */
-    public static LocalTimeOffset lookupFixedOffset(ZoneId zone) {
+    public static LocalTimeOffset fixedOffset(ZoneId zone) {


I renamed this method because the name seemed wrong to me when I read it this time around.

nik9000 · 2020-05-07T17:23:08Z

benchmarks/src/main/java/org/elasticsearch/common/RoundingBenchmark.java

-    @Param({ "MONTH_OF_YEAR", "HOUR_OF_DAY" })
-    public String timeUnit;
+    @Param({ "calendar year", "calendar hour", "10d", "5d", "1h" })
+    public String interval;


These add support for time interval rounding.

nik9000 · 2020-05-07T17:23:32Z

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

@@ -555,7 +555,7 @@ public long inGap(long localMillis, Gap gap) {
            @Override
            public long beforeGap(long localMillis, Gap gap) {
                return gap.previous().localToUtc(localMillis, this);
-            };
+            }


Removed silly ; because it made my eyes hurt.

nik9000 · 2020-05-07T17:23:55Z

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


        private final long interval;
        private final ZoneId timeZone;
-        /** For fixed offset timezones, this is the offset in milliseconds, otherwise TZ_OFFSET_NON_FIXED */
-        private final long fixedOffsetMillis;


We don't need this any more because we can just ask the lookup if it is fixed across the bounds we need.

nik9000 · 2020-05-07T17:24:19Z

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

@@ -773,88 +767,32 @@ public byte id() {

        @Override
        public Prepared prepare(long minUtcMillis, long maxUtcMillis) {
-            return prepareForUnknown();
+            long minLookup = minUtcMillis - interval;


And here we go with the actual change!

nik9000 · 2020-05-07T17:24:52Z

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

+
+            @Override
+            public long nextRoundingValue(long utcMillis) {
+                // TODO this is used in date range's collect so we should optimize it too


This is pretty much what we used to do. It ain't fast, but we can make it faster later.

nik9000 · 2020-05-07T17:25:08Z

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

+
+        private class JavaTimeRounding implements Prepared {
+            @Override
+            public long round(long utcMillis) {


This is just shuffled from where it used to be.

nik9000 · 2020-05-07T17:26:25Z

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

@@ -778,6 +803,18 @@ public void testPrepareLongRangeRoundsNotToMidnight() {
        assertThat(prepared.round(time("9000-03-31T15:25:15.148Z")), isDate(time("9000-03-31T15:00:00Z"), tz));
    }

+    public void testIntervalBeforeGap() {


These were fun examples that came up in the random tests that failed. It makes it a ton easier to rerun them if I pull them out like this.

Can you add a comment/javadoc to this, please? It's unclear to me why this particular data would be problematic

not-napoleon

Testing against java time lends a lot of confidence to this. I don't see any obvious issues with the implementation. Given both of those, I think this looks good.

not-napoleon · 2020-05-07T18:09:06Z

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

+            }
+        }
+
+        private class VariableRounding implements Prepared, LocalTimeOffset.Strategy {


I know this is private, but I still think a bit of javadoc to explain when you want Variable and when you want Fixed would help future generations.

not-napoleon · 2020-05-07T18:14:40Z

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

+            // Round a whole bunch of dates and make sure they line up with the known good java time implementation
+            Rounding.Prepared prepared = rounding.prepare(min, max);
+            Rounding.Prepared javaTimeRounding = rounding.prepareJavaTime();
+            for (int d = 0; d < 1000; d++) {


This is a good test. I like this test.

not-napoleon · 2020-05-07T18:17:59Z

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

@@ -778,6 +803,18 @@ public void testPrepareLongRangeRoundsNotToMidnight() {
        assertThat(prepared.round(time("9000-03-31T15:25:15.148Z")), isDate(time("9000-03-31T15:00:00Z"), tz));
    }

+    public void testIntervalBeforeGap() {


Can you add a comment/javadoc to this, please? It's unclear to me why this particular data would be problematic

nik9000 · 2020-05-07T20:08:51Z

I've pushed some more Javadoc!

We really are relying and the randomized testing against java.util.time to catch stuff. Partially that is because the rules that we use for rounding, while they fit on a page, aren't always the clearest thing to fit in your head.

When an index spans a daylight savings time transition we can't use our optimization that rewrites the requested time zone to a fixed time zone and instead we used to fall back to a java.util.time based rounding implementation. In elastic#55559 we optimized "time unit" rounding. This optimizes "time interval" rounding. The java.util.time based implementation is about 1650% slower than the rounding implementation for a fixed time zone. This replaces it with a similar optimization that is only about 30% slower than the fixed time zone. The java.util.time implementation allocates a ton of short lived objects but the optimized implementation doesn't. So it *might* end up being faster than the microbenchmarks imply.

nik9000 · 2020-05-08T12:01:57Z

Here is a completed benchmark run. The results are in line with what I mentioned in the issue description.

When an index spans a daylight savings time transition we can't use our optimization that rewrites the requested time zone to a fixed time zone and instead we used to fall back to a java.util.time based rounding implementation. In #55559 we optimized "time unit" rounding. This optimizes "time interval" rounding. The java.util.time based implementation is about 1650% slower than the rounding implementation for a fixed time zone. This replaces it with a similar optimization that is only about 30% slower than the fixed time zone. The java.util.time implementation allocates a ton of short lived objects but the optimized implementation doesn't. So it *might* end up being faster than the microbenchmarks imply.

The `date_histogram` aggregation had an optimization where it'd rewrite `time_zones` who's offset from UTC is fixed across the entire index. This rewrite is no longer needed after elastic#56371 because we can tell that a time zone is fixed lower down in the aggregation. So this removes it.

The `date_histogram` aggregation had an optimization where it'd rewrite `time_zones` who's offset from UTC is fixed across the entire index. This rewrite is no longer needed after #56371 because we can tell that a time zone is fixed lower down in the aggregation. So this removes it.

The `date_histogram` aggregation had an optimization where it'd rewrite `time_zones` who's offset from UTC is fixed across the entire index. This rewrite is no longer needed after elastic#56371 because we can tell that a time zone is fixed lower down in the aggregation. So this removes it.

The `date_histogram` aggregation had an optimization where it'd rewrite `time_zones` who's offset from UTC is fixed across the entire index. This rewrite is no longer needed after #56371 because we can tell that a time zone is fixed lower down in the aggregation. So this removes it.

nik9000 added >feature :Analytics/Aggregations Aggregations v8.0.0 v7.9.0 labels May 7, 2020

nik9000 requested a review from not-napoleon May 7, 2020 17:22

elasticmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label May 7, 2020

nik9000 commented May 7, 2020

View reviewed changes

nik9000 mentioned this pull request May 7, 2020

Daylight Savings Time Optimization Followups #56124

Closed

7 tasks

not-napoleon approved these changes May 7, 2020

View reviewed changes

nik9000 mentioned this pull request May 7, 2020

Speed up rounding in auto_date_histogram #56384

Merged

nik9000 added 2 commits May 7, 2020 15:53

Javadoc

8ed0022

Javadoc

b13110f

nik9000 merged commit 8478ee6 into elastic:master May 7, 2020

nik9000 added the backport pending label May 7, 2020

tvernum mentioned this pull request May 8, 2020

[CI] testRandomTimeIntervalRounding Failure #56400

Closed

nik9000 removed the backport pending label May 8, 2020

nik9000 mentioned this pull request Jun 8, 2020

Drop rewriting in date_histogram #57836

Merged

nik9000 mentioned this pull request Jul 1, 2020

Drop rewriting in date_histogram (backport of #57836) #58875

Merged

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Speed up time interval arounding around dst #56371

Speed up time interval arounding around dst #56371

Uh oh!

Conversation

nik9000 commented May 7, 2020

Uh oh!

elasticmachine commented May 7, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

not-napoleon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nik9000 commented May 7, 2020

Uh oh!

nik9000 commented May 8, 2020

Uh oh!

Uh oh!