[SPARK-31630][SQL] Fix perf regression by skipping timestamps rebasing after some threshold #28441

MaxGekk · 2020-05-03T12:31:37Z

What changes were proposed in this pull request?

Skip timestamps rebasing after a global threshold when there is no difference between Julian and Gregorian calendars. This allows to avoid checking hash maps of switch points, and fixes perf regressions in toJavaTimestamp() and fromJavaTimestamp().

Why are the changes needed?

The changes fix perf regressions of conversions to/from external type java.sql.Timestamp.

Before (see the PR's results #28440):

================================================================================================
Conversion from/to external types
================================================================================================

OpenJDK 64-Bit Server VM 1.8.0_252-8u252-b09-1~18.04-b09 on Linux 4.15.0-1063-aws
Intel(R) Xeon(R) CPU E5-2670 v2 @ 2.50GHz
To/from Java's date-time:                 Best Time(ms)   Avg Time(ms)   Stdev(ms)    Rate(M/s)   Per Row(ns)   Relative
------------------------------------------------------------------------------------------------------------------------
From java.sql.Timestamp                             376            388          10         13.3          75.2       1.1X
Collect java.sql.Timestamp                         1878           1937          64          2.7         375.6       0.2X

After:

================================================================================================
Conversion from/to external types
================================================================================================

OpenJDK 64-Bit Server VM 1.8.0_252-8u252-b09-1~18.04-b09 on Linux 4.15.0-1063-aws
Intel(R) Xeon(R) CPU E5-2670 v2 @ 2.50GHz
To/from Java's date-time:                 Best Time(ms)   Avg Time(ms)   Stdev(ms)    Rate(M/s)   Per Row(ns)   Relative
------------------------------------------------------------------------------------------------------------------------
From java.sql.Timestamp                             249            264          24         20.1          49.8       1.7X
Collect java.sql.Timestamp                         1503           1523          24          3.3         300.5       0.3X

Perf improvements in average of:

From java.sql.Timestamp is ~ 34%
To java.sql.Timestamps is ~16%

Does this PR introduce any user-facing change?

No

How was this patch tested?

By existing test suites DateTimeUtilsSuite and RebaseDateTimeSuite.

MaxGekk · 2020-05-03T12:32:00Z

@cloud-fan @HyukjinKwon Please, review this PR.

SparkQA · 2020-05-03T16:52:55Z

Test build #122230 has finished for PR 28441 at commit f4187d2.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

…hreshold

…common-threshold # Conflicts: # sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/RebaseDateTime.scala # sql/core/benchmarks/DateTimeRebaseBenchmark-jdk11-results.txt # sql/core/benchmarks/DateTimeRebaseBenchmark-results.txt

SparkQA · 2020-05-04T13:37:10Z

Test build #122259 has finished for PR 28441 at commit ded04e2.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-05-04T13:39:28Z

Test build #122258 has finished for PR 28441 at commit b9dfb16.

This patch passes all tests.
This patch does not merge cleanly.
This patch adds no public classes.

SparkQA · 2020-05-04T15:12:20Z

Test build #122262 has finished for PR 28441 at commit d31ec0f.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2020-05-05T06:08:59Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/RebaseDateTime.scala


+  private def getLastSwitchTs(rebaseMap: AnyRefMap[String, RebaseInfo]): Long = {
+    val latestTs = rebaseMap.values.map(_.switches.last).max
+    require(rebaseMap.values.forall(_.diffs.last == 0),


ideally require should be the first line in a method.

It uses latestTs in the error message. I am going to improve the message by converting micros to Instant, so, toString should form nicer string:

require(rebaseMap.values.forall(_.diffs.last == 0), s"Differences between Julian and Gregorian calendar after ${microsToInstant(latestTs)} " + "are expected to be zero for available time zones.")

cloud-fan · 2020-05-05T06:10:00Z

LGTM, let's regenerate the benchmark result to fix conflicts.

…common-threshold # Conflicts: # sql/core/benchmarks/DateTimeRebaseBenchmark-jdk11-results.txt # sql/core/benchmarks/DateTimeRebaseBenchmark-results.txt

SparkQA · 2020-05-05T07:05:02Z

Test build #122306 has finished for PR 28441 at commit eaca6a8.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2020-05-05T09:15:46Z

retest this please

SparkQA · 2020-05-05T13:48:31Z

Test build #122313 has finished for PR 28441 at commit eaca6a8.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-05-05T14:00:18Z

Test build #122314 has finished for PR 28441 at commit 2badf88.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2020-05-05T14:11:45Z

thanks, merging to master/3.0!

…g after some threshold ### What changes were proposed in this pull request? Skip timestamps rebasing after a global threshold when there is no difference between Julian and Gregorian calendars. This allows to avoid checking hash maps of switch points, and fixes perf regressions in `toJavaTimestamp()` and `fromJavaTimestamp()`. ### Why are the changes needed? The changes fix perf regressions of conversions to/from external type `java.sql.Timestamp`. Before (see the PR's results #28440): ``` ================================================================================================ Conversion from/to external types ================================================================================================ OpenJDK 64-Bit Server VM 1.8.0_252-8u252-b09-1~18.04-b09 on Linux 4.15.0-1063-aws Intel(R) Xeon(R) CPU E5-2670 v2 2.50GHz To/from Java's date-time: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative ------------------------------------------------------------------------------------------------------------------------ From java.sql.Timestamp 376 388 10 13.3 75.2 1.1X Collect java.sql.Timestamp 1878 1937 64 2.7 375.6 0.2X ``` After: ``` ================================================================================================ Conversion from/to external types ================================================================================================ OpenJDK 64-Bit Server VM 1.8.0_252-8u252-b09-1~18.04-b09 on Linux 4.15.0-1063-aws Intel(R) Xeon(R) CPU E5-2670 v2 2.50GHz To/from Java's date-time: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative ------------------------------------------------------------------------------------------------------------------------ From java.sql.Timestamp 249 264 24 20.1 49.8 1.7X Collect java.sql.Timestamp 1503 1523 24 3.3 300.5 0.3X ``` Perf improvements in average of: 1. From java.sql.Timestamp is ~ 34% 2. To java.sql.Timestamps is ~16% ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? By existing test suites `DateTimeUtilsSuite` and `RebaseDateTimeSuite`. Closes #28441 from MaxGekk/opt-rebase-common-threshold. Authored-by: Max Gekk <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit bef5828) Signed-off-by: Wenchen Fan <[email protected]>

MaxGekk added 3 commits May 2, 2020 22:47

Optimize micros rebasing

6c536d4

Re-gen benchmarks on JDK 8

832f366

Re-gen results on JDK 11

f4187d2

probot-autolabeler bot added the SQL label May 3, 2020

MaxGekk added 7 commits May 4, 2020 06:05

Merge remote-tracking branch 'origin/master' into opt-rebase-common-t…

0d00a2b

…hreshold

Re-gen on JDK 8

f8ab34b

Re-gen on JDK 11

b9dfb16

Make lastSwitchJulianTs public

ded04e2

Re-gen DateTimeRebaseBenchmark-jdk11-results.txt

0c44097

Re-gen DateTimeRebaseBenchmark-results.txt

d31ec0f

cloud-fan reviewed May 5, 2020

View reviewed changes

MaxGekk added 2 commits May 5, 2020 09:16

Merge remote-tracking branch 'remotes/origin/master' into opt-rebase-…

dbdc926

…common-threshold # Conflicts: # sql/core/benchmarks/DateTimeRebaseBenchmark-jdk11-results.txt # sql/core/benchmarks/DateTimeRebaseBenchmark-results.txt

Improve error message in getLastSwitchTs()

eaca6a8

Re-gen DateTimeRebaseBenchmark-jdk11-results.txt

7ef27f8

Re-gen DateTimeRebaseBenchmark-results.txt

2badf88

cloud-fan closed this in bef5828 May 5, 2020

MaxGekk deleted the opt-rebase-common-threshold branch June 5, 2020 19:48

[SPARK-31630][SQL] Fix perf regression by skipping timestamps rebasing after some threshold #28441

[SPARK-31630][SQL] Fix perf regression by skipping timestamps rebasing after some threshold #28441

Uh oh!

Conversation

MaxGekk commented May 3, 2020

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

MaxGekk commented May 3, 2020

Uh oh!

SparkQA commented May 3, 2020

Uh oh!

SparkQA commented May 4, 2020

Uh oh!

SparkQA commented May 4, 2020

Uh oh!

SparkQA commented May 4, 2020

Uh oh!

cloud-fan May 5, 2020

Choose a reason for hiding this comment

Uh oh!

MaxGekk May 5, 2020

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented May 5, 2020

Uh oh!

SparkQA commented May 5, 2020

Uh oh!

cloud-fan commented May 5, 2020

Uh oh!

SparkQA commented May 5, 2020

Uh oh!

SparkQA commented May 5, 2020

Uh oh!

cloud-fan commented May 5, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants