You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by mccheah <gi...@git.apache.org> on 2015/03/11 02:26:14 UTC
[GitHub] spark pull request: [SPARK-6269] Use a different implementation of...
GitHub user mccheah opened a pull request:
https://github.com/apache/spark/pull/4972
[SPARK-6269] Use a different implementation of java.lang.reflect.Array
This patch uses a different implementation of java.lang.reflect.Array. The code is copied and pasted from
https://bugs.openjdk.java.net/browse/JDK-8051447 with appropriate style changes for this project. The appropriate code is in the public domain, so we can use it since the proper Apache licensing is on it.
The idea is to use pure Java code in implementing the methods there, as opposed to relying on native C code which ends up being ill-performing. This improves the performance of estimating the size of arrays when we are checking for spilling in Spark.
Here's the benchmark discussion from the ticket:
I did two tests. The first, less convincing, take-with-a-block-of-salt test I did was do a simple groupByKey operation to collect objects in a 4.0 GB text file RDD into 30,000 buckets. I ran 1 Master and 4 Spark Worker JVMs on my mac, fetching the RDD from a text file simply stored on disk, and saving it out to another file located on local disk. The wall clock times I got back before and after the change were:
Before: 352.195s, 343.871s, 359.080s
After: 342.929583s, 329.456623s, 326.151481s
So, there is a bit of an improvement after the change. I also did some YourKit profiling of the executors to get an idea of how much time was spent in size estimation before and after the change. I roughly saw that size estimation took up less of the time after my change, but YourKit's profiling can be inconsistent and who knows if I was profiling the executors that had the same data between runs?
The more convincing test I did was to run the size-estimation logic itself in an isolated unit test. I ran the following code:
```
{code}
val bigArray = Array.fill(1000)(Array.fill(1000)(java.util.UUID.randomUUID().toString()))
test("String arrays only perf testing") {
val startTime = System.currentTimeMillis()
for (i <- 1 to 50000) {
SizeEstimator.estimate(bigArray)
}
println("Runtime: " + (System.currentTimeMillis() - startTime) / 1000.0000)
}
{code}
```
I wanted to use a 2D array specifically because I wanted to measure the performance of repeatedly calling Array.getLength. I used UUID-Strings to ensure that the strings were randomized (so object re-use doesn't happen), but that they would all be the same size. The results were as follows:
Before change: 209.275s, 190.107s, 185.424s
After change: 160.431s, 149.487s, 151.66s
.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/palantir/spark feature/spark-6269-reflect-array
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/4972.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 #4972
----
----
---
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: [SPARK-6269] Use a different implementation of...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/4972#issuecomment-78511122
I think maybe @rxin is suggesting a bit of code like what you see in `ScalaRunTime.array_length`, for example?
```
def array_length(xs: AnyRef): Int = xs match {
case x: Array[AnyRef] => x.length
case x: Array[Int] => x.length
case x: Array[Double] => x.length
case x: Array[Long] => x.length
case x: Array[Float] => x.length
case x: Array[Char] => x.length
case x: Array[Byte] => x.length
case x: Array[Short] => x.length
case x: Array[Boolean] => x.length
case x: Array[Unit] => x.length
case null => throw new NullPointerException
}
```
---
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: [SPARK-6269] Use a different implementation of...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/4972#issuecomment-78191541
[Test build #28455 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/28455/consoleFull) for PR 4972 at commit [`ca063fc`](https://github.com/apache/spark/commit/ca063fc6d6f932483791e4ed311a099e05646231).
* This patch **passes all tests**.
* This patch merges cleanly.
* This patch adds the following public classes _(experimental)_:
* `public class ArrayReflect `
---
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: [SPARK-6269] Use a different implementation of...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/4972#issuecomment-78789639
[Test build #28545 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/28545/consoleFull) for PR 4972 at commit [`db890ea`](https://github.com/apache/spark/commit/db890ea8f7899022d9c02dcc4a32c2fce9f13c47).
* 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: [SPARK-6269] Use a different implementation of...
Posted by mccheah <gi...@git.apache.org>.
Github user mccheah commented on the pull request:
https://github.com/apache/spark/pull/4972#issuecomment-78410128
Maybe I'm reading too much into this... but Scala arrays are always of the "boxed" type. Does that create issues if I create a Java array of an un-boxed type, i.e. primitive type, and try to check arr.isInstanceOf there? Or, does the fact that the array object itself already live in scala-land mean that it's always going to be array of boxed type?
---
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: [SPARK-6269] Use a different implementation of...
Posted by mccheah <gi...@git.apache.org>.
Github user mccheah commented on the pull request:
https://github.com/apache/spark/pull/4972#issuecomment-78409723
I wanted to avoid Scala because we're working with checking instanceOf Java array of primitive type, which is slightly scary to me. Do you know the behavior of converting Scala arrays and Java arrays of primitive types?
---
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: [SPARK-6269] [CORE] Use ScalaRunTime's array m...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/4972#issuecomment-81882143
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/28662/
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: [SPARK-6269] Use a different implementation of...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/4972#issuecomment-78625350
I haven't thought it through at close range but I had meant something like ...
```
def sizeOf(xs: AnyRef): Int = xs match {
case x: Array[Int] => x.length * 4
case x: Array[Double] => x.length * 8
case x: Array[Long] => x.length * 8
...
}
```
... but if the point is merely to avoid native code, how about just using the 'equivalent' of the `Arrays` methods from Scala? they look like non-native code and do about the same thing as the original optimized version you pasted.
I think you're taking it a step further and trying to assess the component type just once, which is probably a further win but enough to matter and be worth the complexity?
Hm, I don't think `@specialized` can be used here, but I wish it could.
---
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: [SPARK-6269] Use a different implementation of...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/4972#discussion_r26442700
--- Diff: core/src/main/scala/org/apache/spark/util/SizeEstimator.scala ---
@@ -213,7 +211,7 @@ private[spark] object SizeEstimator extends Logging {
index = rand.nextInt(length)
--- End diff --
the for loop above here too
---
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: [SPARK-6269] Use a different implementation of...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/4972#issuecomment-78764134
[Test build #28545 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/28545/consoleFull) for PR 4972 at commit [`db890ea`](https://github.com/apache/spark/commit/db890ea8f7899022d9c02dcc4a32c2fce9f13c47).
* This patch merges cleanly.
---
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: [SPARK-6269] Use a different implementation of...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/4972#issuecomment-78183787
[Test build #28454 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/28454/consoleFull) for PR 4972 at commit [`1fe09de`](https://github.com/apache/spark/commit/1fe09de24d48cdfbff995a7ef1f82d72ff521509).
* This patch merges cleanly.
---
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: [SPARK-6269] Use a different implementation of...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/4972#issuecomment-78420761
[Test build #28497 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/28497/consoleFull) for PR 4972 at commit [`93f4b05`](https://github.com/apache/spark/commit/93f4b054cd70657a23139c2568568d8df90c4329).
* This patch merges cleanly.
---
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: [SPARK-6269] Use a different implementation of...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/4972#issuecomment-78183941
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/28454/
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 pull request: [SPARK-6269] Use a different implementation of...
Posted by mccheah <gi...@git.apache.org>.
Github user mccheah commented on the pull request:
https://github.com/apache/spark/pull/4972#issuecomment-78514801
@rxin can you confirm? However I believe we wanted to only do the cast once and re-use the casted object on all future invocations of array.get(). That being said my convoluted if statement in CastedArray.castAndWrap can be cleaned up with similar Scala code to ScalaRuntime.array_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 pull request: [SPARK-6269] Use a different implementation of...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the pull request:
https://github.com/apache/spark/pull/4972#issuecomment-78412866
Yea - Scala arrays are not always boxed. Array[Int] is the same as int[] in Java.
---
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: [SPARK-6269] Use a different implementation of...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/4972#issuecomment-79648862
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/28599/
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: [SPARK-6269] Use a different implementation of...
Posted by mccheah <gi...@git.apache.org>.
Github user mccheah commented on the pull request:
https://github.com/apache/spark/pull/4972#issuecomment-78420895
I wrote it in scala, extending AnyVal and writing a sealed trait that extends Any to provide the extension point. However, I believe that using the trait incurs the allocation overhead, which is what we were trying to avoid in moving to Scala. From the scala docs
"A value class can only extend universal traits and cannot be extended itself. A universal trait is a trait that extends Any, only has defs as members, and does no initialization. Universal traits allow basic inheritance of methods for value classes, but they incur the overhead of allocation."
http://docs.scala-lang.org/overviews/core/value-classes.html
That being said, I also didn't see any significant performance degradation in moving to Scala. My most recent numbers:
Estimating the size of a 1000 x 1000 2D String array 50,000 times: 153.329 s, 153.1 s, 150.67 s
Estimating the size of a 1000 1D String array 1,000,000 times: 19.951 s, 20.75 s, 20.551 s
So we didn't lose any performance from moving from Java to Scala (compare with numbers I've reported above in this discussion), but we gained a ton of readability. It's odd to be using Java in this Scala-laden codebase anyways, so it seems good to keep our things bound to Scala.
---
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: [SPARK-6269] Use a different implementation of...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/4972#issuecomment-78257987
The micro-benchmark definitely shows the alternative code is faster, although it only benchmarks the time taken in this one method. The question is how much of the overall time is consumed by a simple array length check. Your first test is more convincing, although the difference seems quite a bit larger than I'd expect. Wow, does it really get called so much / the native overhead is that big?
---
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: [SPARK-6269] Use a different implementation of...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/4972#discussion_r26208974
--- Diff: core/src/main/scala/org/apache/spark/util/ArrayReflect.java ---
@@ -0,0 +1,357 @@
+/*
--- End diff --
Given its provenance I'm not sure we can say it's licensed to the ASF, but can state its source and apparent license here.
---
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: [SPARK-6269] Use a different implementation of...
Posted by mccheah <gi...@git.apache.org>.
Github user mccheah commented on the pull request:
https://github.com/apache/spark/pull/4972#issuecomment-79549849
I believe the pull request message and the commit message are out of date, because of the way this thread went.
---
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: [SPARK-6269] Use a different implementation of...
Posted by mccheah <gi...@git.apache.org>.
Github user mccheah commented on the pull request:
https://github.com/apache/spark/pull/4972#issuecomment-78407842
I changed the implementation to not use the code from the ticket, and instead created a wrapper around arrays, and a factory method that did the appropriate casting and building of the wrapper object.
I was mildly concerned about the performance of this implementation, as now I'm instantiating a new object as opposed to before when I was purely invoking static methods. I ran some more localized unit test benchmarks.
With the 1000 x 1000 2D String array tests I was doing before, the performance difference seems to be better, but I might as well attribute the difference to noise since the difference is so small:
Before this PR at all: 209.275 s, 190.107 s, 185.424 s
Before most recent commit: 160.431, 149.487, 151.66
After most recent commit: 151.812, 151.505, 142.953
I was mostly concerned about the overhead to create the object if the arrays were small, though, because now I'm spending more time on creating the wrapper itself. It turns out we're still doing better even on small arrays compared to before the PR however. I tested estimating the size of a 1000-String 1D array:
Before PR: 27.246, 32.124, 28.433
After commit: 20.036, 20.465, 21.38
Most importantly however we should be able to get around the licensing issues now. I also made the code only use the array get() and getLength() methods since we never set() on arrays anywhere.
---
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: [SPARK-6269] Use a different implementation of...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/4972#issuecomment-78184695
[Test build #28455 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/28455/consoleFull) for PR 4972 at commit [`ca063fc`](https://github.com/apache/spark/commit/ca063fc6d6f932483791e4ed311a099e05646231).
* This patch merges cleanly.
---
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: [SPARK-6269] Use a different implementation of...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/4972#discussion_r26387439
--- Diff: core/src/main/scala/org/apache/spark/util/SizeEstimator.scala ---
@@ -184,14 +182,14 @@ private[spark] object SizeEstimator extends Logging {
private val ARRAY_SIZE_FOR_SAMPLING = 200
private val ARRAY_SAMPLE_SIZE = 100 // should be lower than ARRAY_SIZE_FOR_SAMPLING
- private def visitArray(array: AnyRef, cls: Class[_], state: SearchState) {
- val length = JArray.getLength(array)
- val elementClass = cls.getComponentType
+ private def visitArray(array: AnyRef, arrayClass: Class[_], state: SearchState) {
+ val length = ScalaRunTime.array_length(array)
+ val elementClass = arrayClass.getComponentType()
--- End diff --
I think it's OK to add () since they're java methods and are technically declared with (). I think they have no side effects though. More of an observation than anything.
---
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: [SPARK-6269] Use a different implementation of...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/4972#issuecomment-78544903
[Test build #28524 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/28524/consoleFull) for PR 4972 at commit [`cb67ce2`](https://github.com/apache/spark/commit/cb67ce2774df4919b36cf8756fa6ae4a16462482).
* This patch merges cleanly.
---
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: [SPARK-6269] Use a different implementation of...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/4972#issuecomment-78985214
Great, so, this still gets a pretty good speed-up? sounds like a win to me. I'll leave it for a day or so for other thoughts, but it's pretty clean.
---
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: [SPARK-6269] Use a different implementation of...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/4972#discussion_r26387372
--- Diff: core/src/main/scala/org/apache/spark/util/SizeEstimator.scala ---
@@ -18,18 +18,16 @@
package org.apache.spark.util
import java.lang.management.ManagementFactory
-import java.lang.reflect.{Array => JArray}
-import java.lang.reflect.Field
-import java.lang.reflect.Modifier
-import java.util.IdentityHashMap
-import java.util.Random
+import java.lang.reflect.{Field, Modifier}
+import java.util.{IdentityHashMap, Random}
import java.util.concurrent.ConcurrentHashMap
-import scala.collection.mutable.ArrayBuffer
-
import org.apache.spark.Logging
import org.apache.spark.util.collection.OpenHashSet
+import scala.collection.mutable.ArrayBuffer
--- End diff --
Nit: this should go just after java imports
---
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: [SPARK-6269] Use a different implementation of...
Posted by mccheah <gi...@git.apache.org>.
Github user mccheah commented on a diff in the pull request:
https://github.com/apache/spark/pull/4972#discussion_r26508937
--- Diff: core/src/main/scala/org/apache/spark/util/SizeEstimator.scala ---
@@ -184,14 +182,14 @@ private[spark] object SizeEstimator extends Logging {
private val ARRAY_SIZE_FOR_SAMPLING = 200
private val ARRAY_SAMPLE_SIZE = 100 // should be lower than ARRAY_SIZE_FOR_SAMPLING
- private def visitArray(array: AnyRef, cls: Class[_], state: SearchState) {
- val length = JArray.getLength(array)
- val elementClass = cls.getComponentType
+ private def visitArray(array: AnyRef, arrayClass: Class[_], state: SearchState) {
+ val length = ScalaRunTime.array_length(array)
+ val elementClass = arrayClass.getComponentType()
// Arrays have object header and length field which is an integer
var arrSize: Long = alignSize(objectSize + INT_SIZE)
- if (elementClass.isPrimitive) {
+ if (elementClass.isPrimitive()) {
--- End diff --
isPrimitive is a method on Java's class. I don't really know what the convention is there. I think this was a by-product of me manually reverting things so I won't change it.
---
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: [SPARK-6269] Use a different implementation of...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the pull request:
https://github.com/apache/spark/pull/4972#issuecomment-78409991
Scala arrays are just Java arrays. There is no craziness there.
---
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: [SPARK-6269] Use a different implementation of...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/4972#issuecomment-78425912
[Test build #28497 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/28497/consoleFull) for PR 4972 at commit [`93f4b05`](https://github.com/apache/spark/commit/93f4b054cd70657a23139c2568568d8df90c4329).
* This patch **passes all tests**.
* This patch merges cleanly.
* This patch adds the following public classes _(experimental)_:
* `sealed trait CastedArray extends Any `
---
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: [SPARK-6269] Use a different implementation of...
Posted by mccheah <gi...@git.apache.org>.
Github user mccheah commented on a diff in the pull request:
https://github.com/apache/spark/pull/4972#discussion_r26265216
--- Diff: core/src/main/scala/org/apache/spark/util/ArrayReflect.java ---
@@ -0,0 +1,357 @@
+/*
--- End diff --
Hm, yes we could. Scala closures can help us here... I'll whip up something.
---
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: [SPARK-6269] Use a different implementation of...
Posted by mccheah <gi...@git.apache.org>.
Github user mccheah commented on the pull request:
https://github.com/apache/spark/pull/4972#issuecomment-78764036
Latest just uses ScalaRunTime which also doesn't use the native java.lang.reflect.Array. The underlying implementation is basically identical to the first Java implementation that I wrote on here, except I need to do an extra "asInstanceOf[AnyRef]" to convert from Any (which is what ScalaRunTime.array_apply returns) to AnyRef (which is what we want in SizeEstimator).
---
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: [SPARK-6269] Use a different implementation of...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the pull request:
https://github.com/apache/spark/pull/4972#issuecomment-78409663
If you write this in Scala, you can use value classes to avoid the allocation, while preserving the OOP features.
---
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: [SPARK-6269] Use a different implementation of...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the pull request:
https://github.com/apache/spark/pull/4972#issuecomment-80755993
Other than the two minor things I pointed out, and the need to update title/pr description, this change lgtm. 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 pull request: [SPARK-6269] Use a different implementation of...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/4972#issuecomment-79648761
[Test build #28599 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/28599/consoleFull) for PR 4972 at commit [`16ce534`](https://github.com/apache/spark/commit/16ce534b04dba0816eb93b92ebb2448b2f701916).
* 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: [SPARK-6269] Use a different implementation of...
Posted by mccheah <gi...@git.apache.org>.
Github user mccheah commented on the pull request:
https://github.com/apache/spark/pull/4972#issuecomment-78716036
Hm, yeah I'm not too opinionated about saving the tiniest of fractions of seconds by avoiding calling instanceOf too many times - instanceOf is really really cheap compared to java.lang.reflect.Array as it is. If the majority opinion is that I made it too complex we can move to just using Scala's Arrays methods.
(I don't have that much time to work on this any more until after the weekend though.)
---
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: [SPARK-6269] Use a different implementation of...
Posted by mccheah <gi...@git.apache.org>.
Github user mccheah commented on the pull request:
https://github.com/apache/spark/pull/4972#issuecomment-78184492
I believe we use Array.get in the visitArray method in SizeEstimator, so there's that as well.
---
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: [SPARK-6269] Use a different implementation of...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/4972#issuecomment-78425921
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/28497/
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: [SPARK-6269] Use a different implementation of...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/4972#issuecomment-78572799
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/28524/
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: [SPARK-6269] Use a different implementation of...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/4972#issuecomment-78541923
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/28521/
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 pull request: [SPARK-6269] Use a different implementation of...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/4972#issuecomment-78541914
[Test build #28521 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/28521/consoleFull) for PR 4972 at commit [`5d53c4c`](https://github.com/apache/spark/commit/5d53c4cd8f89e8aeec98ddfaa6277e69b56d9b18).
* This patch **fails Scala style tests**.
* This patch merges cleanly.
* This patch adds the following public classes _(experimental)_:
* `sealed trait CastedArray extends Any `
---
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: [SPARK-6269] Use a different implementation of...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/4972#discussion_r26442697
--- Diff: core/src/main/scala/org/apache/spark/util/SizeEstimator.scala ---
@@ -200,7 +198,7 @@ private[spark] object SizeEstimator extends Logging {
if (length <= ARRAY_SIZE_FOR_SAMPLING) {
for (i <- 0 until length) {
--- End diff --
while you are doing this, do you mind change the for loop into a while loop? i.e.
```scala
var i = 0
while (i < length) {
...
i += 1
}
```
---
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: [SPARK-6269] Use a different implementation of...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the pull request:
https://github.com/apache/spark/pull/4972#issuecomment-78184317
@mccheah thanks for submitting this. Is the only thing we use getLength?
---
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: [SPARK-6269] Use a different implementation of...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/4972#issuecomment-80674937
@mccheah yeah are you able to go back and revise the PR title, JIRA title, and description? I can touch some of that up on merge but just boiling it down to a quick and up to date summary would help me.
---
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: [SPARK-6269] Use a different implementation of...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/4972#issuecomment-78407461
[Test build #28489 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/28489/consoleFull) for PR 4972 at commit [`a557ab8`](https://github.com/apache/spark/commit/a557ab8512e0703ae33d0a144d7912fcff0deaf4).
* This patch merges cleanly.
---
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: [SPARK-6269] Use ScalaRunTime's array methods ...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/4972#issuecomment-81848626
Nah your commit messages don't matter since those get squashed. You updated the right things. LGTM. I'll merge after a pause for any more comments.
---
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: [SPARK-6269] Use ScalaRunTime's array methods ...
Posted by mccheah <gi...@git.apache.org>.
Github user mccheah commented on the pull request:
https://github.com/apache/spark/pull/4972#issuecomment-81846808
I just updated the PR description and title as well. Not sure if it's necessary to amend the commit messages?
---
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: [SPARK-6269] Use a different implementation of...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/4972#discussion_r26442690
--- Diff: core/src/main/scala/org/apache/spark/util/SizeEstimator.scala ---
@@ -184,14 +182,14 @@ private[spark] object SizeEstimator extends Logging {
private val ARRAY_SIZE_FOR_SAMPLING = 200
private val ARRAY_SAMPLE_SIZE = 100 // should be lower than ARRAY_SIZE_FOR_SAMPLING
- private def visitArray(array: AnyRef, cls: Class[_], state: SearchState) {
- val length = JArray.getLength(array)
- val elementClass = cls.getComponentType
+ private def visitArray(array: AnyRef, arrayClass: Class[_], state: SearchState) {
+ val length = ScalaRunTime.array_length(array)
+ val elementClass = arrayClass.getComponentType()
// Arrays have object header and length field which is an integer
var arrSize: Long = alignSize(objectSize + INT_SIZE)
- if (elementClass.isPrimitive) {
+ if (elementClass.isPrimitive()) {
--- End diff --
is isPrimitive defined with parentheses? If not, let's not change it here.
---
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: [SPARK-6269] Use a different implementation of...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/4972#issuecomment-81843236
[Test build #28662 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/28662/consoleFull) for PR 4972 at commit [`8527852`](https://github.com/apache/spark/commit/8527852ef1eba96298a5809ad192b32ed6f08217).
* This patch merges cleanly.
---
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: [SPARK-6269] Use a different implementation of...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/4972#issuecomment-78541562
[Test build #28521 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/28521/consoleFull) for PR 4972 at commit [`5d53c4c`](https://github.com/apache/spark/commit/5d53c4cd8f89e8aeec98ddfaa6277e69b56d9b18).
* This patch merges cleanly.
---
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: [SPARK-6269] Use a different implementation of...
Posted by mccheah <gi...@git.apache.org>.
Github user mccheah commented on a diff in the pull request:
https://github.com/apache/spark/pull/4972#discussion_r26277496
--- Diff: core/src/main/scala/org/apache/spark/util/CastedArray.scala ---
@@ -0,0 +1,126 @@
+/*
+* Licensed to the Apache Software Foundation (ASF) under one or more
+* contributor license agreements. See the NOTICE file distributed with
+* this work for additional information regarding copyright ownership.
+* The ASF licenses this file to You under the Apache License, Version 2.0
+* (the "License"); you may not use this file except in compliance with
+* the License. You may obtain a copy of the License at
+*
+* http://www.apache.org/licenses/LICENSE-2.0
+*
+* Unless required by applicable law or agreed to in writing, software
+* distributed under the License is distributed on an "AS IS" BASIS,
+* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+* See the License for the specific language governing permissions and
+* limitations under the License.
+*/
+
+package org.apache.spark.util
+
+/**
+ * Provides a wrapper around an object that is known to be an array, but the specific
+ * type for the array is unknown.
+ *
+ * Normally, in situations when such an array is to be accessed reflectively, one would use
+ * {@link java.lang.reflect.Array} using getLength() and get() methods. However, it turns
+ * out that such methods are ill-performant.
+ *
+ * It turns out it is better to just use instanceOf and lots of casting over calling through
+ * to the native C implementation. There is some discussion and a sample code snippet in
+ * <a href=https://bugs.openjdk.java.net/browse/JDK-8051447>an open JDK ticket</a>. In this
+ * class, that approach is implemented in an alternative way: creating a wrapper object to
+ * wrap the array allows the cast to be done once, so the overhead of casting multiple times
+ * is also avoided. It turns out we invoke the get() method to get the value of the array
+ * numerous times, so doing the cast just once is worth the cost of constructing the wrapper
+ * object for larger arrays.
+ */
+sealed trait CastedArray extends Any {
+ def get(i: Int): AnyRef
+ def getLength(): Int
+}
+
+object CastedArray {
+ def castAndWrap(arr: AnyRef): CastedArray = {
+ if (arr.isInstanceOf[Array[Boolean]]) {
+ return new BooleanCastedArray(arr.asInstanceOf[Array[Boolean]])
+ } else if (arr.isInstanceOf[Array[Byte]]) {
+ return new ByteCastedArray(arr.asInstanceOf[Array[Byte]])
+ } else if (arr.isInstanceOf[Array[Char]]) {
+ return new CharCastedArray(arr.asInstanceOf[Array[Char]])
+ } else if (arr.isInstanceOf[Array[Double]]) {
+ return new DoubleCastedArray(arr.asInstanceOf[Array[Double]])
+ } else if (arr.isInstanceOf[Array[Float]]) {
+ return new FloatCastedArray(arr.asInstanceOf[Array[Float]])
+ } else if (arr.isInstanceOf[Array[Int]]) {
+ return new IntCastedArray(arr.asInstanceOf[Array[Int]])
+ } else if (arr.isInstanceOf[Array[Long]]) {
+ return new LongCastedArray(arr.asInstanceOf[Array[Long]])
+ } else if (arr.isInstanceOf[Array[Object]]) {
+ return new ObjectCastedArray(arr.asInstanceOf[Array[Object]])
+ } else if (arr.isInstanceOf[Array[Short]]) {
+ return new ShortCastedArray(arr.asInstanceOf[Array[Short]])
+ } else {
+ throw createBadArrayException(arr)
+ }
+ }
+
+ // Boxing is not ideal, but we want to return AnyRef here. An alternative implementation
+ // that used Java wouldn't force explicitly boxing... but returning Object there would
+ // make the boxing happen implicitly anyways. In practice this tends to be okay
+ // in terms of performance.
+ private class BooleanCastedArray(val arr: Array[Boolean]) extends AnyVal with CastedArray {
+ override def get(i: Int): AnyRef = Boolean.box(arr(i))
--- End diff --
Part of the reason why boxing is okay is because SizeEstimator doesn't actually use these on primitive arrays. If it gets a primitive array, size estimation just multiplies the size of the array by the size of the primitive type. So we don't ever invoke get() in those situations.
---
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: [SPARK-6269] Use a different implementation of...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/4972#issuecomment-78413624
[Test build #28489 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/28489/consoleFull) for PR 4972 at commit [`a557ab8`](https://github.com/apache/spark/commit/a557ab8512e0703ae33d0a144d7912fcff0deaf4).
* This patch **passes all tests**.
* This patch merges cleanly.
* This patch adds the following public classes _(experimental)_:
* `public abstract class ReflectArrayGetter `
---
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: [SPARK-6269] Use a different implementation of...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/4972#issuecomment-78191546
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/28455/
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: [SPARK-6269] Use a different implementation of...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/4972#issuecomment-78789661
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/28545/
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: [SPARK-6269] Use a different implementation of...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/4972#issuecomment-79550839
[Test build #28599 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/28599/consoleFull) for PR 4972 at commit [`16ce534`](https://github.com/apache/spark/commit/16ce534b04dba0816eb93b92ebb2448b2f701916).
* This patch merges cleanly.
---
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: [SPARK-6269] Use a different implementation of...
Posted by mccheah <gi...@git.apache.org>.
Github user mccheah commented on the pull request:
https://github.com/apache/spark/pull/4972#issuecomment-78321866
I noticed that a good chunk of the time spent in the size estimation method was in Array.getLength; @justinuang (who originally proposed this idea - got to give credit where it's due!) also observed this and originally tipped me off to it.
---
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: [SPARK-6269] Use a different implementation of...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/4972#issuecomment-78183939
[Test build #28454 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/28454/consoleFull) for PR 4972 at commit [`1fe09de`](https://github.com/apache/spark/commit/1fe09de24d48cdfbff995a7ef1f82d72ff521509).
* This patch **fails to build**.
* This patch merges cleanly.
* This patch adds the following public classes _(experimental)_:
* `public class ArrayReflect `
---
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: [SPARK-6269] Use a different implementation of...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/4972#discussion_r26208935
--- Diff: core/src/main/scala/org/apache/spark/util/ArrayReflect.java ---
@@ -0,0 +1,357 @@
+/*
+* Licensed to the Apache Software Foundation (ASF) under one or more
+* contributor license agreements. See the NOTICE file distributed with
+* this work for additional information regarding copyright ownership.
+* The ASF licenses this file to You under the Apache License, Version 2.0
+* (the "License"); you may not use this file except in compliance with
+* the License. You may obtain a copy of the License at
+*
+* http://www.apache.org/licenses/LICENSE-2.0
+*
+* Unless required by applicable law or agreed to in writing, software
+* distributed under the License is distributed on an "AS IS" BASIS,
+* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+* See the License for the specific language governing permissions and
+* limitations under the License.
+*/
+
+package org.apache.spark.util;
+
+/**
+ * Alternative implementation of {@link java.lang.reflect.Array} that is more performant
+ * over calling to native C in many cases. Based on code from
+ * <a href=https://bugs.openjdk.java.net/browse/JDK-8051447>an open JDK ticket</a>.
+ */
+public class ArrayReflect {
--- End diff --
We only use `getLength` and `get`. In the name of minimizing the chunk of code that needs to be added, I suppose we can just implement those two methods?
---
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: [SPARK-6269] [CORE] Use ScalaRunTime's array m...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/4972
---
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: [SPARK-6269] Use a different implementation of...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/4972#issuecomment-78413627
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/28489/
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: [SPARK-6269] Use a different implementation of...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/4972#issuecomment-78572760
[Test build #28524 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/28524/consoleFull) for PR 4972 at commit [`cb67ce2`](https://github.com/apache/spark/commit/cb67ce2774df4919b36cf8756fa6ae4a16462482).
* This patch **passes all tests**.
* This patch merges cleanly.
* This patch adds the following public classes _(experimental)_:
* `sealed trait CastedArray extends Any `
---
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: [SPARK-6269] [CORE] Use ScalaRunTime's array m...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/4972#issuecomment-81882122
[Test build #28662 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/28662/consoleFull) for PR 4972 at commit [`8527852`](https://github.com/apache/spark/commit/8527852ef1eba96298a5809ad192b32ed6f08217).
* 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