Implement rounding optimization for fixed offset timezones #46670

csoulios · 2019-09-12T13:55:38Z

Fixes #45702 with date_histogram aggregation when using fixed_interval.

Implement code change discussed at:
#45702 (comment)

Optimization has been implemented for both fixed and calendar intervals

spinscale · 2019-09-12T13:57:21Z

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


        TimeUnitRounding(DateTimeUnit unit, ZoneId timeZone) {
            this.unit = unit;
            this.timeZone = timeZone;
            this.unitRoundsToMidnight = this.unit.field.getBaseUnit().getDuration().toMillis() > 3600000L;
-            this.isUtcTimeZone = timeZone.normalized().equals(ZoneOffset.UTC);


I remember adding this because of performance problems with timeZone.getRules().getOffset(Instant.EPOCH) in the UTC case. did you benchmark that this is not the case?

Why would the performance differ ? UTC is a fixed timezone so this change just englobes more cases, we're just changing the way we check if we can apply a fixed offset ?

I misunderstood your comment sorry, there is a TODO in the java code saying that getRules should be optimized so +1 to resolve the offset once in the ctr and use it to determine whether the fast rounding should be used.

the previous implementation did not call timeZone.getRules().getOffset(Instant.EPOCH).getTotalSeconds() in the UTC case

+1 to resolve the fixeed offset here and call timeZone.getRules().getOffset(Instant.EPOCH) only once.

yep agreed, this should be changed

spinscale · 2019-09-12T13:58:29Z

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

+            // This works as long as the tz offset doesn't change. It is worth getting this case out of the way first,
+            // as the calculations for fixing things near to offset changes are a little expensive and unnecessary
+            // in the common case of working with fixed offset timezones (such as UTC).
+            if (timeZone.getRules().isFixedOffset() == true) {


see above. can you benchmark this against the UTC case and see if the performance differs?

These are the results of the esrally benchmark used for reproducing the bug.

Dataset is the timestamps-gaussian-sameday
Query is:

"aggs": { "2": { "date_histogram": { "field": "@timestamp", "fixed_interval": "24h", "min_doc_count": 1 } } }, "query": { "match_all": {} }

baseline: master branch (8.x)
contender: branch with fix

| Metric | Task | Baseline | Contender | Diff | Unit | |-------------------------------+--------------------------+-------------+-------------+----------+--------| | Median Throughput | query-agg-date_histogram | 0.0896863 | 0.202938 | 0.11325 | ops/s | | Max Throughput | query-agg-date_histogram | 0.0903743 | 0.207398 | 0.11702 | ops/s | | 50th percentile latency | query-agg-date_histogram | 161809 | 3268.02 | -158541 | ms | | 90th percentile latency | query-agg-date_histogram | 230431 | 3435.84 | -226995 | ms | | 100th percentile latency | query-agg-date_histogram | 247591 | 3502.15 | -244089 | ms | | 50th percentile service time | query-agg-date_histogram | 10808.3 | 3263.49 | -7544.81 | ms | | 90th percentile service time | query-agg-date_histogram | 11114.6 | 3431.34 | -7683.28 | ms | | 100th percentile service time | query-agg-date_histogram | 11688.9 | 3499.13 | -8189.74 | ms |

I am going to work on the micro benchmarks

can you also compare the patch with the check for the UTC timezone in the ctor and without?

This is really a regression and we're just copying what we used to do with Joda. Let's setup a quick micro benchmarks for this if you want but the most important part here is correctness ;).

same here, we should resolve the fixed offset once in the ctr

Moved this to constructor as discussed. Benchmark results improved by ~12%.

See results below (contender is the newer revision):

|-------------------------------+--------------------------+-------------+-------------+----------+--------| | Metric | Task | Baseline | Contender | Diff | Unit | |-------------------------------+--------------------------+-------------+-------------+----------+--------| | 50th percentile latency | query-agg-date_histogram | 2892.92 | 2539.31 | -353.609 | ms | | 90th percentile latency | query-agg-date_histogram | 3086.98 | 2762.28 | -324.694 | ms | | 100th percentile latency | query-agg-date_histogram | 3182.51 | 2865.38 | -317.137 | ms | | 50th percentile service time | query-agg-date_histogram | 2885.19 | 2532.11 | -353.078 | ms | | 90th percentile service time | query-agg-date_histogram | 3078.69 | 2755.39 | -323.297 | ms | | 100th percentile service time | query-agg-date_histogram | 3174.46 | 2857.74 | -316.713 | ms | |-------------------------------+--------------------------+-------------+-------------+----------+--------|

elasticmachine · 2019-09-12T13:59:10Z

Pinging @elastic/es-analytics-geo

spinscale

As I am unsure about the performance implications, maybe resurrecting the RoundingBenchmark for testing from the 7.x branch makes sense here (or just test on the 7.x branch then port)

jimczi

Thanks @csoulios , the changes look good, I left some comments

jimczi · 2019-09-12T16:04:11Z

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


        TimeUnitRounding(DateTimeUnit unit, ZoneId timeZone) {
            this.unit = unit;
            this.timeZone = timeZone;
            this.unitRoundsToMidnight = this.unit.field.getBaseUnit().getDuration().toMillis() > 3600000L;
-            this.isUtcTimeZone = timeZone.normalized().equals(ZoneOffset.UTC);


+1 to resolve the fixeed offset here and call timeZone.getRules().getOffset(Instant.EPOCH) only once.

jimczi · 2019-09-12T16:05:24Z

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


        TimeUnitRounding(DateTimeUnit unit, ZoneId timeZone) {
            this.unit = unit;
            this.timeZone = timeZone;
            this.unitRoundsToMidnight = this.unit.field.getBaseUnit().getDuration().toMillis() > 3600000L;
-            this.isUtcTimeZone = timeZone.normalized().equals(ZoneOffset.UTC);


yep agreed, this should be changed

jimczi · 2019-09-12T16:05:40Z

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

+            // This works as long as the tz offset doesn't change. It is worth getting this case out of the way first,
+            // as the calculations for fixing things near to offset changes are a little expensive and unnecessary
+            // in the common case of working with fixed offset timezones (such as UTC).
+            if (timeZone.getRules().isFixedOffset() == true) {


same here, we should resolve the fixed offset once in the ctr

csoulios · 2019-09-13T12:53:39Z

I ran ES Rally benchmark (https://github.com/csoulios/date_histogram-benchmark) for the following scenarios (on my laptop):

ES 6.8.0, UTC tz
ES 8.x (with fix), UTC tz
ES 8.x (with fix), -01:00 tz
ES 8.x (with fix), Europe/Berlin tz

All produced similar results:

|-------------------------------+--------------------------+-------------+-----------+--------------+---------------------|
| Metric                        | Task                     | 6.8.0 (UTC) | 8.x (UTC) | 8.x (-01:00) | 8.x (Europe/Berlin) |
|-------------------------------+--------------------------+-------------+-----------+--------------+---------------------|
|      50th percentile latency  | query-agg-date_histogram |     2570.75 |   2539.31 |      2687.07 |             2583.18 |  
|      90th percentile latency  | query-agg-date_histogram |     2666.16 |   2762.28 |      2764.50 |             2674.80 |
|     100th percentile latency  | query-agg-date_histogram |     2780.84 |   2865.38 |      2786.68 |             2898.41 | 
|-------------------------------+--------------------------+-------------+-----------+--------------+---------------------|

jimczi

LGTM, thanks @csoulios

spinscale

left a few minor nits, feel free to ignore them

LGTM otherwise, thanks for fixing!

spinscale · 2019-09-16T07:02:22Z

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


        private final DateTimeUnit unit;
        private final ZoneId timeZone;
        private final boolean unitRoundsToMidnight;
-        private final boolean isUtcTimeZone;
+        /** For fixed offset timezones, this is the offset in milliseconds, otherwise TZ_OFFSET_NON_FIXED */
+        private final long fixedOffset;


ignoreable nit: maybe name this fixedOffsetMs or something to indicate its contents

spinscale · 2019-09-16T07:03:19Z

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

@@ -218,17 +218,20 @@ public Rounding build() {
    static class TimeUnitRounding extends Rounding {

        static final byte ID = 1;
+        static final long TZ_OFFSET_NON_FIXED = -1;


comment nit: maybe include a small comment what -1 indicates, and why this can never be -1 in a regular offset

spinscale · 2019-09-16T07:03:36Z

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

@@ -432,20 +436,25 @@ public String toString() {
        }

        static final byte ID = 2;
+        static final long TZ_OFFSET_NON_FIXED = -1;


minor nit: would one instance of this be enough?

spinscale · 2019-09-16T07:03:58Z

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

@@ -218,17 +218,20 @@ public Rounding build() {
    static class TimeUnitRounding extends Rounding {

        static final byte ID = 1;
+        static final long TZ_OFFSET_NON_FIXED = -1;


minor nit: can be private?

matriv · 2019-09-16T16:20:22Z

@elasticmachine run elasticsearch-ci/bwc
@elasticmachine run elasticsearch-ci/default-distro

…6670) Fixes elastic#45702 with date_histogram aggregation when using fixed_interval. Optimization has been implemented for both fixed and calendar intervals

Implement rounding optimization for fixed offset timezones. Fixes ela…

96e66a7

…stic#45702

csoulios requested review from spinscale and jimczi September 12, 2019 13:55

spinscale reviewed Sep 12, 2019

View reviewed changes

csoulios added :Analytics/Aggregations Aggregations v8.0.0 labels Sep 12, 2019

csoulios added the >regression label Sep 12, 2019

spinscale suggested changes Sep 12, 2019

View reviewed changes

jimczi requested changes Sep 12, 2019

View reviewed changes

Set TZ fixed offset in the class constructor to improve performance

08dca7d

csoulios changed the title ~~Implement rounding optimization for fixed offset timezones.~~ Implement rounding optimization for fixed offset timezones Sep 13, 2019

csoulios requested review from spinscale and jimczi September 13, 2019 12:00

csoulios added >regression and removed >regression labels Sep 13, 2019

jimczi approved these changes Sep 13, 2019

View reviewed changes

csoulios added v7.5.0 v7.4.1 backport pending labels Sep 13, 2019

spinscale approved these changes Sep 16, 2019

View reviewed changes

Minor changes to address review comments

db54962

Merge branch 'master' into fixed_interval_rounding

9d2341e

csoulios merged commit f7a34c8 into elastic:master Sep 16, 2019

csoulios deleted the fixed_interval_rounding branch September 16, 2019 20:17

colings86 added v7.4.0 and removed v7.4.1 labels Sep 17, 2019

colings86 added v7.4.1 and removed v7.4.0 labels Sep 17, 2019

DaveCTurner mentioned this pull request Sep 17, 2019

TimeIntervalRounding is not optimised for fixed-offset timezones #46771

Closed

This was referenced Sep 18, 2019

[7.4] Implement rounding optimization for fixed offset timezones #46807

Merged

[7.x] Implement rounding optimization for fixed offset timezones (#46670) #46809

Merged

csoulios removed the backport pending label Sep 18, 2019

colings86 added v7.4.0 and removed v7.4.1 labels Sep 20, 2019

jimczi added v7.5.0 and removed v7.5.0 labels Oct 16, 2019

$@polyfractal$ polyfractal mentioned this pull request Dec 12, 2019

date_histogram with fixed_interval suffer from DST changes #50139

Open

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement rounding optimization for fixed offset timezones #46670

Implement rounding optimization for fixed offset timezones #46670

csoulios commented Sep 12, 2019

spinscale Sep 12, 2019

jimczi Sep 12, 2019

jimczi Sep 12, 2019

spinscale Sep 12, 2019

jimczi Sep 12, 2019

jimczi Sep 12, 2019

spinscale Sep 12, 2019

csoulios Sep 12, 2019 •

edited

Loading

spinscale Sep 12, 2019

jimczi Sep 12, 2019

jimczi Sep 12, 2019

csoulios Sep 13, 2019 •

edited

Loading

elasticmachine commented Sep 12, 2019

spinscale left a comment

jimczi left a comment

jimczi Sep 12, 2019

jimczi Sep 12, 2019

jimczi Sep 12, 2019

csoulios commented Sep 13, 2019 •

edited

Loading

jimczi left a comment

spinscale left a comment

spinscale Sep 16, 2019

spinscale Sep 16, 2019

spinscale Sep 16, 2019

spinscale Sep 16, 2019

matriv commented Sep 16, 2019

Implement rounding optimization for fixed offset timezones #46670

Implement rounding optimization for fixed offset timezones #46670

Conversation

csoulios commented Sep 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csoulios Sep 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csoulios Sep 13, 2019 • edited Loading

Choose a reason for hiding this comment

elasticmachine commented Sep 12, 2019

spinscale left a comment

Choose a reason for hiding this comment

jimczi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csoulios commented Sep 13, 2019 • edited Loading

jimczi left a comment

Choose a reason for hiding this comment

spinscale left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matriv commented Sep 16, 2019

csoulios Sep 12, 2019 •

edited

Loading

csoulios Sep 13, 2019 •

edited

Loading

csoulios commented Sep 13, 2019 •

edited

Loading