You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by maropu <gi...@git.apache.org> on 2017/08/22 08:20:20 UTC
[GitHub] spark pull request #19021: [SPARK-21603][SQL][FOLLOW-UP] Change the default ...
GitHub user maropu opened a pull request:
https://github.com/apache/spark/pull/19021
[SPARK-21603][SQL][FOLLOW-UP] Change the default value of maxLinesPerFunction into 4000
## What changes were proposed in this pull request?
This pr changed the default value of `maxLinesPerFunction` into `4000`. In #18810, we had this new option to disable code generation for too long functions and I found this option only affected `Q17` and `Q66` in TPC-DS. But, `Q66` had some performance regression:
```
Q17 w/o #18810, 3224ms --> q17 w/#18810, 2627ms (improvement)
Q66 w/o #18810, 1712ms --> q66 w/#18810, 3032ms (regression)
```
To keep the previous performance in TPC-DS, we better set higher value at `maxLinesPerFunction` by default.
## How was this patch tested?
Existing tests.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/maropu/spark SPARK-21603-FOLLOWUP-1
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/19021.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #19021
----
commit d0bc6a62cd58147c9809d9c99fd4452cf7e40aff
Author: Takeshi Yamamuro <ya...@apache.org>
Date: 2017-08-22T08:05:29Z
Change the default value of maxLinesPerFunction into 4000
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19021: [SPARK-21603][SQL][FOLLOW-UP] Change the default ...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on a diff in the pull request:
https://github.com/apache/spark/pull/19021#discussion_r134657177
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala ---
@@ -577,10 +577,10 @@ object SQLConf {
.doc("The maximum lines of a single Java function generated by whole-stage codegen. " +
"When the generated function exceeds this threshold, " +
"the whole-stage codegen is deactivated for this subtree of the current query plan. " +
- "The default value 2667 is the max length of byte code JIT supported " +
- "for a single function(8000) divided by 3.")
+ "The default value 4000 is the max length of byte code JIT supported " +
+ "for a single function(8000) divided by 2.")
.intConf
- .createWithDefault(2667)
--- End diff --
YES.... Just double check it. The default of `HugeMethodLimit`is 8000... Thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19021: [SPARK-21603][SQL][FOLLOW-UP] Change the default value o...
Posted by maropu <gi...@git.apache.org>.
Github user maropu commented on the issue:
https://github.com/apache/spark/pull/19021
I'll check performance changes in TPC-DS.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19021: [SPARK-21603][SQL][FOLLOW-UP] Change the default value o...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/19021
LGTM
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19021: [SPARK-21603][SQL][FOLLOW-UP] Change the default value o...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19021
Merged build finished. Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19021: [SPARK-21603][SQL][FOLLOW-UP] Change the default value o...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/19021
Thanks! Merging to master.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19021: [SPARK-21603][SQL][FOLLOW-UP] Change the default value o...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19021
**[Test build #80976 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/80976/testReport)** for PR 19021 at commit [`d0bc6a6`](https://github.com/apache/spark/commit/d0bc6a62cd58147c9809d9c99fd4452cf7e40aff).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19021: [SPARK-21603][SQL][FOLLOW-UP] Change the default value o...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on the issue:
https://github.com/apache/spark/pull/19021
@maropu Should be a good idea. Especially the number of lines of code may not be intuitive to set for this purpose.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19021: [SPARK-21603][SQL][FOLLOW-UP] Change the default value o...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19021
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/80976/
Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19021: [SPARK-21603][SQL][FOLLOW-UP] Change the default ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/19021
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19021: [SPARK-21603][SQL][FOLLOW-UP] Change the default ...
Posted by maropu <gi...@git.apache.org>.
Github user maropu commented on a diff in the pull request:
https://github.com/apache/spark/pull/19021#discussion_r134631466
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala ---
@@ -577,10 +577,10 @@ object SQLConf {
.doc("The maximum lines of a single Java function generated by whole-stage codegen. " +
"When the generated function exceeds this threshold, " +
"the whole-stage codegen is deactivated for this subtree of the current query plan. " +
- "The default value 2667 is the max length of byte code JIT supported " +
- "for a single function(8000) divided by 3.")
+ "The default value 4000 is the max length of byte code JIT supported " +
+ "for a single function(8000) divided by 2.")
.intConf
- .createWithDefault(2667)
--- End diff --
I've already checked `2800`; this value activated too-long-function optimization in Q17/Q66 and Q66 had regression. So, I think `2731` and `2049` possibly have the same regression (I've also checked `2900` that disabled this in Q17/Q66).
btw, why the computation is based on `8K`? It seems the threshold of `DontCompileHugeMethods` is 8000?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19021: [SPARK-21603][SQL][FOLLOW-UP] Change the default value o...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19021
**[Test build #80976 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/80976/testReport)** for PR 19021 at commit [`d0bc6a6`](https://github.com/apache/spark/commit/d0bc6a62cd58147c9809d9c99fd4452cf7e40aff).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19021: [SPARK-21603][SQL][FOLLOW-UP] Change the default value o...
Posted by maropu <gi...@git.apache.org>.
Github user maropu commented on the issue:
https://github.com/apache/spark/pull/19021
Just for your info, again, I looked into this issue in TPC-DS quries; I added [some code](https://github.com/apache/spark/compare/master...maropu:SPARK-21603-FOLLOWUP-3) to check the actual bytecode size of these quries and I found the gen'd function in Q17/Q66 only had too-long bytecode over `8000`:
```
===== TPCDS QUERY BENCHMARK OUTPUT FOR q17 =====
17/08/23 14:45:02 WARN CodeGenerator: GeneratedClass.agg_doAggregateWithKeys is too large to do JIT compilation on HotSpot; the size of agg_doAggregateWithKeys is 17665; the limit is 8000
===== TPCDS QUERY BENCHMARK OUTPUT FOR q66 =====
17/08/23 14:55:39 WARN CodeGenerator: GeneratedClass.agg_doAggregateWithKeys is too large to do JIT compilation on HotSpot; the size of agg_doAggregateWithKeys is 11012; the limit is 8000
17/08/23 14:55:39 WARN CodeGenerator: GeneratedClass.agg_doAggregateWithKeys is too large to do JIT compilation on HotSpot; the size of agg_doAggregateWithKeys is 13420; the limit is 8000
17/08/23 14:55:39 WARN CodeGenerator: GeneratedClass.agg_doAggregateWithKeys is too large to do JIT compilation on HotSpot; the size of agg_doAggregateWithKeys is 16641; the limit is 8000
```
BTW, why we don't check if gen'd bytecode size is over `8000` directly instead of code line num. in #18810? cc: @gatorsmile @viirya @kiszk
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19021: [SPARK-21603][SQL][FOLLOW-UP] Change the default ...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on a diff in the pull request:
https://github.com/apache/spark/pull/19021#discussion_r134568008
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala ---
@@ -577,10 +577,10 @@ object SQLConf {
.doc("The maximum lines of a single Java function generated by whole-stage codegen. " +
"When the generated function exceeds this threshold, " +
"the whole-stage codegen is deactivated for this subtree of the current query plan. " +
- "The default value 2667 is the max length of byte code JIT supported " +
- "for a single function(8000) divided by 3.")
+ "The default value 4000 is the max length of byte code JIT supported " +
+ "for a single function(8000) divided by 2.")
.intConf
- .createWithDefault(2667)
--- End diff --
Could you also try 2731 and 2049? and see the perf difference?
```
8K = 8192
8192 / 3 + 1 = 2731
8192 / 4 + 1 = 2049
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19021: [SPARK-21603][SQL][FOLLOW-UP] Change the default value o...
Posted by maropu <gi...@git.apache.org>.
Github user maropu commented on the issue:
https://github.com/apache/spark/pull/19021
Got the numbers and see them [here](https://docs.google.com/spreadsheets/d/10-tmFdYcCQpYwhBV4BFdWWCa8LqXdEqAe2WoP4Abfqk/edit?usp=sharing). The numbers seems to be the almost same with the master just before #18810 merged. @gatorsmile @viirya
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19021: [SPARK-21603][SQL][FOLLOW-UP] Change the default value o...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/19021
cc @rednaxelafx
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19021: [SPARK-21603][SQL][FOLLOW-UP] Change the default value o...
Posted by maropu <gi...@git.apache.org>.
Github user maropu commented on the issue:
https://github.com/apache/spark/pull/19021
Also, I talked with @viirya [here](https://github.com/apache/spark/pull/18931#issuecomment-323697425) and `-1` seems to be more natural to disable this too-long-function option. If we don't strongly disagree with this, I could make [another trivial pr](https://github.com/apache/spark/compare/master...maropu:SPARK-21603-FOLLOWUP-2) to fix, too. @gatorsmile
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org