[SPARK-7311] Introduce internal Serializer API for determining if serializers support object relocation #5924

JoshRosen · 2015-05-05T22:00:50Z

This patch extends the Serializer interface with a new @Private API which allows serializers to indicate whether they support relocation of serialized objects in serializer stream output.

This relocatibilty property is described in more detail in Serializer.scala, but in a nutshell a serializer supports relocation if reordering the bytes of serialized objects in serialization stream output is equivalent to having re-ordered those elements prior to serializing them. The optimized shuffle path introduced in #4450 and #5868 both rely on serializers having this property; this patch just centralizes the logic for determining whether a serializer has this property. I also added tests and comments clarifying when this works for KryoSerializer.

This change allows the optimizations in #4450 to be applied for shuffles that use SqlSerializer2.

…nsafeShuffle path is used. Conflicts: core/src/main/scala/org/apache/spark/shuffle/unsafe/UnsafeShuffleManager.scala

I verified that the Kryo tests will fail if we remove the auto-reset check in KryoSerializer. I also checked that this test fails if we mistakenly enable this flag for JavaSerializer. This demonstrates that the test case is actually capable of detecting the types of bugs that it's trying to prevent. Of course, it's possible that certain bugs will only surface when serializing specific data types, so we'll still have to be cautious when overriding `supportsRelocationOfSerializedObjects` for new serializers.

This lays some groundwork for re-using this test logic for serializers defined in other subprojects (those projects can just declare a test-jar dependency on Spark core).

JoshRosen · 2015-05-05T22:02:48Z

core/src/main/scala/org/apache/spark/serializer/Serializer.scala

+   *
+   * See SPARK-7311 for more details.
+   */
+  @Private


I'm not sure that we want to commit to this as a stable public API, which is why I've chosen to mark this as private and leave comments warning users that this API is private and subject to change. If someone can think of a better way to restrict use / implementation of this method, I'd be happy to incorporate that change.

JoshRosen · 2015-05-05T22:11:13Z

/cc @rxin @sryza

JoshRosen · 2015-05-05T22:13:12Z

core/src/main/scala/org/apache/spark/serializer/Serializer.scala

+   * See SPARK-7311 for more details.
+   */
+  @Private
+  private[spark] def supportsRelocationOfSerializedObjects: Boolean = false


I'm not set on this name, by the way; happy to change if someone thinks of a less verbose name that's not misleading.

rxin · 2015-05-05T22:14:32Z

LGTM.

sryza · 2015-05-05T22:47:46Z

core/src/main/scala/org/apache/spark/serializer/Serializer.scala

@@ -23,7 +23,7 @@ import java.nio.ByteBuffer
 import scala.reflect.ClassTag

 import org.apache.spark.{SparkConf, SparkEnv}
-import org.apache.spark.annotation.DeveloperApi
+import org.apache.spark.annotation.{Private, Experimental, DeveloperApi}


nit: alphabetize

Argh, I really need to fix my IntelliJ settings. I switched versions and didn't port all of my import sorting settings over, so stuff like this keeps happening :(

sryza · 2015-05-05T22:51:10Z

LGTM other than a couple tiny nits

SparkQA · 2015-05-06T00:23:35Z

Test build #31910 has finished for PR 5924 at commit 0a7ebd7.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- public class EnumUtil
- class BinaryClassificationEvaluator(JavaEvaluator, HasLabelCol, HasRawPredictionCol):
- class HasRawPredictionCol(Params):
- class Evaluator(object):
- class JavaEvaluator(Evaluator, JavaWrapper):

JoshRosen · 2015-05-06T17:51:53Z

I've addressed those tiny nits, so I'm now going to merge this into master and branch-1.4.

…ializers support object relocation This patch extends the `Serializer` interface with a new `Private` API which allows serializers to indicate whether they support relocation of serialized objects in serializer stream output. This relocatibilty property is described in more detail in `Serializer.scala`, but in a nutshell a serializer supports relocation if reordering the bytes of serialized objects in serialization stream output is equivalent to having re-ordered those elements prior to serializing them. The optimized shuffle path introduced in #4450 and #5868 both rely on serializers having this property; this patch just centralizes the logic for determining whether a serializer has this property. I also added tests and comments clarifying when this works for KryoSerializer. This change allows the optimizations in #4450 to be applied for shuffles that use `SqlSerializer2`. Author: Josh Rosen <[email protected]> Closes #5924 from JoshRosen/SPARK-7311 and squashes the following commits: 50a68ca [Josh Rosen] Address minor nits 0a7ebd7 [Josh Rosen] Clarify reason why SqlSerializer2 supports this serializer 123b992 [Josh Rosen] Cleanup for submitting as standalone patch. 4aa61b2 [Josh Rosen] Add missing newline 2c1233a [Josh Rosen] Small refactoring of SerializerPropertiesSuite to enable test re-use: 0ba75e6 [Josh Rosen] Add tests for serializer relocation property. 450fa21 [Josh Rosen] Back out accidental log4j.properties change 86d4dcd [Josh Rosen] Flag that SparkSqlSerializer2 supports relocation b9624ee [Josh Rosen] Expand serializer API and use new function to help control when new UnsafeShuffle path is used. (cherry picked from commit 002c123) Signed-off-by: Josh Rosen <[email protected]>

SparkQA · 2015-05-06T17:59:06Z

Test build #32002 has finished for PR 5924 at commit 50a68ca.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

AmplabJenkins · 2015-05-06T17:59:07Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/32002/
Test FAILed.

JoshRosen · 2015-05-06T18:01:40Z

Whoops, looks like this caused a build break because the patch that introduced the @Private annotation was reverted. I'll push a hotfix to re-add that annotation.

…ializers support object relocation This patch extends the `Serializer` interface with a new `Private` API which allows serializers to indicate whether they support relocation of serialized objects in serializer stream output. This relocatibilty property is described in more detail in `Serializer.scala`, but in a nutshell a serializer supports relocation if reordering the bytes of serialized objects in serialization stream output is equivalent to having re-ordered those elements prior to serializing them. The optimized shuffle path introduced in apache#4450 and apache#5868 both rely on serializers having this property; this patch just centralizes the logic for determining whether a serializer has this property. I also added tests and comments clarifying when this works for KryoSerializer. This change allows the optimizations in apache#4450 to be applied for shuffles that use `SqlSerializer2`. Author: Josh Rosen <[email protected]> Closes apache#5924 from JoshRosen/SPARK-7311 and squashes the following commits: 50a68ca [Josh Rosen] Address minor nits 0a7ebd7 [Josh Rosen] Clarify reason why SqlSerializer2 supports this serializer 123b992 [Josh Rosen] Cleanup for submitting as standalone patch. 4aa61b2 [Josh Rosen] Add missing newline 2c1233a [Josh Rosen] Small refactoring of SerializerPropertiesSuite to enable test re-use: 0ba75e6 [Josh Rosen] Add tests for serializer relocation property. 450fa21 [Josh Rosen] Back out accidental log4j.properties change 86d4dcd [Josh Rosen] Flag that SparkSqlSerializer2 supports relocation b9624ee [Josh Rosen] Expand serializer API and use new function to help control when new UnsafeShuffle path is used.

JoshRosen added 8 commits May 5, 2015 13:54

Expand serializer API and use new function to help control when new U…

b9624ee

…nsafeShuffle path is used. Conflicts: core/src/main/scala/org/apache/spark/shuffle/unsafe/UnsafeShuffleManager.scala

Flag that SparkSqlSerializer2 supports relocation

86d4dcd

Back out accidental log4j.properties change

450fa21

Small refactoring of SerializerPropertiesSuite to enable test re-use:

2c1233a

This lays some groundwork for re-using this test logic for serializers defined in other subprojects (those projects can just declare a test-jar dependency on Spark core).

Add missing newline

4aa61b2

Cleanup for submitting as standalone patch.

123b992

Clarify reason why SqlSerializer2 supports this serializer

0a7ebd7

JoshRosen mentioned this pull request May 5, 2015

[SPARK-7081] Faster sort-based shuffle path using binary processing cache-aware sort #5868

Closed

JoshRosen reviewed May 5, 2015
View reviewed changes

sryza reviewed May 5, 2015
View reviewed changes

Address minor nits

50a68ca

JoshRosen changed the title ~~[SPARK-7311] Introduce internal Serializer API method for determining if serializers support object relocation~~ [SPARK-7311] Introduce internal Serializer API for determining if serializers support object relocation May 6, 2015

asfgit closed this in 002c123 May 6, 2015

JoshRosen deleted the SPARK-7311 branch May 6, 2015 17:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-7311] Introduce internal Serializer API for determining if serializers support object relocation #5924

[SPARK-7311] Introduce internal Serializer API for determining if serializers support object relocation #5924

JoshRosen commented May 5, 2015

JoshRosen May 5, 2015

JoshRosen commented May 5, 2015

JoshRosen May 5, 2015

rxin commented May 5, 2015

sryza May 5, 2015

JoshRosen May 5, 2015

sryza commented May 5, 2015

SparkQA commented May 6, 2015

JoshRosen commented May 6, 2015

SparkQA commented May 6, 2015

AmplabJenkins commented May 6, 2015

JoshRosen commented May 6, 2015

[SPARK-7311] Introduce internal Serializer API for determining if serializers support object relocation #5924

[SPARK-7311] Introduce internal Serializer API for determining if serializers support object relocation #5924

Conversation

JoshRosen commented May 5, 2015

JoshRosen May 5, 2015

Choose a reason for hiding this comment

JoshRosen commented May 5, 2015

JoshRosen May 5, 2015

Choose a reason for hiding this comment

rxin commented May 5, 2015

sryza May 5, 2015

Choose a reason for hiding this comment

JoshRosen May 5, 2015

Choose a reason for hiding this comment

sryza commented May 5, 2015

SparkQA commented May 6, 2015

JoshRosen commented May 6, 2015

SparkQA commented May 6, 2015

AmplabJenkins commented May 6, 2015

JoshRosen commented May 6, 2015