You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by lw-lin <gi...@git.apache.org> on 2016/09/01 07:46:13 UTC
[GitHub] spark pull request #14914: [SPARK-17359][SQL][MLLib] Use ArrayBuffer.+=(A) i...
GitHub user lw-lin opened a pull request:
https://github.com/apache/spark/pull/14914
[SPARK-17359][SQL][MLLib] Use ArrayBuffer.+=(A) instead of ArrayBuffer.append(A) in performance critical paths
## What changes were proposed in this pull request?
We should generally use `ArrayBuffer.+=(A)` rather than `ArrayBuffer.append(A)`, because `append(A)` would involve extra boxing / unboxing.
## How was this patch tested?
N/A
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/lw-lin/spark append_to_plus_eq_v2
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/14914.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 #14914
----
commit fba1c5e430f90163cc10266812f2a0137d012be3
Author: Liwei Lin <lw...@gmail.com>
Date: 2016-08-15T02:56:58Z
append -> +=
----
---
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 #14914: [SPARK-17359][SQL][MLLib] Use ArrayBuffer.+=(A) i...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/14914#discussion_r77135576
--- Diff: mllib-local/src/main/scala/org/apache/spark/ml/linalg/Matrices.scala ---
@@ -999,7 +999,7 @@ object Matrices {
val data = new ArrayBuffer[(Int, Int, Double)]()
dnMat.foreachActive { (i, j, v) =>
if (v != 0.0) {
- data.append((i, j + startCol, v))
+ data.+=((i, j + startCol, v))
--- End diff --
() can't create a tuple of 1 element so it shouldn't be ambiguous at all. I think it's preferable to the slightly funny .+= 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 issue #14914: [SPARK-17359][SQL][MLLib] Use ArrayBuffer.+=(A) instead ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/14914
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/64975/
Test FAILed.
---
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 #14914: [SPARK-17359][SQL][MLLib] Use ArrayBuffer.+=(A) instead ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/14914
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/64769/
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 #14914: [SPARK-17359][SQL][MLLib] Use ArrayBuffer.+=(A) instead ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/14914
**[Test build #3248 has finished](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/3248/consoleFull)** for PR 14914 at commit [`f997c2b`](https://github.com/apache/spark/commit/f997c2b3f5c0e718dee029356eab25087e742054).
* 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 #14914: [SPARK-17359][SQL][MLLib] Use ArrayBuffer.+=(A) instead ...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/14914
+1. It's a shame about the tuples issues and `Tuple3(...)` is a little ugly but no big deal.
---
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 #14914: [SPARK-17359][SQL][MLLib] Use ArrayBuffer.+=(A) instead ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/14914
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 #14914: [SPARK-17359][SQL][MLLib] Use ArrayBuffer.+=(A) instead ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/14914
**[Test build #64975 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/64975/consoleFull)** for PR 14914 at commit [`f997c2b`](https://github.com/apache/spark/commit/f997c2b3f5c0e718dee029356eab25087e742054).
---
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 #14914: [SPARK-17359][SQL][MLLib] Use ArrayBuffer.+=(A) i...
Posted by lw-lin <gi...@git.apache.org>.
Github user lw-lin commented on a diff in the pull request:
https://github.com/apache/spark/pull/14914#discussion_r77131280
--- Diff: mllib-local/src/main/scala/org/apache/spark/ml/linalg/Matrices.scala ---
@@ -999,7 +999,7 @@ object Matrices {
val data = new ArrayBuffer[(Int, Int, Double)]()
dnMat.foreachActive { (i, j, v) =>
if (v != 0.0) {
- data.append((i, j + startCol, v))
+ data.+=((i, j + startCol, v))
--- End diff --
> I think it's reasonable to fix this everywhere.
ok let me do that -- I was conservative in fixing only these instances that were in a loop.
---
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 #14914: [SPARK-17359][SQL][MLLib] Use ArrayBuffer.+=(A) instead ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/14914
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 pull request #14914: [SPARK-17359][SQL][MLLib] Use ArrayBuffer.+=(A) i...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/14914
---
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 #14914: [SPARK-17359][SQL][MLLib] Use ArrayBuffer.+=(A) instead ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/14914
**[Test build #64769 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/64769/consoleFull)** for PR 14914 at commit [`fba1c5e`](https://github.com/apache/spark/commit/fba1c5e430f90163cc10266812f2a0137d012be3).
---
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 #14914: [SPARK-17359][SQL][MLLib] Use ArrayBuffer.+=(A) i...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/14914#discussion_r77129894
--- Diff: mllib-local/src/main/scala/org/apache/spark/ml/linalg/Matrices.scala ---
@@ -999,7 +999,7 @@ object Matrices {
val data = new ArrayBuffer[(Int, Int, Double)]()
dnMat.foreachActive { (i, j, v) =>
if (v != 0.0) {
- data.append((i, j + startCol, v))
+ data.+=((i, j + startCol, v))
--- End diff --
You can write `data += ((i, j + startCol, v))`
Yes, I think it may be worth optimizing this because append actually expects varargs.
There are many more instances in the code though. I think it's reasonable to fix this everywhere. At least non-test code, and at least anything where performance could be important.
---
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 #14914: [SPARK-17359][SQL][MLLib] Use ArrayBuffer.+=(A) instead ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/14914
**[Test build #3248 has started](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/3248/consoleFull)** for PR 14914 at commit [`f997c2b`](https://github.com/apache/spark/commit/f997c2b3f5c0e718dee029356eab25087e742054).
---
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 #14914: [SPARK-17359][SQL][MLLib] Use ArrayBuffer.+=(A) instead ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/14914
**[Test build #64770 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/64770/consoleFull)** for PR 14914 at commit [`980a3a4`](https://github.com/apache/spark/commit/980a3a46e42330a04ab5ed66867123681806a4c0).
* 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 pull request #14914: [SPARK-17359][SQL][MLLib] Use ArrayBuffer.+=(A) i...
Posted by lw-lin <gi...@git.apache.org>.
Github user lw-lin commented on a diff in the pull request:
https://github.com/apache/spark/pull/14914#discussion_r77130880
--- Diff: mllib-local/src/main/scala/org/apache/spark/ml/linalg/Matrices.scala ---
@@ -999,7 +999,7 @@ object Matrices {
val data = new ArrayBuffer[(Int, Int, Double)]()
dnMat.foreachActive { (i, j, v) =>
if (v != 0.0) {
- data.append((i, j + startCol, v))
+ data.+=((i, j + startCol, v))
--- End diff --
I don't have strong preference, but people might mis-interpret `data += ((i, j + startCol, v))` as adding a tuple of a tuple?
---
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 #14914: [SPARK-17359][SQL][MLLib] Use ArrayBuffer.+=(A) i...
Posted by lw-lin <gi...@git.apache.org>.
Github user lw-lin commented on a diff in the pull request:
https://github.com/apache/spark/pull/14914#discussion_r77128029
--- Diff: mllib-local/src/main/scala/org/apache/spark/ml/linalg/Matrices.scala ---
@@ -999,7 +999,7 @@ object Matrices {
val data = new ArrayBuffer[(Int, Int, Double)]()
dnMat.foreachActive { (i, j, v) =>
if (v != 0.0) {
- data.append((i, j + startCol, v))
+ data.+=((i, j + startCol, v))
--- End diff --
Writing it as `data += (i, j + startCol, v)` would yield compilation errors:
```
Too many arguments for methods =+(A)
Type mismatch, expected: (Int, Int, Double), actual: Int
```
thus here it written as `data.+=((i, j + startCol, v))`
---
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 #14914: [SPARK-17359][SQL][MLLib] Use ArrayBuffer.+=(A) instead ...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/14914
Merged 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 pull request #14914: [SPARK-17359][SQL][MLLib] Use ArrayBuffer.+=(A) i...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/14914#discussion_r77141688
--- Diff: mllib-local/src/main/scala/org/apache/spark/ml/linalg/Matrices.scala ---
@@ -999,7 +999,7 @@ object Matrices {
val data = new ArrayBuffer[(Int, Int, Double)]()
dnMat.foreachActive { (i, j, v) =>
if (v != 0.0) {
- data.append((i, j + startCol, v))
+ data.+=((i, j + startCol, v))
--- End diff --
You can do
```
data += Tuple3(i, j + startCol, v)
```
probably the most clear
---
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 #14914: [SPARK-17359][SQL][MLLib] Use ArrayBuffer.+=(A) instead ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/14914
**[Test build #64769 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/64769/consoleFull)** for PR 14914 at commit [`fba1c5e`](https://github.com/apache/spark/commit/fba1c5e430f90163cc10266812f2a0137d012be3).
* 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 #14914: [SPARK-17359][SQL][MLLib] Use ArrayBuffer.+=(A) instead ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/14914
**[Test build #64770 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/64770/consoleFull)** for PR 14914 at commit [`980a3a4`](https://github.com/apache/spark/commit/980a3a46e42330a04ab5ed66867123681806a4c0).
---
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 #14914: [SPARK-17359][SQL][MLLib] Use ArrayBuffer.+=(A) instead ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/14914
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/64770/
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 #14914: [SPARK-17359][SQL][MLLib] Use ArrayBuffer.+=(A) instead ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/14914
**[Test build #64975 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/64975/consoleFull)** for PR 14914 at commit [`f997c2b`](https://github.com/apache/spark/commit/f997c2b3f5c0e718dee029356eab25087e742054).
* This patch **fails Spark unit 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 #14914: [SPARK-17359][SQL][MLLib] Use ArrayBuffer.+=(A) instead ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/14914
Merged build finished. Test FAILed.
---
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