[SPARK-22328][Core] ClosureCleaner should not miss referenced superclass fields #19556

viirya · 2017-10-23T07:15:00Z

What changes were proposed in this pull request?

When the given closure uses some fields defined in super class, ClosureCleaner can't figure them and don't set it properly. Those fields will be in null values.

How was this patch tested?

Added test.

SparkQA · 2017-10-23T09:45:33Z

Test build #82971 has finished for PR 19556 at commit 29c5d73.

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

SparkQA · 2017-10-23T11:52:57Z

Test build #82973 has finished for PR 19556 at commit 6606910.

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

SparkQA · 2017-10-23T15:01:45Z

Test build #82975 has finished for PR 19556 at commit 5ac7540.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
test(\"SPARK-22328: ClosureCleaner misses referenced superclass fields: case 1\")
test(\"SPARK-22328: ClosureCleaner misses referenced superclass fields: case 2\")
abstract class TestAbstractClass2 extends Serializable

SparkQA · 2017-10-23T16:53:32Z

Test build #82987 has finished for PR 19556 at commit da747ca.

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

srowen · 2017-10-23T18:02:10Z

core/src/main/scala/org/apache/spark/util/ClosureCleaner.scala

+    setAccessedFields(outerClass, clone, obj, accessedFields)
+
+    var superClass = outerClass.getSuperclass()
+    while (superClass != null) {


Can this be something more like ...

var currentClass = outerClass do { setAccessedFields(currentClass, clone, obj, accessedFields) currentClass = currentClass.getSuperclass() } while (currentClass != null)

Just avoids repeating the key method call here. Same above and below.

Thanks. Looks good.

viirya · 2017-10-24T00:35:35Z

cc @cloud-fan for review too.

SparkQA · 2017-10-24T03:42:25Z

Test build #82999 has finished for PR 19556 at commit 5d7efd1.

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

srowen

I don't feel very expert on the ClosureCleaner, but have been looking at it a lot recently, and this looks reasonable to me at a glance.

cloud-fan · 2017-10-24T11:50:52Z

core/src/main/scala/org/apache/spark/util/ClosureCleaner.scala

+  /** Initializes the accessed fields for outer classes and their super classes. */
+  private def initAccessedFields(
+      accessedFields: Map[Class[_], Set[String]],
+      outerClasses: Seq[Class[_]]): Unit = {


what if multiple outer classes have the same parent class?

I think from the view of closure, even multiple outer classes have the same parent class, the access of the fields in the parent class shouldn't conflict.

I added a related test. Please see if it can clarify your concern.

jiangxb1987

LGTM

SparkQA · 2017-10-24T17:55:28Z

Test build #83016 has finished for PR 19556 at commit de5cbde.

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

cloud-fan · 2017-10-25T05:43:49Z

core/src/main/scala/org/apache/spark/util/ClosureCleaner.scala

+      outerClasses: Seq[Class[_]]): Unit = {
+    for (cls <- outerClasses) {
+      var currentClass = cls
+      do {


I feel it's better to use while loop here. Programmatically the loop requires currentClass != null, even for the first loop. To completely keep the previous behavior, we can add a assert(cls != null) before the loop.

Ok. Updated.

cloud-fan · 2017-10-25T05:50:05Z

core/src/main/scala/org/apache/spark/util/ClosureCleaner.scala

+
+    var currentClass = outerClass
+    do {
+      setAccessedFields(currentClass, clone, obj, accessedFields)


Assume we have class A and B having the same parent class P. P has 2 fields a and b. The closure accessed A.a and B.b, so when we clone A object, we should only set field a, when we clone B object, we should only set field b. However here seems we set field a and b for A and B object, which is sub-optimal.

Seems this is also a issue for the outerClasses, maybe I missed something...

Seems that is true. For a closure that only accessed A.a, we clone the whole A object which contains both a and b fields. This is the fact in existing ClosureCleaner.

As this is not a regression, IIUC, will it block this change?

yea let's leave it

SparkQA · 2017-10-25T10:32:54Z

Test build #83037 has finished for PR 19556 at commit 4d8f91e.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
assert(currentClass != null, \"The outer class can't be null.\")

cloud-fan · 2017-10-25T15:28:44Z

core/src/main/scala/org/apache/spark/util/ClosureCleaner.scala

+    val clone = instantiateClass(outerClass, parent)
+
+    var currentClass = outerClass
+    do {


please update this do while loop too

Ok. It's late, I will update this and below tomorrow. Thanks.

cloud-fan · 2017-10-25T15:29:40Z

core/src/main/scala/org/apache/spark/util/ClosureCleaner.scala

-                new FieldAccessFinder(fields, findTransitively, Some(m), visitedMethods), 0)
+
+              var currentClass = cl
+              do {


viirya · 2017-10-26T01:54:45Z

@cloud-fan Two remaining do while loop are updated.

SparkQA · 2017-10-26T04:51:16Z

Test build #83069 has finished for PR 19556 at commit e26d093.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
assert(currentClass != null, \"The outer class can't be null.\")
assert(currentClass != null, \"The outer class can't be null.\")

cloud-fan · 2017-10-26T20:45:00Z

thanks, merging to master/2.2!

…ass fields When the given closure uses some fields defined in super class, `ClosureCleaner` can't figure them and don't set it properly. Those fields will be in null values. Added test. Author: Liang-Chi Hsieh <viirya@gmail.com> Closes #19556 from viirya/SPARK-22328. (cherry picked from commit 4f8dc6b) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

…ass fields When the given closure uses some fields defined in super class, `ClosureCleaner` can't figure them and don't set it properly. Those fields will be in null values. Added test. Author: Liang-Chi Hsieh <viirya@gmail.com> Closes apache#19556 from viirya/SPARK-22328. (cherry picked from commit 4f8dc6b) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

viirya force-pushed the SPARK-22328 branch from 29c5d73 to 6606910 Compare October 23, 2017 08:33

viirya added 2 commits October 23, 2017 08:34

ClosureCleaner should fill referenced superclass fields.

6606910

Add another test.

5ac7540

Refactoring tests.

da747ca

srowen reviewed Oct 23, 2017

View reviewed changes

Address comment.

5d7efd1

srowen reviewed Oct 24, 2017

View reviewed changes

cloud-fan reviewed Oct 24, 2017

View reviewed changes

Add a test.

de5cbde

jiangxb1987 approved these changes Oct 24, 2017

View reviewed changes

cloud-fan reviewed Oct 25, 2017

View reviewed changes

Address comment.

4d8f91e

cloud-fan reviewed Oct 25, 2017

View reviewed changes

Address comments.

e26d093

asfgit closed this in 4f8dc6b Oct 26, 2017

viirya deleted the SPARK-22328 branch December 27, 2023 18:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-22328][Core] ClosureCleaner should not miss referenced superclass fields #19556

[SPARK-22328][Core] ClosureCleaner should not miss referenced superclass fields #19556

viirya commented Oct 23, 2017

SparkQA commented Oct 23, 2017

SparkQA commented Oct 23, 2017

SparkQA commented Oct 23, 2017

SparkQA commented Oct 23, 2017

srowen Oct 23, 2017

viirya Oct 24, 2017

viirya commented Oct 24, 2017

SparkQA commented Oct 24, 2017

srowen left a comment

cloud-fan Oct 24, 2017

viirya Oct 24, 2017

viirya Oct 24, 2017

jiangxb1987 left a comment

SparkQA commented Oct 24, 2017

cloud-fan Oct 25, 2017

viirya Oct 25, 2017

cloud-fan Oct 25, 2017

cloud-fan Oct 25, 2017

viirya Oct 25, 2017 •

edited

Loading

viirya Oct 25, 2017

cloud-fan Oct 25, 2017

SparkQA commented Oct 25, 2017

cloud-fan Oct 25, 2017

viirya Oct 25, 2017

cloud-fan Oct 25, 2017

viirya commented Oct 26, 2017

SparkQA commented Oct 26, 2017

cloud-fan commented Oct 26, 2017

[SPARK-22328][Core] ClosureCleaner should not miss referenced superclass fields #19556

[SPARK-22328][Core] ClosureCleaner should not miss referenced superclass fields #19556

Conversation

viirya commented Oct 23, 2017

What changes were proposed in this pull request?

How was this patch tested?

SparkQA commented Oct 23, 2017

SparkQA commented Oct 23, 2017

SparkQA commented Oct 23, 2017

SparkQA commented Oct 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viirya commented Oct 24, 2017

SparkQA commented Oct 24, 2017

srowen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiangxb1987 left a comment

Choose a reason for hiding this comment

SparkQA commented Oct 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viirya Oct 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Oct 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viirya commented Oct 26, 2017

SparkQA commented Oct 26, 2017

cloud-fan commented Oct 26, 2017

viirya Oct 25, 2017 •

edited

Loading