[SPARK-7120][SPARK-7121] Closure cleaner nesting + documentation + tests #5685

andrewor14 · 2015-04-24T10:19:21Z

Note: ~600 lines of this is test code, and ~100 lines documentation.

[SPARK-7121] ClosureCleaner does not handle nested closures properly. For instance, in SparkContext, I tried to do the following:

def scope[T](body: => T): T = body // no-op
def myCoolMethod(path: String): RDD[String] = scope {
  parallelize(1 to 10).map { _ => path }
}

and I got an exception complaining that SparkContext is not serializable. The issue here is that the inner closure is getting its path from the outer closure (the scope), but the outer closure references the SparkContext object itself to get the parallelize method.

Note, however, that the inner closure doesn't actually need the SparkContext; it just needs a field from the outer closure. If we modify ClosureCleaner to clean the outer closure recursively using only the fields accessed by the inner closure, then we can serialize the inner closure.

[SPARK-7120] Also, the other thing is that this file is one of the least understood, partly because it is very low level and is written a long time ago. This patch attempts to change that by adding the missing documentation.

This is blocking my effort on a separate task #5729.

See in-code comments for more detail on what this means.

... in case anything breaks, we should be able to resort to old behavior.

SparkQA · 2015-04-24T11:33:33Z

Test build #30930 has finished for PR 5685 at commit 2390a60.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- * class SomethingNotSerializable
- logDebug(s" + cloning the object $obj of class $
- class FieldAccessFinder(
This patch does not change any dependencies.

srowen · 2015-04-24T11:42:53Z

You know a lot more about this than I, but I was under the impression that the closure cleaner couldn't clean beyond a level or so because it would then be modifying local object state by nulling fields in them and that's not necessarily permissible. I'm sure you're on top of that, just noting my recollection from similar discussions.

pwendell · 2015-04-24T16:56:12Z

@srowen wondering, what do you mean by "local state"? The closure cleaner only nulls out fields in clones of local objects (actually the nulling mechanism is really just that it creates a clone and then selectively copies over certain fields from the original and neglects others). So I think it should be fine.

aarondav · 2015-04-24T16:58:32Z

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

    if (!inInterpreter) {
      // This is a bona fide closure class, whose constructor has no effects
      // other than to set its fields, so use its constructor
      val cons = cls.getConstructors()(0)
      val params = cons.getParameterTypes.map(createNullValue).toArray
-      if (outer != null) {
-        params(0) = outer // First param is always outer object
+      if (enclosingObject!= null) {


oops thanks

srowen · 2015-04-24T17:30:49Z

Yeah I'm probably misunderstanding prior discussions, but I had the impression there was a reason you couldn't always clear references two hops away. This would be great to improve if it can be done.

andrewor14 · 2015-04-24T18:43:54Z

@srowen we cannot null fields in parent closures in place, but the closure cleaner actually clones these parent closures so it's safe to null things out there. Also, we did clear normal references multiple hops away; we just didn't also clear the parent pointers of the enclosing closures because there wasn't a way to determine if these pointers are actually needed (i.e. transitively referenced).

aarondav · 2015-04-24T20:14:31Z

core/src/test/scala/org/apache/spark/util/ClosureCleanerSuite.scala

@@ -50,7 +50,7 @@ class ClosureCleanerSuite extends FunSuite {
    val obj = new TestClassWithNesting(1)
    assert(obj.run() === 96) // 4 * (1+2+3+4) + 4 * (1+2+3+4) + 16 * 1
  }
-  
+


The suite should be expanded with the fix, right? Could you also test the common for loop case that others have seen in the past?

edit: I see your comment says tests are pending; maybe just note that I'd love to see a for loop test in particular.

coming soon

The existing ones are not passing yet because cleaning closures is not idempotent. This will be added in a future commit.

We need this for tests because we clean the same closure many times there. Outside of tests this is probably not important.

mateiz · 2015-04-24T23:58:17Z

Does this happen in user programs or just in SparkContext? This is exactly what ClosureCleaner was designed to deal with, so I'm surprised that it's a problem.

mateiz · 2015-04-25T00:02:19Z

BTW the general workaround for this kind of stuff is to grab the values from outside the closure into a local val before you call something. I'd look into how to do that before changing the ClosureCleaner too heavily.

andrewor14 · 2015-04-25T00:22:49Z

Yes, this also affects user programs. For example I modified SparkPi to follow the pattern I described in the PR description

    ...
    val slices = if (args.length > 0) args(0).toInt else 2
    (1 to 1).foreach { j =>
      val count = spark.parallelize(1 until n, slices).map { i =>
        val z = slices // *** This is the culprit ***
        val x = random * 2 - 1
        val y = random * 2 - 1
        if (x * x + y * y < 1) 1 else 0
      }.reduce(_ + _)
    }
    ...

If you run this in master branch you will run into a task not serializable exception. I was able to verify that this patch fixes this as long as spark.closureCleaner.transitive is enabled. If this is not enabled, if fails with the same exception as before.

andrewor14 · 2015-04-25T00:26:56Z

The main issue with grabbing the values into the closure is that we'll need to do this everywhere. My intention is to wrap many existing methods (in SparkContext and other files) in closures, and it will be very difficult and manual to have to localize the variables in all of these places.

Now we keep track of the methods that we visited to avoid visiting the same method twice.

andrewor14 · 2015-04-25T21:38:32Z

Jenkins, test this please?

SparkQA · 2015-04-25T21:59:12Z

Test build #700 has started for PR 5685 at commit eb127e5.

SparkQA · 2015-04-29T21:42:06Z

Test build #31315 has finished for PR 5685 at commit 26c7aba.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- * class SomethingNotSerializable
- logDebug(s" + cloning the object $obj of class $
This patch does not change any dependencies.

pwendell · 2015-04-29T23:15:46Z

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

+   *       def y = 2
+   *       scope("two") { println(y + 1) }
+   *     }
+   *     def scope(name: String)(body: => Unit) = body


maybe move this def up to define it before you use it

pwendell · 2015-04-30T06:14:33Z

Hey Andrew,

This is looking good. The code is quite dense but as far as I can tell, this is correct. I left some more surface level comments, if you can get to those I think it will be good to go.

zsxwing · 2015-04-30T20:13:28Z

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

+      // The outer pointer may be null if we have cleaned this closure before
+      if (outer != null) {
+        if (isClosure(f.getType)) {
+          return f.getType :: getOuterClasses(f.get(obj))


"f.get(obj)" -> "outer"

oops yes, thanks.

pwendell · 2015-05-01T05:42:19Z

@andrewor14 if you can update this I think this one is good to go.

SparkQA · 2015-05-02T01:04:29Z

Test build #31637 has finished for PR 5685 at commit 26c5072.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- * class SomethingNotSerializable
- logDebug(s" + cloning the object $obj of class $

SparkQA · 2015-05-02T01:09:23Z

Test build #31639 has finished for PR 5685 at commit ea874bc.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- * class SomethingNotSerializable
- logDebug(s" + cloning the object $obj of class $

andrewor14 · 2015-05-02T01:20:58Z

Ok the latest changes are G2G from my side.

SparkQA · 2015-05-02T03:48:51Z

Test build #31642 timed out for PR 5685 at commit 0bbe77f after a configured wait of 150m.

DStream#transform isn't cleaning closures correctly. It is passing an RDD to ClosureCleaner#clean. We should fix this separately outside of this patch.

andrewor14 · 2015-05-02T05:19:53Z

It turns out that in streaming we currently pass an RDD into ClosureCleaner#clean so we can't do the type safety check that you suggested @pwendell. I will fix this separately later in #5860.

pwendell · 2015-05-02T05:21:38Z

@andrewor14 okay makes sense. Thanks for all the hard work on this andrew LGTM pending tests.

SparkQA · 2015-05-02T06:53:20Z

Test build #31654 has finished for PR 5685 at commit cd46230.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- * class SomethingNotSerializable
- logDebug(s" + cloning the object $obj of class $

…ests Note: ~600 lines of this is test code, and ~100 lines documentation. **[SPARK-7121]** ClosureCleaner does not handle nested closures properly. For instance, in SparkContext, I tried to do the following: ``` def scope[T](body: => T): T = body // no-op def myCoolMethod(path: String): RDD[String] = scope { parallelize(1 to 10).map { _ => path } } ``` and I got an exception complaining that SparkContext is not serializable. The issue here is that the inner closure is getting its path from the outer closure (the scope), but the outer closure references the SparkContext object itself to get the `parallelize` method. Note, however, that the inner closure doesn't actually need the SparkContext; it just needs a field from the outer closure. If we modify ClosureCleaner to clean the outer closure recursively using only the fields accessed by the inner closure, then we can serialize the inner closure. **[SPARK-7120]** Also, the other thing is that this file is one of the least understood, partly because it is very low level and is written a long time ago. This patch attempts to change that by adding the missing documentation. This is blocking my effort on a separate task apache#5729. Author: Andrew Or <andrew@databricks.com> Closes apache#5685 from andrewor14/closure-cleaner and squashes the following commits: cd46230 [Andrew Or] Revert a small change that affected streaming 0bbe77f [Andrew Or] Fix style ea874bc [Andrew Or] Fix tests 26c5072 [Andrew Or] Address comments 16fbcfd [Andrew Or] Merge branch 'master' of github.com:apache/spark into closure-cleaner 26c7aba [Andrew Or] Revert "In sc.runJob, actually clean the inner closure" 6f75784 [Andrew Or] Revert "Guard against NPE if CC is used outside of an application" e909a42 [Andrew Or] Guard against NPE if CC is used outside of an application 3998168 [Andrew Or] In sc.runJob, actually clean the inner closure 9187066 [Andrew Or] Merge branch 'master' of github.com:apache/spark into closure-cleaner d889950 [Andrew Or] Revert "Bypass SerializationDebugger for now (SPARK-7180)" 9419efe [Andrew Or] Bypass SerializationDebugger for now (SPARK-7180) 6d4d3f1 [Andrew Or] Fix scala style? 4aab379 [Andrew Or] Merge branch 'master' of github.com:apache/spark into closure-cleaner e45e904 [Andrew Or] More minor updates (wording, renaming etc.) 8b71cdb [Andrew Or] Update a few comments eb127e5 [Andrew Or] Use private method tester for a few things a3aa465 [Andrew Or] Add more tests for individual closure cleaner operations e672170 [Andrew Or] Guard against potential infinite cycles in method visitor 6d36f38 [Andrew Or] Fix closure cleaner visibility 2106f12 [Andrew Or] Merge branch 'master' of github.com:apache/spark into closure-cleaner 263593d [Andrew Or] Finalize tests 06fd668 [Andrew Or] Make closure cleaning idempotent a4866e3 [Andrew Or] Add tests (still WIP) 438c68f [Andrew Or] Minor changes 2390a60 [Andrew Or] Feature flag this new behavior 86f7823 [Andrew Or] Implement transitive cleaning + add missing documentation

Andrew Or added 2 commits April 24, 2015 03:05

Implement transitive cleaning + add missing documentation

86f7823

See in-code comments for more detail on what this means.

Feature flag this new behavior

2390a60

... in case anything breaks, we should be able to resort to old behavior.

aarondav reviewed Apr 24, 2015
View reviewed changes

Minor changes

438c68f

aarondav reviewed Apr 24, 2015
View reviewed changes

Andrew Or added 3 commits April 24, 2015 16:18

Add tests (still WIP)

a4866e3

The existing ones are not passing yet because cleaning closures is not idempotent. This will be added in a future commit.

Make closure cleaning idempotent

06fd668

We need this for tests because we clean the same closure many times there. Outside of tests this is probably not important.

Finalize tests

263593d

Andrew Or added 2 commits April 24, 2015 17:29

Merge branch 'master' of github.com:apache/spark into closure-cleaner

2106f12

Fix closure cleaner visibility

6d36f38

andrewor14 changed the title ~~[SPARK-7120][SPARK-7121][WIP] Closure cleaner nesting + documentation~~ [SPARK-7120][SPARK-7121] Closure cleaner nesting + documentation Apr 25, 2015

Guard against potential infinite cycles in method visitor

e672170

Now we keep track of the methods that we visited to avoid visiting the same method twice.

andrewor14 force-pushed the closure-cleaner branch from 29da296 to e672170 Compare April 25, 2015 02:32

Add more tests for individual closure cleaner operations

a3aa465

andrewor14 changed the title ~~[SPARK-7120][SPARK-7121] Closure cleaner nesting + documentation~~ [SPARK-7120][SPARK-7121] Closure cleaner nesting + documentation + tests Apr 25, 2015

Use private method tester for a few things

eb127e5

Update a few comments

8b71cdb

pwendell reviewed Apr 29, 2015
View reviewed changes

zsxwing reviewed Apr 30, 2015
View reviewed changes

Andrew Or added 2 commits May 1, 2015 17:44

Merge branch 'master' of github.com:apache/spark into closure-cleaner

16fbcfd

Address comments

26c5072

Fix tests

ea874bc

Fix style

0bbe77f

Revert a small change that affected streaming

cd46230

DStream#transform isn't cleaning closures correctly. It is passing an RDD to ClosureCleaner#clean. We should fix this separately outside of this patch.

asfgit closed this in 7394e7a May 2, 2015

andrewor14 deleted the closure-cleaner branch May 2, 2015 07:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-7120][SPARK-7121] Closure cleaner nesting + documentation + tests #5685

[SPARK-7120][SPARK-7121] Closure cleaner nesting + documentation + tests #5685

andrewor14 commented Apr 24, 2015

SparkQA commented Apr 24, 2015

srowen commented Apr 24, 2015

pwendell commented Apr 24, 2015

aarondav Apr 24, 2015

andrewor14 Apr 24, 2015

srowen commented Apr 24, 2015

andrewor14 commented Apr 24, 2015

aarondav Apr 24, 2015

andrewor14 Apr 24, 2015

mateiz commented Apr 24, 2015

mateiz commented Apr 25, 2015

andrewor14 commented Apr 25, 2015

andrewor14 commented Apr 25, 2015

andrewor14 commented Apr 25, 2015

SparkQA commented Apr 25, 2015

SparkQA commented Apr 29, 2015

pwendell Apr 29, 2015

pwendell commented Apr 30, 2015

zsxwing Apr 30, 2015

andrewor14 May 1, 2015

pwendell commented May 1, 2015

SparkQA commented May 2, 2015

SparkQA commented May 2, 2015

andrewor14 commented May 2, 2015

SparkQA commented May 2, 2015

andrewor14 commented May 2, 2015

pwendell commented May 2, 2015

SparkQA commented May 2, 2015

[SPARK-7120][SPARK-7121] Closure cleaner nesting + documentation + tests #5685

[SPARK-7120][SPARK-7121] Closure cleaner nesting + documentation + tests #5685

Conversation

andrewor14 commented Apr 24, 2015

SparkQA commented Apr 24, 2015

srowen commented Apr 24, 2015

pwendell commented Apr 24, 2015

aarondav Apr 24, 2015

Choose a reason for hiding this comment

andrewor14 Apr 24, 2015

Choose a reason for hiding this comment

srowen commented Apr 24, 2015

andrewor14 commented Apr 24, 2015

aarondav Apr 24, 2015

Choose a reason for hiding this comment

andrewor14 Apr 24, 2015

Choose a reason for hiding this comment

mateiz commented Apr 24, 2015

mateiz commented Apr 25, 2015

andrewor14 commented Apr 25, 2015

andrewor14 commented Apr 25, 2015

andrewor14 commented Apr 25, 2015

SparkQA commented Apr 25, 2015

SparkQA commented Apr 29, 2015

pwendell Apr 29, 2015

Choose a reason for hiding this comment

pwendell commented Apr 30, 2015

zsxwing Apr 30, 2015

Choose a reason for hiding this comment

andrewor14 May 1, 2015

Choose a reason for hiding this comment

pwendell commented May 1, 2015

SparkQA commented May 2, 2015

SparkQA commented May 2, 2015

andrewor14 commented May 2, 2015

SparkQA commented May 2, 2015

andrewor14 commented May 2, 2015

pwendell commented May 2, 2015

SparkQA commented May 2, 2015