[SPARK-6152] Use shaded ASM5 to support closure cleaning of Java 8 compiled classes #9512

JoshRosen · 2015-11-05T23:45:59Z

This patch modifies Spark's closure cleaner (and a few other places) to use ASM 5, which is necessary in order to support cleaning of closures that were compiled by Java 8.

In order to avoid ASM dependency conflicts, Spark excludes ASM from all of its dependencies and uses a shaded version of ASM 4 that comes from reflectasm (see SPARK-782 and #232). This patch updates Spark to use a shaded version of ASM 5.0.4 that was published by the Apache XBean project; the POM used to create the shaded artifact can be found at https://github.com/apache/geronimo-xbean/blob/xbean-4.4/xbean-asm5-shaded/pom.xml.

http://movingfulcrum.tumblr.com/post/80826553604/asm-framework-50-the-missing-migration-guide was a useful resource while upgrading the code to use the new ASM5 opcodes.

I also added a new regression tests in the java8-tests subproject; the existing tests were insufficient to catch this bug, which only affected Scala 2.11 user code which was compiled targeting Java 8.

JoshRosen · 2015-11-05T23:46:43Z

/cc @pwendell @srowen

JoshRosen · 2015-11-06T01:08:15Z

Apparently ASM is a common source of Java 8 upgrade pain: http://product.hubspot.com/blog/upgrading-to-java-8-at-scale

SparkQA · 2015-11-06T02:19:35Z

Test build #45165 has finished for PR 9512 at commit e5e27ce.

This patch fails PySpark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):\n * final class DecisionTreeRegressor @Since(\"1.4.0\") (@Since(\"1.4.0\") override val uid: String)\n * final class GBTRegressor @Since(\"1.4.0\") (@Since(\"1.4.0\") override val uid: String)\n * class IsotonicRegression @Since(\"1.5.0\") (@Since(\"1.5.0\") override val uid: String)\n * class LinearRegression @Since(\"1.3.0\") (@Since(\"1.3.0\") override val uid: String)\n * final class RandomForestRegressor @Since(\"1.4.0\") (@Since(\"1.4.0\") override val uid: String)\n

SparkQA · 2015-11-06T02:41:07Z

Test build #45169 has finished for PR 9512 at commit c3e7f4a.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):\n * final class DecisionTreeRegressor @Since(\"1.4.0\") (@Since(\"1.4.0\") override val uid: String)\n * final class GBTRegressor @Since(\"1.4.0\") (@Since(\"1.4.0\") override val uid: String)\n * class IsotonicRegression @Since(\"1.5.0\") (@Since(\"1.5.0\") override val uid: String)\n * class LinearRegression @Since(\"1.3.0\") (@Since(\"1.3.0\") override val uid: String)\n * final class RandomForestRegressor @Since(\"1.4.0\") (@Since(\"1.4.0\") override val uid: String)\n

JoshRosen · 2015-11-06T02:44:56Z

Jenkins retest this please.

SparkQA · 2015-11-06T05:44:04Z

Test build #45190 has finished for PR 9512 at commit c3e7f4a.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

JoshRosen · 2015-11-10T02:12:28Z

I suppose it would be nice to actually run the Java 8 tests to see whether they would have caught this.

pwendell · 2015-11-10T07:10:10Z

This looks fine (i.e. LGTM). However, we could also look into actually shading asm ourselves in our published artifacts, similar to how we now shade jetty and other things. I'm fine to just use this already shaded one too though. Seems to do no harm and will allow us to work with Java 8.

JoshRosen · 2015-11-10T18:21:41Z

Jenkins, retest this please.

JoshRosen · 2015-11-10T20:51:43Z

I added a regression test to the java8-tests subproject and verified that it fails before this patch's changes.

SparkQA · 2015-11-10T20:54:13Z

Test build #45535 has finished for PR 9512 at commit c3e7f4a.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

JoshRosen · 2015-11-10T21:05:48Z

project/SparkBuild.scala

+  import BuildCommons._
+
+  lazy val settings = Seq(
+    compilerJVMVersion := "1.8"


Subtlety: this setting will break the java8 tests when running on Scala 2.10.x, which does not support the emission of Java 8 bytecode. For this reason, we need separate settings for configuring the scalc and javac target JVM versions and need to only set scalac to 1.8 when running Scala 2.11.4 or higher.

JoshRosen · 2015-11-10T22:40:50Z

@marmbrus, it turns out that our java8-tests Maven profile disables the Scala compiler plugin (presumably because it was being used to compile Java sources and was hard to configure properly). I could spend some time to make it so that the regression test for this patch can run in Maven, but I'd rather work on other things given how much time I've already sunk into this. Therefore, I just amended the "Building Spark" guide to contain the one-liner for running this in SBT. I'll defer proper configuration / execution of these tests to a followup / someone else.

marmbrus · 2015-11-10T22:43:08Z

I think the most important thing is that it will be run by the PR builder.

JoshRosen · 2015-11-10T22:44:44Z

The Java 8 tests currently aren't run at all (in fact, some of the streaming ones appear to be broken). In order to test this in the PR builder, I'll have to configure it to use JDK 8 and will have to supply the right profiles. I could probably auto-set the right profile based on the detected JDK version.

SparkQA · 2015-11-10T23:46:23Z

Test build #45546 has finished for PR 9512 at commit 38191a1.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2015-11-10T23:58:40Z

Test build #45549 has finished for PR 9512 at commit fd96788.

This patch fails PySpark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2015-11-11T03:29:54Z

Test build #45569 has finished for PR 9512 at commit 01f5cca.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):\n * public class JavaLBFGSExample\n * class SlidingRDDPartition[T](val idx: Int, val prev: Partition, val tail: Seq[T], val offset: Int)\n * class SlidingRDD[T: ClassTag](@transient val parent: RDD[T], val windowSize: Int, val step: Int)\n

JoshRosen · 2015-11-11T05:43:59Z

sql/core/src/test/scala/org/apache/spark/sql/execution/metric/SQLMetricsSuite.scala

-    } catch {
-      case _: IllegalArgumentException => None
-    }
+    Some(new ClassReader(new ByteArrayInputStream(baos.toByteArray)))


Just realized that this no longer needs to return an Option; will update now to keep the code clean.

JoshRosen · 2015-11-11T06:26:20Z

Jenkins, retest this please.

SparkQA · 2015-11-11T08:41:36Z

Test build #45598 has finished for PR 9512 at commit 9833667.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2015-11-11T09:23:34Z

Test build #45609 has finished for PR 9512 at commit 9833667.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):\n * public class JavaLBFGSExample\n

marmbrus · 2015-11-11T19:16:12Z

Thanks, I'm going to merge this to master and 1.6. We should look at turning Java 8 test on in Jenkins as a follow up.

…mpiled classes This patch modifies Spark's closure cleaner (and a few other places) to use ASM 5, which is necessary in order to support cleaning of closures that were compiled by Java 8. In order to avoid ASM dependency conflicts, Spark excludes ASM from all of its dependencies and uses a shaded version of ASM 4 that comes from `reflectasm` (see [SPARK-782](https://issues.apache.org/jira/browse/SPARK-782) and #232). This patch updates Spark to use a shaded version of ASM 5.0.4 that was published by the Apache XBean project; the POM used to create the shaded artifact can be found at https://github.com/apache/geronimo-xbean/blob/xbean-4.4/xbean-asm5-shaded/pom.xml. http://movingfulcrum.tumblr.com/post/80826553604/asm-framework-50-the-missing-migration-guide was a useful resource while upgrading the code to use the new ASM5 opcodes. I also added a new regression tests in the `java8-tests` subproject; the existing tests were insufficient to catch this bug, which only affected Scala 2.11 user code which was compiled targeting Java 8. Author: Josh Rosen <joshrosen@databricks.com> Closes #9512 from JoshRosen/SPARK-6152. (cherry picked from commit 529a1d3) Signed-off-by: Michael Armbrust <michael@databricks.com>

…mpiled classes This patch modifies Spark's closure cleaner (and a few other places) to use ASM 5, which is necessary in order to support cleaning of closures that were compiled by Java 8. In order to avoid ASM dependency conflicts, Spark excludes ASM from all of its dependencies and uses a shaded version of ASM 4 that comes from `reflectasm` (see [SPARK-782](https://issues.apache.org/jira/browse/SPARK-782) and apache#232). This patch updates Spark to use a shaded version of ASM 5.0.4 that was published by the Apache XBean project; the POM used to create the shaded artifact can be found at https://github.com/apache/geronimo-xbean/blob/xbean-4.4/xbean-asm5-shaded/pom.xml. http://movingfulcrum.tumblr.com/post/80826553604/asm-framework-50-the-missing-migration-guide was a useful resource while upgrading the code to use the new ASM5 opcodes. I also added a new regression tests in the `java8-tests` subproject; the existing tests were insufficient to catch this bug, which only affected Scala 2.11 user code which was compiled targeting Java 8. Author: Josh Rosen <joshrosen@databricks.com> Closes apache#9512 from JoshRosen/SPARK-6152.

References: http://movingfulcrum.tumblr.com/post/80826553604/asm-framework-50-the-missing-migration-guide apache/spark#9512

* WIP * Use Kryo 3.1.0-SNAPSHOT; register ClosureSerializer * Update ClosureCleaner to use ASM 5 APIs: References: http://movingfulcrum.tumblr.com/post/80826553604/asm-framework-50-the-missing-migration-guide apache/spark#9512 * Use sbt-doge to exclude certain subprojects from the 2.12 build. * Remove testOptions change. * Only register ClosureSerializer when running on Java 8 JDK. * Update to use sbt-doge's +++ "strict aggregation" operator. * Workaround to fix PriorityQueueTest (there's probably a better way to do this). * Replace Unit.box() call by scala.runtime.BoxedUnit.UNIT See https://issues.scala-lang.org/browse/SI-6710 for context. This is necessary for Scala 2.12.0-M5. * Update to 2.12.0 final. * Undo BoxedUnit workaround since it's no longer necessary. * Create Java8ClosureRegistrar * Fix Java8ClosureRegistrar; standardize formatting.

Use shaded ASM5 for closure cleaning.

e5e27ce

Do not call deprecated method

c3e7f4a

JoshRosen added 2 commits November 10, 2015 12:37

Add ability to configure JDK versions in test code compliation.

59d649b

Add regression test.

38191a1

JoshRosen reviewed Nov 10, 2015
View reviewed changes

JoshRosen added 2 commits November 10, 2015 13:17

Use separate settings for scalac and javac JVM versions.

fd96788

Add instructions on running Java 8 tests in Scala.

002d1ea

Merge remote-tracking branch 'origin/master' into SPARK-6152

01f5cca

JoshRosen reviewed Nov 11, 2015
View reviewed changes

Remove unnecessary Option usage.

9833667

asfgit closed this in 529a1d3 Nov 11, 2015

JoshRosen mentioned this pull request Apr 8, 2016

Build and publish against current Scala 2.12 milestone twitter/chill#252

Closed

JoshRosen added a commit to JoshRosen/chill that referenced this pull request Apr 8, 2016

Update ClosureCleaner to use ASM 5 APIs:

c98f80e

References: http://movingfulcrum.tumblr.com/post/80826553604/asm-framework-50-the-missing-migration-guide apache/spark#9512

DieBauer mentioned this pull request Apr 9, 2017

[FLINK-5005] WIP: publish scala 2.12 artifacts apache/flink#3703

Closed

3 tasks

gatorsmile mentioned this pull request Apr 10, 2017

[SPARK-20264][SQL] asm should be non-test dependency in sql/core #17574

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-6152] Use shaded ASM5 to support closure cleaning of Java 8 compiled classes #9512

[SPARK-6152] Use shaded ASM5 to support closure cleaning of Java 8 compiled classes #9512

JoshRosen commented Nov 5, 2015

JoshRosen commented Nov 5, 2015

JoshRosen commented Nov 6, 2015

SparkQA commented Nov 6, 2015

SparkQA commented Nov 6, 2015

JoshRosen commented Nov 6, 2015

SparkQA commented Nov 6, 2015

JoshRosen commented Nov 10, 2015

pwendell commented Nov 10, 2015

JoshRosen commented Nov 10, 2015

JoshRosen commented Nov 10, 2015

SparkQA commented Nov 10, 2015

JoshRosen Nov 10, 2015

JoshRosen commented Nov 10, 2015

marmbrus commented Nov 10, 2015

JoshRosen commented Nov 10, 2015

SparkQA commented Nov 10, 2015

SparkQA commented Nov 10, 2015

SparkQA commented Nov 11, 2015

JoshRosen Nov 11, 2015

JoshRosen commented Nov 11, 2015

SparkQA commented Nov 11, 2015

SparkQA commented Nov 11, 2015

marmbrus commented Nov 11, 2015

[SPARK-6152] Use shaded ASM5 to support closure cleaning of Java 8 compiled classes #9512

[SPARK-6152] Use shaded ASM5 to support closure cleaning of Java 8 compiled classes #9512

Conversation

JoshRosen commented Nov 5, 2015

JoshRosen commented Nov 5, 2015

JoshRosen commented Nov 6, 2015

SparkQA commented Nov 6, 2015

SparkQA commented Nov 6, 2015

JoshRosen commented Nov 6, 2015

SparkQA commented Nov 6, 2015

JoshRosen commented Nov 10, 2015

pwendell commented Nov 10, 2015

JoshRosen commented Nov 10, 2015

JoshRosen commented Nov 10, 2015

SparkQA commented Nov 10, 2015

JoshRosen Nov 10, 2015

Choose a reason for hiding this comment

JoshRosen commented Nov 10, 2015

marmbrus commented Nov 10, 2015

JoshRosen commented Nov 10, 2015

SparkQA commented Nov 10, 2015

SparkQA commented Nov 10, 2015

SparkQA commented Nov 11, 2015

JoshRosen Nov 11, 2015

Choose a reason for hiding this comment

JoshRosen commented Nov 11, 2015

SparkQA commented Nov 11, 2015

SparkQA commented Nov 11, 2015

marmbrus commented Nov 11, 2015