You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by sryza <gi...@git.apache.org> on 2014/04/25 22:21:11 UTC
[GitHub] spark pull request: SPARK-1632. Remove unnecessary boxing in compa...
GitHub user sryza opened a pull request:
https://github.com/apache/spark/pull/559
SPARK-1632. Remove unnecessary boxing in compares in ExternalAppendOnlyM...
...ap
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/sryza/spark sandy-spark-1632
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/559.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 #559
----
commit 04e3884c6ce8084ee3d9d2db7ae16924b940b424
Author: Sandy Ryza <sa...@cloudera.com>
Date: 2014-04-25T20:12:08Z
SPARK-1632. Remove unnecessary boxing in compares in ExternalAppendOnlyMap
----
---
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.
---
[GitHub] spark pull request: SPARK-1632. Remove unnecessary boxing in compa...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on the pull request:
https://github.com/apache/spark/pull/559#issuecomment-41439051
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.
---
[GitHub] spark pull request: SPARK-1632. Remove unnecessary boxing in compa...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/559#discussion_r12029918
--- Diff: core/src/main/scala/org/apache/spark/util/collection/ExternalAppendOnlyMap.scala ---
@@ -337,8 +337,8 @@ class ExternalAppendOnlyMap[K, V, C](
}
override def compareTo(other: StreamBuffer): Int = {
- // minus sign because mutable.PriorityQueue dequeues the max, not the min
- -minKeyHash.compareTo(other.minKeyHash)
+ // descending order because mutable.PriorityQueue dequeues the max, not the min
+ if (other.minKeyHash < minKeyHash) -1 else if (other.minKeyHash == minKeyHash) 0 else 1
--- End diff --
Java 7 only :(
http://docs.oracle.com/javase/7/docs/api/java/lang/Integer.html#compare(int, int)
Guava has `Ints.compare()` though:
http://docs.guava-libraries.googlecode.com/git/javadoc/com/google/common/primitives/Ints.html#compare(int, int)
---
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.
---
[GitHub] spark pull request: SPARK-1632. Remove unnecessary boxing in compa...
Posted by mridulm <gi...@git.apache.org>.
Github user mridulm commented on a diff in the pull request:
https://github.com/apache/spark/pull/559#discussion_r12029894
--- Diff: core/src/main/scala/org/apache/spark/util/collection/ExternalAppendOnlyMap.scala ---
@@ -337,8 +337,8 @@ class ExternalAppendOnlyMap[K, V, C](
}
override def compareTo(other: StreamBuffer): Int = {
- // minus sign because mutable.PriorityQueue dequeues the max, not the min
- -minKeyHash.compareTo(other.minKeyHash)
+ // descending order because mutable.PriorityQueue dequeues the max, not the min
+ if (other.minKeyHash < minKeyHash) -1 else if (other.minKeyHash == minKeyHash) 0 else 1
--- End diff --
It is better to use Integer.compare instead of writing these manually.
---
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.
---
[GitHub] spark pull request: SPARK-1632. Remove unnecessary boxing in compa...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/559#discussion_r12014441
--- Diff: core/src/main/scala/org/apache/spark/util/collection/ExternalAppendOnlyMap.scala ---
@@ -422,7 +422,8 @@ class ExternalAppendOnlyMap[K, V, C](
private[spark] object ExternalAppendOnlyMap {
private class KCComparator[K, C] extends Comparator[(K, C)] {
def compare(kc1: (K, C), kc2: (K, C)): Int = {
- kc1._1.hashCode().compareTo(kc2._1.hashCode())
+ if (kc1._1.hashCode() < kc2._1.hashCode()) -1
--- End diff --
Worth saving the hashCodes rather than recomputing? it might be non-trivial
---
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.
---
[GitHub] spark pull request: SPARK-1632. Remove unnecessary boxing in compa...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/559#issuecomment-41438931
Merged build finished. All automated tests 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.
---
[GitHub] spark pull request: SPARK-1632. Remove unnecessary boxing in compa...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/559#issuecomment-41441704
All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/14495/
---
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.
---
[GitHub] spark pull request: SPARK-1632. Remove unnecessary boxing in compa...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/559#issuecomment-41435506
Merged build started.
---
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.
---
[GitHub] spark pull request: SPARK-1632. Remove unnecessary boxing in compa...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/559
---
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.
---
[GitHub] spark pull request: SPARK-1632. Remove unnecessary boxing in compa...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/559#issuecomment-41438932
All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/14493/
---
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.
---
[GitHub] spark pull request: SPARK-1632. Remove unnecessary boxing in compa...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/559#discussion_r12030457
--- Diff: core/src/main/scala/org/apache/spark/util/collection/ExternalAppendOnlyMap.scala ---
@@ -337,8 +337,8 @@ class ExternalAppendOnlyMap[K, V, C](
}
override def compareTo(other: StreamBuffer): Int = {
- // minus sign because mutable.PriorityQueue dequeues the max, not the min
- -minKeyHash.compareTo(other.minKeyHash)
+ // descending order because mutable.PriorityQueue dequeues the max, not the min
+ if (other.minKeyHash < minKeyHash) -1 else if (other.minKeyHash == minKeyHash) 0 else 1
--- End diff --
We could also write our own `Utils.compare`, though this is only used in 2 places so I think it's fine to just write it out directly. Also this is a super hot code path so maybe it's better to just avoid function calls altogether (maybe, maybe not).
---
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.
---
[GitHub] spark pull request: SPARK-1632. Remove unnecessary boxing in compa...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/559#issuecomment-41441702
Merged build finished. All automated tests 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.
---
[GitHub] spark pull request: SPARK-1632. Remove unnecessary boxing in compa...
Posted by pwendell <gi...@git.apache.org>.
Github user pwendell commented on the pull request:
https://github.com/apache/spark/pull/559#issuecomment-41452491
Thanks, merged.
---
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.
---
[GitHub] spark pull request: SPARK-1632. Remove unnecessary boxing in compa...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/559#issuecomment-41437706
Merged build triggered.
---
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.
---
[GitHub] spark pull request: SPARK-1632. Remove unnecessary boxing in compa...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/559#issuecomment-41435496
Merged build triggered.
---
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.
---
[GitHub] spark pull request: SPARK-1632. Remove unnecessary boxing in compa...
Posted by sryza <gi...@git.apache.org>.
Github user sryza commented on a diff in the pull request:
https://github.com/apache/spark/pull/559#discussion_r12015241
--- Diff: core/src/main/scala/org/apache/spark/util/collection/ExternalAppendOnlyMap.scala ---
@@ -422,7 +422,8 @@ class ExternalAppendOnlyMap[K, V, C](
private[spark] object ExternalAppendOnlyMap {
private class KCComparator[K, C] extends Comparator[(K, C)] {
def compare(kc1: (K, C), kc2: (K, C)): Int = {
- kc1._1.hashCode().compareTo(kc2._1.hashCode())
+ if (kc1._1.hashCode() < kc2._1.hashCode()) -1
--- End diff --
Good point
---
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.
---
[GitHub] spark pull request: SPARK-1632. Remove unnecessary boxing in compa...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/559#issuecomment-41438793
Merged build started.
---
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.
---