You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by seyfe <gi...@git.apache.org> on 2016/09/09 19:34:45 UTC
[GitHub] spark pull request #15032: [SPARK-17480][SQL] Improve perf of gatherCompress...
GitHub user seyfe opened a pull request:
https://github.com/apache/spark/pull/15032
[SPARK-17480][SQL] Improve perf of gatherCompressibilityStats by removing List.length
## What changes were proposed in this pull request?
Scala's List.length method is O(N) and it makes the gatherCompressibilityStats function O(N^2). Eliminate the List.length calls by writing it in Scala way.
https://github.com/scala/scala/blob/2.10.x/src/library/scala/collection/LinearSeqOptimized.scala#L36
## How was this patch tested?
Profiled a Spark job and found that CompressibleColumnBuilder is using 39% of the CPU. Out of this 39% CompressibleColumnBuilder->gatherCompressibilityStats is using 23% of it. 6.24% of the CPU is spend on List.length which is called inside gatherCompressibilityStats.
After this change we started to save 6.24% of the CPU.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/seyfe/spark gatherCompressibilityStats
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/15032.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 #15032
----
commit f181b92c1f3a4f91707bc1912dde29987b01c08f
Author: Ergin Seyfe <es...@fb.com>
Date: 2016-09-09T19:16:39Z
Improve performance of gatherCompressibilityStats by removing List.length
----
---
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 #15032: [SPARK-17480][SQL] Improve perf of gatherCompressibility...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/15032
**[Test build #65165 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/65165/consoleFull)** for PR 15032 at commit [`f181b92`](https://github.com/apache/spark/commit/f181b92c1f3a4f91707bc1912dde29987b01c08f).
---
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 #15032: [SPARK-17480][SQL] Improve perf of gatherCompressibility...
Posted by seyfe <gi...@git.apache.org>.
Github user seyfe commented on the issue:
https://github.com/apache/spark/pull/15032
I looked at HiveInspectors class and you are right. We could replace them with
```
for (i <- 0 until fieldRefs.size) {
// or alternatively:
for (i <- inspectors.indices) {
```
in any case, these will create a Range object and do a foreach like:
`(0 until fieldRefs.size).foreach`
So, I would suggest keeping them as it's but let's get the length and store in a local variable so we won't call it in a loop. I will prepare the change.
---
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 #15032: [SPARK-17480][SQL] Improve perf of gatherCompressibility...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/15032
**[Test build #65165 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/65165/consoleFull)** for PR 15032 at commit [`f181b92`](https://github.com/apache/spark/commit/f181b92c1f3a4f91707bc1912dde29987b01c08f).
* 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 #15032: [SPARK-17480][SQL] Improve perf of gatherCompressibility...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/15032
Ok though more readable with _ syntax
---
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 #15032: [SPARK-17480][SQL] Improve performance by removin...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on a diff in the pull request:
https://github.com/apache/spark/pull/15032#discussion_r78265623
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/aggregate/AggregationIterator.scala ---
@@ -181,11 +182,7 @@ abstract class AggregationIterator(
// Process all expression-based aggregate functions.
updateProjection.target(currentBuffer)(joinedRow(currentBuffer, row))
// Process all imperative aggregate functions.
- var i = 0
- while (i < updateFunctions.length) {
- updateFunctions(i)(currentBuffer, row)
- i += 1
- }
+ updateFunctions.foreach(updateFunction => updateFunction(currentBuffer, row))
--- End diff --
Alternatively, why not turn `updateFunctions` into an `Array`?
---
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 #15032: [SPARK-17480][SQL] Improve perf of gatherCompressibility...
Posted by seyfe <gi...@git.apache.org>.
Github user seyfe commented on the issue:
https://github.com/apache/spark/pull/15032
Sure. Changed to _. syntax as suggested.
---
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 #15032: [SPARK-17480][SQL] Improve performance by removin...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on a diff in the pull request:
https://github.com/apache/spark/pull/15032#discussion_r78265127
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/aggregate/AggregationIterator.scala ---
@@ -181,11 +182,7 @@ abstract class AggregationIterator(
// Process all expression-based aggregate functions.
updateProjection.target(currentBuffer)(joinedRow(currentBuffer, row))
// Process all imperative aggregate functions.
- var i = 0
- while (i < updateFunctions.length) {
- updateFunctions(i)(currentBuffer, row)
- i += 1
- }
+ updateFunctions.foreach(updateFunction => updateFunction(currentBuffer, row))
--- End diff --
I just wonder if it'd make sense if we cache the length and use just a normal `while` loop as done above. This seems not encouraged [1]. I believe this is fine but not for critical path. IIUC, this will , at least, introduce virtual function calls for each row. I found a blog just to be verbose to make sure myself [2]. I am worried because this change can be a example in the future. Maybe, we need a clear reason to change this.
[1] https://github.com/databricks/scala-style-guide#traversal-and-zipwithindex
[2] http://www.scalablescala.com/roller/scala/entry/why_is_using_for_foreach
---
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 #15032: [SPARK-17480][SQL] Improve perf of gatherCompressibility...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/15032
Jenkins test this please
---
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 #15032: [SPARK-17480][SQL] Improve performance by removing or ca...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/15032
**[Test build #3258 has started](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/3258/consoleFull)** for PR 15032 at commit [`75e0d8d`](https://github.com/apache/spark/commit/75e0d8d0e26e962d61e214c79ac1e33d372911c6).
---
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 #15032: [SPARK-17480][SQL] Improve perf of gatherCompressibility...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/15032
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/65165/
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 #15032: [SPARK-17480][SQL] Improve performance by removing or ca...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/15032
Merged to master and to 2.0 because it's a very low risk change and potential perf bottleneck
---
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 #15032: [SPARK-17480][SQL] Improve performance by removin...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on a diff in the pull request:
https://github.com/apache/spark/pull/15032#discussion_r78267647
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/aggregate/AggregationIterator.scala ---
@@ -181,11 +182,7 @@ abstract class AggregationIterator(
// Process all expression-based aggregate functions.
updateProjection.target(currentBuffer)(joinedRow(currentBuffer, row))
// Process all imperative aggregate functions.
- var i = 0
- while (i < updateFunctions.length) {
- updateFunctions(i)(currentBuffer, row)
- i += 1
- }
+ updateFunctions.foreach(updateFunction => updateFunction(currentBuffer, row))
--- End diff --
For [1], it's written as below and it seems including functional transformations
> Use while loops instead of for loops or functional transformations (e.g. `map`, `foreach`).
Then, could you then confirm the bytecode is virtually the same or more efficient? My point was that I think we need a clear reason in this case.
---
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 #15032: [SPARK-17480][SQL] Improve performance by removin...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on a diff in the pull request:
https://github.com/apache/spark/pull/15032#discussion_r78267738
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/aggregate/AggregationIterator.scala ---
@@ -181,11 +182,7 @@ abstract class AggregationIterator(
// Process all expression-based aggregate functions.
updateProjection.target(currentBuffer)(joinedRow(currentBuffer, row))
// Process all imperative aggregate functions.
- var i = 0
- while (i < updateFunctions.length) {
- updateFunctions(i)(currentBuffer, row)
- i += 1
- }
+ updateFunctions.foreach(updateFunction => updateFunction(currentBuffer, row))
--- End diff --
Ah, right, i rushed both your comment and blog (please excuse this as it's my phone and I am outside). Yeap. I am fine with this.
---
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 #15032: [SPARK-17480][SQL] Improve perf of gatherCompressibility...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/15032
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 #15032: [SPARK-17480][SQL] Improve performance by removing or ca...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/15032
**[Test build #3260 has started](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/3260/consoleFull)** for PR 15032 at commit [`75e0d8d`](https://github.com/apache/spark/commit/75e0d8d0e26e962d61e214c79ac1e33d372911c6).
---
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 #15032: [SPARK-17480][SQL] Improve performance by removin...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/15032
---
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 #15032: [SPARK-17480][SQL] Improve perf of gatherCompressibility...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/15032
I was double-checking this for my own understanding, and I see that although these are all `Seq`s, that the default implementation is `$colon$colon` and indeed it is a `List`.
I skimmed for other instances of this and found some possible matches in `HiveInspectors` and `AggregationIterator`. Have a look. If they're the same issue we should fix those too as the construction is there if anything for 'optimization' but if it only makes it slower, it should just use the simpler functional idiom.
---
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 #15032: [SPARK-17480][SQL] Improve performance by removin...
Posted by seyfe <gi...@git.apache.org>.
Github user seyfe commented on a diff in the pull request:
https://github.com/apache/spark/pull/15032#discussion_r78266659
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/aggregate/AggregationIterator.scala ---
@@ -181,11 +182,7 @@ abstract class AggregationIterator(
// Process all expression-based aggregate functions.
updateProjection.target(currentBuffer)(joinedRow(currentBuffer, row))
// Process all imperative aggregate functions.
- var i = 0
- while (i < updateFunctions.length) {
- updateFunctions(i)(currentBuffer, row)
- i += 1
- }
+ updateFunctions.foreach(updateFunction => updateFunction(currentBuffer, row))
--- End diff --
Thanks for your input @HyukjinKwon! I avoided the zipWithIndex because of that reason and kept the while loops unchanged. Just cached the length due to specified reason.
However for the parts that we don't need the index, we can use foreach method. It's not same as what is posted in [2].
The problem in [2] is `for (i <- Range) ` will be converted to `Range.foreach`. However in this particular case, we don't create the Range object and I don't think there is any virtual function calls. Please correct me if I am wrong.
---
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 #15032: [SPARK-17480][SQL] Improve perf of gatherCompressibility...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/15032
Can one of the admins verify this patch?
---
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 #15032: [SPARK-17480][SQL] Improve performance by removing or ca...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/15032
**[Test build #3258 has finished](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/3258/consoleFull)** for PR 15032 at commit [`75e0d8d`](https://github.com/apache/spark/commit/75e0d8d0e26e962d61e214c79ac1e33d372911c6).
* This patch passes all tests.
* This patch **does not merge 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 #15032: [SPARK-17480][SQL] Improve performance by removing or ca...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/15032
**[Test build #3260 has finished](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/3260/consoleFull)** for PR 15032 at commit [`75e0d8d`](https://github.com/apache/spark/commit/75e0d8d0e26e962d61e214c79ac1e33d372911c6).
* 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