[SPARK-27684][SQL] Avoid conversion overhead for primitive types by mgaido91 · Pull Request #24636 · apache/spark

mgaido91 · 2019-05-18T13:59:23Z

What changes were proposed in this pull request?

As outlined in the JIRA by @JoshRosen, our conversion mechanism from catalyst types to scala ones is pretty inefficient for primitive data types. Indeed, in these cases, most of the times we are adding useless calls to identity function or anyway to functions which return the same value. Using the information we have when we generate the code, we can avoid most of these overheads.

How was this patch tested?

Here is a simple test which shows the benefit that this PR can bring:

test("SPARK-27684: perf evaluation") {
val intLongUdf = ScalaUDF(
(a: Int, b: Long) => a + b, LongType,
Literal(1) :: Literal(1L) :: Nil,
true :: true :: Nil,
nullable = false)
val plan = generateProject(
MutableProjection.create(Alias(intLongUdf, s"udf")() :: Nil),
intLongUdf)
plan.initialize(0)
var i = 0
val N = 100000000
val t0 = System.nanoTime()
while(i < N) {
plan(EmptyRow).get(0, intLongUdf.dataType)
plan(EmptyRow).get(0, intLongUdf.dataType)
plan(EmptyRow).get(0, intLongUdf.dataType)
plan(EmptyRow).get(0, intLongUdf.dataType)
plan(EmptyRow).get(0, intLongUdf.dataType)
plan(EmptyRow).get(0, intLongUdf.dataType)
plan(EmptyRow).get(0, intLongUdf.dataType)
plan(EmptyRow).get(0, intLongUdf.dataType)
plan(EmptyRow).get(0, intLongUdf.dataType)
plan(EmptyRow).get(0, intLongUdf.dataType)
i += 1
}
val t1 = System.nanoTime()
println(s"Avg time: ${(t1 - t0).toDouble / N} ns")
}

The output before the patch is:

Avg time: 51.27083294 ns

after, we get:

Avg time: 11.85874227 ns

which is ~5X faster.

Moreover a benchmark has been added for Scala UDF. The output after the patch can be seen in this PR, before the patch, the output was:

================================================================================================
UDF with mixed input types
================================================================================================
Java HotSpot(TM) 64-Bit Server VM 1.8.0_152-b16 on Mac OS X 10.13.6
Intel(R) Core(TM) i7-4558U CPU @ 2.80GHz
long/nullable int/string to string: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
------------------------------------------------------------------------------------------------------------------------
long/nullable int/string to string wholestage off 257 287 42 0,4 2569,5 1,0X
long/nullable int/string to string wholestage on 158 172 18 0,6 1579,0 1,6X
Java HotSpot(TM) 64-Bit Server VM 1.8.0_152-b16 on Mac OS X 10.13.6
Intel(R) Core(TM) i7-4558U CPU @ 2.80GHz
long/nullable int/string to option: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
------------------------------------------------------------------------------------------------------------------------
long/nullable int/string to option wholestage off 104 107 5 1,0 1037,9 1,0X
long/nullable int/string to option wholestage on 80 92 12 1,2 804,0 1,3X
Java HotSpot(TM) 64-Bit Server VM 1.8.0_152-b16 on Mac OS X 10.13.6
Intel(R) Core(TM) i7-4558U CPU @ 2.80GHz
long/nullable int to primitive: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
------------------------------------------------------------------------------------------------------------------------
long/nullable int to primitive wholestage off 71 76 7 1,4 712,1 1,0X
long/nullable int to primitive wholestage on 64 71 6 1,6 636,2 1,1X
================================================================================================
UDF with primitive types
================================================================================================
Java HotSpot(TM) 64-Bit Server VM 1.8.0_152-b16 on Mac OS X 10.13.6
Intel(R) Core(TM) i7-4558U CPU @ 2.80GHz
long/nullable int to string: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
------------------------------------------------------------------------------------------------------------------------
long/nullable int to string wholestage off 60 60 0 1,7 600,3 1,0X
long/nullable int to string wholestage on 55 64 8 1,8 551,2 1,1X
Java HotSpot(TM) 64-Bit Server VM 1.8.0_152-b16 on Mac OS X 10.13.6
Intel(R) Core(TM) i7-4558U CPU @ 2.80GHz
long/nullable int to option: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
------------------------------------------------------------------------------------------------------------------------
long/nullable int to option wholestage off 66 73 9 1,5 663,0 1,0X
long/nullable int to option wholestage on 30 32 2 3,3 300,7 2,2X
Java HotSpot(TM) 64-Bit Server VM 1.8.0_152-b16 on Mac OS X 10.13.6
Intel(R) Core(TM) i7-4558U CPU @ 2.80GHz
long/nullable int/string to primitive: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
------------------------------------------------------------------------------------------------------------------------
long/nullable int/string to primitive wholestage off 32 35 5 3,2 316,7 1,0X
long/nullable int/string to primitive wholestage on 41 68 17 2,4 414,0 0,8X

The improvements are particularly visible in the second case, ie. when only primitive types are used as inputs.

SparkQA · 2019-05-18T17:01:07Z

Test build #105515 has finished for PR 24636 at commit 37ced27.

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

JoshRosen · 2019-05-18T16:11:03Z

+ val initArg = if (CatalystTypeConverters.isPrimitive(dt)) {
+ val convertedTerm = ctx.freshName("conv")
+ s"""
+ |${CodeGenerator.boxedType(dt)} $convertedTerm = ${eval.value};


Out of curiosity, why do we need this extra convertedTerm for the boxing? Could you instead do
Object $argTerm = ${eval.isNull} ? null : ${eval.value};
and avoid the use of an extra variable name? Or if you want more typechecking, do
${CodeGenerator.boxedType(dt)} $argTerm = ${eval.isNull} ? null : ${eval.value};
and used the boxed type as $argTerm's type?
To avoid repetition and more tightly scope the conditional part of the argument convert logic, we even might consider something like this
valboxedType=CodeGenerator.boxedType(dt) valmaybeConverted=if (CatalystTypeConverters.isPrimitive(dt)) { eval.value } else { "$convertersTerm[$i].apply(${eval.value})" } s"$boxedType$argTerm = ${eval.isNull} ? null : $maybeConverted;"

Well, actually my first trial was exactly what you are suggesting here, but it didn't work: indeed it can cause compilation error (the error message is something like no common type for void and int). Then, I also tried:
val boxedType = CodeGenerator.boxedType(dt) val maybeConverted = if (CatalystTypeConverters.isPrimitive(dt)) { s"(${boxedType}) eval.value" } else { "$convertersTerm[$i].apply(${eval.value})" } s"$boxedType $argTerm = ${eval.isNull} ? null : $maybeConverted;"
but this fails too with a confusing error message. Honestly, I am not sure why this 2nd solution doesn't work, since I tried taking the code and compiling it with jdk and it worked. My best guess is that it is a janino bug which doesn't support it.
I did several trials but I haven't found any better alternative as this seemed the only syntax working with janino.

Interesting!
I've filed a bug against Janino to report this issue: janino-compiler/janino#90

viirya · 2019-05-19T01:40:19Z

+ val (funcArgs, initArgs) = evals.zipWithIndex.zip(children.map(_.dataType)).map {
+ case ((eval, i), dt) =>
+ val argTerm = ctx.freshName("arg")
+ val initArg = if (CatalystTypeConverters.isPrimitive(dt)) {


Use CodeGenerator.isPrimitiveType? We can save the change to CatalystTypeConverters.

it doesn't work, you can see the UT failures. For types like timestamp it is not the same.

SparkQA · 2019-05-19T14:06:26Z

Test build #105532 has finished for PR 24636 at commit 3df235d.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-05-19T17:03:47Z

Test build #105533 has finished for PR 24636 at commit ad57acf.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

This reverts commit ad57acf.

SparkQA · 2019-05-19T20:41:33Z

Test build #105537 has finished for PR 24636 at commit fead323.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

mgaido91 · 2019-05-20T08:02:06Z

retest this please

SparkQA · 2019-05-20T11:04:52Z

Test build #105562 has finished for PR 24636 at commit fead323.

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

kiszk · 2019-05-21T01:56:20Z

Good PR. I will review this carefully.

One minor comment: I like performance improvements using Benchmark class. On the other hand, I am not convinced by the first experiment in the description. Since there is no warm-up time. it may include the execution time on interpreter instead of native code.

mgaido91 · 2019-05-21T09:41:59Z

Thanks for you comment @kiszk .

I added the warm-up, and the result is barely the same. Here is the code:

test("SPARK-27684: perf evaluation") {
val intLongUdf = ScalaUDF(
(a: Int, b: Long) => a + b, LongType,
Literal(1) :: Literal(1L) :: Nil,
true :: true :: Nil,
nullable = false)
val plan = generateProject(
MutableProjection.create(Alias(intLongUdf, s"udf")() :: Nil),
intLongUdf)
plan.initialize(0)
var i = 0
val N = 100000000
while(i < 1000) {
plan(EmptyRow).get(0, intLongUdf.dataType)
plan(EmptyRow).get(0, intLongUdf.dataType)
plan(EmptyRow).get(0, intLongUdf.dataType)
plan(EmptyRow).get(0, intLongUdf.dataType)
plan(EmptyRow).get(0, intLongUdf.dataType)
plan(EmptyRow).get(0, intLongUdf.dataType)
plan(EmptyRow).get(0, intLongUdf.dataType)
plan(EmptyRow).get(0, intLongUdf.dataType)
plan(EmptyRow).get(0, intLongUdf.dataType)
plan(EmptyRow).get(0, intLongUdf.dataType)
i += 1
}
i = 0
val t0 = System.nanoTime()
while(i < N) {
plan(EmptyRow).get(0, intLongUdf.dataType)
plan(EmptyRow).get(0, intLongUdf.dataType)
plan(EmptyRow).get(0, intLongUdf.dataType)
plan(EmptyRow).get(0, intLongUdf.dataType)
plan(EmptyRow).get(0, intLongUdf.dataType)
plan(EmptyRow).get(0, intLongUdf.dataType)
plan(EmptyRow).get(0, intLongUdf.dataType)
plan(EmptyRow).get(0, intLongUdf.dataType)
plan(EmptyRow).get(0, intLongUdf.dataType)
plan(EmptyRow).get(0, intLongUdf.dataType)
i += 1
}
val t1 = System.nanoTime()
println(s"Avg time: ${(t1 - t0).toDouble / N} ns")
}

and the results are:

Old Avg time: 49.58303799 ns
New Avg time: 12.66588096 ns

Any more comments/concerns?

skambha · 2019-05-22T00:49:31Z

+ doRunBenchmarkWithPrimitiveTypes(sampleUDF, cardinality)
+ }
+
+ codegenBenchmark("long/nullable int/string to primitive", cardinality) {


Looks like a typo -- should this be "long/nullable int to primitive"

skambha · 2019-05-22T00:51:08Z

+ doRunBenchmarkWithMixedTypes(sampleUDF, cardinality)
+ }
+
+ codegenBenchmark("long/nullable int to primitive", cardinality) {


"long/nullable int to primitive" , change to "long/nullable int/string to primitive"

skambha · 2019-05-22T01:03:11Z

+ * Results will be written to "benchmarks/UDFBenchmark-results.txt".
+ * }}}
+ */
+object UDFBenchmark extends SqlBasedBenchmark {


Thanks @mgaido91 for your work on this performance improvement. I'm curious if you tried the JIRA test case from @JoshRosen with your changes. How close does this get us? Also do you think it might be worthwhile to add that test in this benchmark suite as well.

Well, everything can be added. If you think it is critical, we can add it. Indeed, the test reported in the description is very similar, as it is doing a + 1, which is not so different from an identity. I think the point here is to identify how much from the overhead is saved, and the tests performed show that the overhead is reduced significantly.

Anyway I added it, as shown in the results the overhead is now ~ 20% instead of ~50%

SparkQA · 2019-05-22T09:09:28Z

Test build #105673 has finished for PR 24636 at commit 74e70f5.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-05-22T09:50:29Z

Test build #105675 has finished for PR 24636 at commit 010b3d4.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

mgaido91 · 2019-05-22T11:36:59Z

retest this please

SparkQA · 2019-05-22T13:05:28Z

Test build #105690 has finished for PR 24636 at commit 010b3d4.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

mgaido91 · 2019-05-22T13:20:15Z

retest this please

SparkQA · 2019-05-22T16:29:06Z

Test build #105693 has finished for PR 24636 at commit 010b3d4.

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

kiszk · 2019-05-24T10:26:46Z

+ val convertedTerm = ctx.freshName("conv")
+ s"""
+ |${CodeGenerator.boxedType(dt)} $convertedTerm = ${eval.value};
+ |Object $argTerm = ${eval.isNull} ? null : $convertedTerm;


Can we remove ${eval.isNull} ? ... if ${eval.isNull} is compile-time constant?

IMO maybe we can do this in a separate PR?
I wouldn't be surprised if there's other places in Spark (beyond this method / file) where we could apply similar fixes (and if we're going to apply this in a lot of places then it might even be nice to write some sort of helper for generating / managing null checks.
Does this PR look good to you otherwise?

I agree with @JoshRosen . It would be better to define a generic approach for addressing this and there are several instances of it. I can create another JIRA and start a PR for that if you're ok with it.

gatorsmile · 2019-05-24T16:28:30Z

cc @ueshin

JoshRosen · 2019-05-28T14:32:19Z

@kiszk @ueshin @gatorsmile, does this PR now look good to you? If so, I'd like to get this merged soon so that it doesn't go stale.

ueshin

I'm sorry for the delay.
LGTM.

JoshRosen · 2019-05-31T00:10:01Z

I've merged this to master. Thanks @mgaido91 (and to everyone who helped with review)!

mgaido91 · 2019-05-31T08:01:19Z

thank you all!

[SPARK-27684][SQL] Avoid conversion overhead for primitive types
37ced27

JoshRosen reviewed May 18, 2019
View reviewed changes

viirya reviewed May 19, 2019
View reviewed changes

JoshRosen mentioned this pull request May 19, 2019
"Incompatible expression types" or verification errors when using ternary expressions with null in one branch janino-compiler/janino#90
Closed

mgaido91 force-pushed the SPARK-27684 branch 2 times, most recently from caea444 to ad57acfCompare May 19, 2019 14:59

address comment
ad57acf

Revert "address comment"
fead323
This reverts commit ad57acf.

skambha reviewed May 22, 2019
View reviewed changes

mgaido91 added 2 commits May 22, 2019 09:50

fix typos
74e70f5

add benchmark for identity function overhead
010b3d4

kiszk reviewed May 24, 2019
View reviewed changes

ueshin approved these changes May 30, 2019
View reviewed changes

JoshRosen closed this in 93db7b8May 31, 2019

mgaido91 mentioned this pull request Jul 14, 2019
[SPARK-28133][SQL] Add acosh/asinh/atanh functions to SQL #25041
Closed

Uh oh!

Conversation

mgaido91 commented May 18, 2019

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented May 18, 2019

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!

viiryaMay 19, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented May 19, 2019

Uh oh!

SparkQA commented May 19, 2019

Uh oh!

SparkQA commented May 19, 2019

Uh oh!

mgaido91 commented May 20, 2019

Uh oh!

SparkQA commented May 20, 2019

Uh oh!

kiszk commented May 21, 2019

Uh oh!

mgaido91 commented May 21, 2019

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!

SparkQA commented May 22, 2019

Uh oh!

SparkQA commented May 22, 2019

Uh oh!

mgaido91 commented May 22, 2019

Uh oh!

SparkQA commented May 22, 2019

Uh oh!

mgaido91 commented May 22, 2019

Uh oh!

SparkQA commented May 22, 2019

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!

gatorsmile commented May 24, 2019

Uh oh!

JoshRosen commented May 28, 2019

Uh oh!

ueshin left a comment

Choose a reason for hiding this comment

Uh oh!

JoshRosen commented May 31, 2019

Uh oh!

mgaido91 commented May 31, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

viiryaMay 19, 2019•
edited
Loading