You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by srowen <gi...@git.apache.org> on 2017/11/06 15:08:11 UTC

[GitHub] spark pull request #19675: [SPARK-14540][BUILD] Support Scala 2.12 closures ...

GitHub user srowen opened a pull request:

    https://github.com/apache/spark/pull/19675

    [SPARK-14540][BUILD] Support Scala 2.12 closures and Java 8 lambdas in ClosureCleaner (step 0)

    ## What changes were proposed in this pull request?
    
    Preliminary changes to get ClosureCleaner to work with Scala 2.12. Makes many usages just work, but not all. This does _not_ resolve the JIRA.
    
    ## How was this patch tested?
    
    Existing tests

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/srowen/spark SPARK-14540.0

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/spark/pull/19675.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 #19675
    
----
commit b27a9feafbbc438bec4f64d1e7f576b2d06e2041
Author: Sean Owen <so...@cloudera.com>
Date:   2017-11-06T12:52:52Z

    Preliminary changes to get ClosureCleaner to work with Scala 2.12. Makes many usages just work, but not all.

----


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19675: [SPARK-14540][BUILD] Support Scala 2.12 closures and Jav...

Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:

    https://github.com/apache/spark/pull/19675
  
    Merged to master


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19675: [SPARK-14540][BUILD] Support Scala 2.12 closures and Jav...

Posted by ShaneDelmore <gi...@git.apache.org>.
Github user ShaneDelmore commented on the issue:

    https://github.com/apache/spark/pull/19675
  
    Does that mean no chance of 2.12 support for ~6 months?


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19675: [SPARK-14540][BUILD] Support Scala 2.12 closures and Jav...

Posted by jvican <gi...@git.apache.org>.
Github user jvican commented on the issue:

    https://github.com/apache/spark/pull/19675
  
    I would be very happy to accept feedback on the latest spores implementation https://github.com/scalacenter/spores/ and make them production ready, possibly adding them into the language after a potential SIP. I've always thought that they have lots of unexploited potential.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19675: [SPARK-14540][BUILD] Support Scala 2.12 closures and Jav...

Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:

    https://github.com/apache/spark/pull/19675
  
    @fedeoasi I got just about everything but the core issue fixed. I think someone with a lot of understanding of the JVM and Scala would be well positioned to work on this; not a lot of Spark needed. But it's fairly subtle.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19675: [SPARK-14540][BUILD] Support Scala 2.12 closures and Jav...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/19675
  
    Test PASSed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/83498/
    Test PASSed.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19675: [SPARK-14540][BUILD] Support Scala 2.12 closures and Jav...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/19675
  
    **[Test build #83496 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/83496/testReport)** for PR 19675 at commit [`b27a9fe`](https://github.com/apache/spark/commit/b27a9feafbbc438bec4f64d1e7f576b2d06e2041).
     * This patch **fails Scala style tests**.
     * This patch merges cleanly.
     * This patch adds no public classes.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19675: [SPARK-14540][BUILD] Support Scala 2.12 closures and Jav...

Posted by ekrich <gi...@git.apache.org>.
Github user ekrich commented on the issue:

    https://github.com/apache/spark/pull/19675
  
    I just posted the info on https://gitter.im/scala/contributors but there is also scala/center and scala/scala or the forum at https://contributors.scala-lang.org/ . Maybe Lightbend too.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19675: [SPARK-14540][BUILD] Support Scala 2.12 closures and Jav...

Posted by ekrich <gi...@git.apache.org>.
Github user ekrich commented on the issue:

    https://github.com/apache/spark/pull/19675
  
    Thank-you for the clarification.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19675: [SPARK-14540][BUILD] Support Scala 2.12 closures and Jav...

Posted by hepin1989 <gi...@git.apache.org>.
Github user hepin1989 commented on the issue:

    https://github.com/apache/spark/pull/19675
  
    @fedeoasi Be a sponsor 


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19675: [SPARK-14540][BUILD] Support Scala 2.12 closures and Jav...

Posted by retronym <gi...@git.apache.org>.
Github user retronym commented on the issue:

    https://github.com/apache/spark/pull/19675
  
    I suppose the problematic case is:
    
    ```
    scala> :javap p2.C$D#$anonfun$test$1
      public static final java.lang.String $anonfun$test$1(p2.C$D, java.lang.String);
        descriptor: (Lp2/C$D;Ljava/lang/String;)Ljava/lang/String;
        flags: ACC_PUBLIC, ACC_STATIC, ACC_FINAL, ACC_SYNTHETIC
        Code:
          stack=2, locals=2, args_size=2
             0: new           #51                 // class java/lang/StringBuilder
             3: dup
             4: invokespecial #55                 // Method java/lang/StringBuilder."<init>":()V
             7: aload_0
             8: invokevirtual #57                 // Method p2$C$D$$$outer:()Lp2/C;
            11: invokevirtual #61                 // Method p2/C.c:()Ljava/lang/String;
            14: invokevirtual #65                 // Method java/lang/StringBuilder.append:(Ljava/lang/String;)Ljava/lang/StringBuilder;
            17: aload_1
            18: invokevirtual #65                 // Method java/lang/StringBuilder.append:(Ljava/lang/String;)Ljava/lang/StringBuilder;
            21: invokevirtual #68                 // Method java/lang/StringBuilder.toString:()Ljava/lang/String;
            24: areturn
          LocalVariableTable:
            Start  Length  Slot  Name   Signature
                0      25     0 $this   Lp2/C$D;
                0      25     1     x   Ljava/lang/String;
          LineNumberTable:
            line 1: 0
        MethodParameters:
          Name                           Flags
          $this                          final synthetic
          x                              final
    
    ```
    
    Where you'd prefer to eagerly capture `C.this.c`, rather than capturing `D`.
    
    You could do that manually as:
    
    ```
    scala> :paste -raw
    // Entering paste mode (ctrl-D to finish)
    
    package p3; class C { val c = "c"; class D { def test = { val capturedC = c; (x: String) => capturedC + x } } }
    ```
    
    In general, this could change semantics, `x` might not be initialized by `C.<init>` at the time of the lambda capture. Spores is a good solution here: you have do the rewrite manually (so the semantic change is your responsibility!), but spores will make sure you've remembered to do it.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19675: [SPARK-14540][BUILD] Support Scala 2.12 closures and Jav...

Posted by fedeoasi <gi...@git.apache.org>.
Github user fedeoasi commented on the issue:

    https://github.com/apache/spark/pull/19675
  
    The JIRA story has been unassigned for a while (since November 2017). Is anyone thinking of picking it up? With the first Scala 2.13 release coming soon it would be great to get this moving.
    
    @srowen do you think it's possible for a somebody with no prior Spark development experience to pick this up or does it require an expert?
    



---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19675: [SPARK-14540][BUILD] Support Scala 2.12 closures and Jav...

Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:

    https://github.com/apache/spark/pull/19675
  
    Yes that should be correct unless something else has gone wrong for 2.12 in master in the past few months. 


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request #19675: [SPARK-14540][BUILD] Support Scala 2.12 closures ...

Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:

    https://github.com/apache/spark/pull/19675#discussion_r149103715
  
    --- Diff: graphx/src/main/scala/org/apache/spark/graphx/lib/ShortestPaths.scala ---
    @@ -25,7 +25,7 @@ import org.apache.spark.graphx._
      * Computes shortest paths to the given set of landmark vertices, returning a graph where each
      * vertex attribute is a map containing the shortest-path distance to each reachable landmark.
      */
    -object ShortestPaths {
    +object ShortestPaths extends Serializable {
    --- End diff --
    
    The two test changes aren't ultimately going to be necessary if the ClosureCleaner is fully updated, but, these are a few easy fixes to tests that can easily be changed to avoid some closure cleaning anyway.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19675: [SPARK-14540][BUILD] Support Scala 2.12 closures and Jav...

Posted by jvican <gi...@git.apache.org>.
Github user jvican commented on the issue:

    https://github.com/apache/spark/pull/19675
  
    Is this issue partially or finally fixed? I could try to get this to the finish line for a nice start of the year.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19675: [SPARK-14540][BUILD] Support Scala 2.12 closures and Jav...

Posted by ekrich <gi...@git.apache.org>.
Github user ekrich commented on the issue:

    https://github.com/apache/spark/pull/19675
  
    @srowen Are you needing expertise to get the last issues fixed? It would sure be nice to see this completed and released. Java 8/Scala 2.12 was a hard upgrade for Scala too and it has taken the point releases to solve some problems needed for Spark. Scala 2.12.0 was released on 3 Nov 2016 and 2.13 is on the horizon so it would be great to see Spark support 2.12 very soon.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19675: [SPARK-14540][BUILD] Support Scala 2.12 closures and Jav...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/19675
  
    **[Test build #83498 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/83498/testReport)** for PR 19675 at commit [`bf393ae`](https://github.com/apache/spark/commit/bf393ae2904881004d621f3729064a2739ccb039).


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request #19675: [SPARK-14540][BUILD] Support Scala 2.12 closures ...

Posted by srowen <gi...@git.apache.org>.
Github user srowen closed the pull request at:

    https://github.com/apache/spark/pull/19675


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19675: [SPARK-14540][BUILD] Support Scala 2.12 closures and Jav...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/19675
  
    Merged build finished. Test PASSed.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19675: [SPARK-14540][BUILD] Support Scala 2.12 closures and Jav...

Posted by ekrich <gi...@git.apache.org>.
Github user ekrich commented on the issue:

    https://github.com/apache/spark/pull/19675
  
    Interestingly, when searching for spores I ran across this post - https://www.reddit.com/r/scala/comments/702lct/what_ever_happened_to_sip21_aka_scala_spores/


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19675: [SPARK-14540][BUILD] Support Scala 2.12 closures and Jav...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/19675
  
    **[Test build #83498 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/83498/testReport)** for PR 19675 at commit [`bf393ae`](https://github.com/apache/spark/commit/bf393ae2904881004d621f3729064a2739ccb039).
     * This patch passes all tests.
     * This patch merges cleanly.
     * This patch adds the following public classes _(experimental)_:
      * `class StateObject(var counter: Int = 0, var expireCounter: Int = 0) extends Serializable`


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19675: [SPARK-14540][BUILD] Support Scala 2.12 closures and Jav...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/19675
  
    Test FAILed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/83496/
    Test FAILed.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19675: [SPARK-14540][BUILD] Support Scala 2.12 closures and Jav...

Posted by fedeoasi <gi...@git.apache.org>.
Github user fedeoasi commented on the issue:

    https://github.com/apache/spark/pull/19675
  
    Ideally we would get someone from Scala or Scala Center to pick this up. Anyone knows how to get in touch with them?


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request #19675: [SPARK-14540][BUILD] Support Scala 2.12 closures ...

Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:

    https://github.com/apache/spark/pull/19675#discussion_r149103482
  
    --- Diff: core/src/main/scala/org/apache/spark/util/ClosureCleaner.scala ---
    @@ -38,12 +38,13 @@ private[spark] object ClosureCleaner extends Logging {
         // Copy data over, before delegating to ClassReader - else we can run out of open file handles.
         val className = cls.getName.replaceFirst("^.*\\.", "") + ".class"
         val resourceStream = cls.getResourceAsStream(className)
    -    // todo: Fixme - continuing with earlier behavior ...
    -    if (resourceStream == null) return new ClassReader(resourceStream)
    --- End diff --
    
    Note that this already immediately generated an exception from `ClassReader`


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19675: [SPARK-14540][BUILD] Support Scala 2.12 closures and Jav...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/19675
  
    Merged build finished. Test FAILed.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19675: [SPARK-14540][BUILD] Support Scala 2.12 closures and Jav...

Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:

    https://github.com/apache/spark/pull/19675
  
    No, 2.11 would still have to be supported. Next minor release is probably 6 months out 


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19675: [SPARK-14540][BUILD] Support Scala 2.12 closures and Jav...

Posted by skonto <gi...@git.apache.org>.
Github user skonto commented on the issue:

    https://github.com/apache/spark/pull/19675
  
    Current status:
    ```
    ProactiveClosureSerializationSuite:
    - throws expected serialization exceptions on actions
    - mapPartitions transformations throw proactive serialization exceptions *** FAILED ***
      Expected exception org.apache.spark.SparkException to be thrown, but no exception was thrown (ProactiveClosureSerializationSuite.scala:58)
    - map transformations throw proactive serialization exceptions *** FAILED ***
      Expected exception org.apache.spark.SparkException to be thrown, but no exception was thrown (ProactiveClosureSerializationSuite.scala:58)
    - filter transformations throw proactive serialization exceptions *** FAILED ***
      Expected exception org.apache.spark.SparkException to be thrown, but no exception was thrown (ProactiveClosureSerializationSuite.scala:58)
    - flatMap transformations throw proactive serialization exceptions *** FAILED ***
      Expected exception org.apache.spark.SparkException to be thrown, but no exception was thrown (ProactiveClosureSerializationSuite.scala:58)
    - mapPartitionsWithIndex transformations throw proactive serialization exceptions *** FAILED ***
      Expected exception org.apache.spark.SparkException to be thrown, but no exception was thrown (ProactiveClosureSerializationSuite.scala:58)
    Run completed in 15 minutes, 23 seconds.
    Total number of tests run: 2096
    Suites: completed 212, aborted 0
    Tests: succeeded 2079, failed 17, canceled 4, ignored 8, pending 0
    *** 17 TESTS FAILED ***
    [INFO] ------------------------------------------------------------------------
    [INFO] Reactor Summary:
    [INFO] 
    [INFO] Spark Project Parent POM 2.4.0-SNAPSHOT ............ SUCCESS [ 24.719 s]
    [INFO] Spark Project Tags ................................. SUCCESS [ 31.125 s]
    [INFO] Spark Project Sketch ............................... SUCCESS [ 14.175 s]
    [INFO] Spark Project Local DB ............................. SUCCESS [  5.103 s]
    [INFO] Spark Project Networking ........................... SUCCESS [ 40.177 s]
    [INFO] Spark Project Shuffle Streaming Service ............ SUCCESS [  5.344 s]
    [INFO] Spark Project Unsafe ............................... SUCCESS [  7.984 s]
    [INFO] Spark Project Launcher ............................. SUCCESS [  6.129 s]
    [INFO] Spark Project Core ................................. FAILURE [17:17 min]
    ```


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19675: [SPARK-14540][BUILD] Support Scala 2.12 closures and Jav...

Posted by skonto <gi...@git.apache.org>.
Github user skonto commented on the issue:

    https://github.com/apache/spark/pull/19675
  
    @srowen If I try with `/dev/change-scala-version 2.12 and -Pscala-2.12` two tests still fail right?


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19675: [SPARK-14540][BUILD] Support Scala 2.12 closures and Jav...

Posted by jvican <gi...@git.apache.org>.
Github user jvican commented on the issue:

    https://github.com/apache/spark/pull/19675
  
    @srowen Can you concretize when the next Apache spark version will be released? I guess that the whole Scala 2.12 support will require a new major version bump, and I don't know how often you cut these releases.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19675: [SPARK-14540][BUILD] Support Scala 2.12 closures and Jav...

Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:

    https://github.com/apache/spark/pull/19675
  
    Did you have a look at the JIRA? lots more detail there. https://issues.apache.org/jira/browse/SPARK-14540


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19675: [SPARK-14540][BUILD] Support Scala 2.12 closures and Jav...

Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:

    https://github.com/apache/spark/pull/19675
  
    It is partially fixed. If you switch to the 2.12 build you should find it compiles and runs, and 99% of tests pass. However it is definitely not complete.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19675: [SPARK-14540][BUILD] Support Scala 2.12 closures and Jav...

Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:

    https://github.com/apache/spark/pull/19675
  
    I am not working on this and am not aware of anyone working on it. Yes, it will only be resolved when someone picks up the last piece of work and finishes it. It should be down to handling how closures are serialized as lambdas now; all the other updates should be in place.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19675: [SPARK-14540][BUILD] Support Scala 2.12 closures and Jav...

Posted by retronym <gi...@git.apache.org>.
Github user retronym commented on the issue:

    https://github.com/apache/spark/pull/19675
  
    It just occurred to me that my answer above, discussing ways to access the bytecode of the lambda class, aren't really relevant here. The structure of the lambda class is always the same: it just passes the captures and the function parameters to the lambda implementation method, which is hosted in the class that encloses the lambda. You could analyse the body of this method to see if you can find any unused captures (even though scalac has already endeavoured to do this).
    
    Let's look at a concrete example:
    
    ```
    cala> def inspect(closure: Object) = { val writeReplace = closure.getClass.getDeclaredMethod("writeReplace"); writeReplace.setAccessible(true); writeReplace.invoke(closure).asInstanceOf[java.lang.invoke.SerializedLambda]}
    inspect: (closure: Object)java.lang.invoke.SerializedLambda
    
    scala> :paste -raw
    // Entering paste mode (ctrl-D to finish)
    
    package p1; class C { def c = "c"; class D { def test = (x: String) => c + x } }
    
    // Exiting paste mode, now interpreting.
    
    scala> val c = new p1.C; val d = new c.D; val closure = d.test; val sl = inspect(closure)
    c: p1.C = p1.C@5034681f
    d: c.D = p1.C$D@1dfe5dd1
    closure: String => String = p1.C$D$$Lambda$1160/155129334@1fe05fff
    sl: java.lang.invoke.SerializedLambda = SerializedLambda[capturingClass=class p1.C$D, functionalInterfaceMethod=scala/Function1.apply:(Ljava/lang/Object;)Ljava/lang/Object;, implementation=invokeStatic p1/C$D.$anonfun$test$1:(Lp1/C$D;Ljava/lang/String;)Ljava/lang/String;, instantiatedMethodType=(Ljava/lang/String;)Ljava/lang/String;, numCaptured=1]
    
    scala> :javap p1.C$D#$anonfun$test$1
      public static final java.lang.String $anonfun$test$1(p1.C$D, java.lang.String);
        descriptor: (Lp1/C$D;Ljava/lang/String;)Ljava/lang/String;
        flags: ACC_PUBLIC, ACC_STATIC, ACC_FINAL, ACC_SYNTHETIC
        Code:
          stack=2, locals=2, args_size=2
             0: new           #51                 // class java/lang/StringBuilder
             3: dup
             4: invokespecial #55                 // Method java/lang/StringBuilder."<init>":()V
             7: aload_0
             8: invokevirtual #57                 // Method p1$C$D$$$outer:()Lp1/C;
            11: invokevirtual #61                 // Method p1/C.c:()Ljava/lang/String;
            14: invokevirtual #65                 // Method java/lang/StringBuilder.append:(Ljava/lang/String;)Ljava/lang/StringBuilder;
            17: aload_1
            18: invokevirtual #65                 // Method java/lang/StringBuilder.append:(Ljava/lang/String;)Ljava/lang/StringBuilder;
            21: invokevirtual #68                 // Method java/lang/StringBuilder.toString:()Ljava/lang/String;
            24: areturn
          LocalVariableTable:
            Start  Length  Slot  Name   Signature
                0      25     0 $this   Lp1/C$D;
                0      25     1     x   Ljava/lang/String;
          LineNumberTable:
            line 1: 0
        MethodParameters:
          Name                           Flags
          $this                          final synthetic
          x
    ```


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19675: [SPARK-14540][BUILD] Support Scala 2.12 closures and Jav...

Posted by retronym <gi...@git.apache.org>.
Github user retronym commented on the issue:

    https://github.com/apache/spark/pull/19675
  
    Another, semi-nuclear option is to use an unsupported option of `LambdaMetafactory` to dump generated classfiles to disk.
    
    ```
    ⚡ mkdir /tmp/dump
    
    ~/code/compiler-benchmark/target/profile-basic on master*
    ⚡ scala -J-Djdk.internal.lambda.dumpProxyClasses=/tmp/dump
    Welcome to Scala 2.12.4 (Java HotSpot(TM) 64-Bit Server VM, Java 1.8.0_162).
    Type in expressions for evaluation. Or try :help.
    
    scala> :paste -raw
    // Entering paste mode (ctrl-D to finish)
    
    package p1
    class C { val x = "foo"; def test1 = () => this.x; def test2 = () => C.this; def test3 = () => "" }
    
    // Exiting paste mode, now interpreting.
    
    scala> new p1.C().test1
    res1: () => String = p1.C$$Lambda$1071/1940783703@6fc1020a
    
    scala> :quit
    
    ⚡ find /tmp/dump/p1
    /tmp/dump/p1
    /tmp/dump/p1/C$$Lambda$1071.class
    ```


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19675: [SPARK-14540][BUILD] Support Scala 2.12 closures and Jav...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/19675
  
    **[Test build #83496 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/83496/testReport)** for PR 19675 at commit [`b27a9fe`](https://github.com/apache/spark/commit/b27a9feafbbc438bec4f64d1e7f576b2d06e2041).


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19675: [SPARK-14540][BUILD] Support Scala 2.12 closures and Jav...

Posted by retronym <gi...@git.apache.org>.
Github user retronym commented on the issue:

    https://github.com/apache/spark/pull/19675
  
    I'm happy to help, but I would appreciate if someone could pose the question you have about the lambda encoding / `SerializedLambda` in a standalone fashion.
    
    In the meantime, here are some details that might help.
    
    To find the captured fields of a Scala 2.12+ (or Java 8+ lambda), you can use:
    
    ```
    scala> :paste -raw
    // Entering paste mode (ctrl-D to finish)
    
    package p1;
    class C { val x = "foo"; def test1 = () => this.x; def test2 = () => C.this; def test3 = () => "" }
    
    // Exiting paste mode, now interpreting.
    
    
    scala> def inspect(closure: Object) = { val writeReplace = closure.getClass.getDeclaredMethod("writeReplace"); writeReplace.setAccessible(true); val sl = writeReplace.invoke(closure).asInstanceOf[java.lang.invoke.SerializedLambda]; println(closure); println(sl); println(List.tabulate(sl.getCapturedArgCount)(sl.getCapturedArg))}
    inspect: (closure: Object)Unit
    
    scala> inspect(new p1.C().test1)
    p1.C$$Lambda$1072/2133998394@79b37277
    SerializedLambda[capturingClass=class p1.C, functionalInterfaceMethod=scala/Function0.apply:()Ljava/lang/Object;, implementation=invokeStatic p1/C.$anonfun$test1$1:(Lp1/C;)Ljava/lang/String;, instantiatedMethodType=()Ljava/lang/String;, numCaptured=1]
    List(p1.C@107f5239)
    
    scala> inspect(new p1.C().test2)
    p1.C$$Lambda$1412/2049133976@1215aae8
    SerializedLambda[capturingClass=class p1.C, functionalInterfaceMethod=scala/Function0.apply:()Ljava/lang/Object;, implementation=invokeStatic p1/C.$anonfun$test2$1:(Lp1/C;)Lp1/C;, instantiatedMethodType=()Lp1/C;, numCaptured=1]
    List(p1.C@21378791)
    
    scala> inspect(new p1.C().test3)
    p1.C$$Lambda$1413/399039406@7643ae41
    SerializedLambda[capturingClass=class p1.C, functionalInterfaceMethod=scala/Function0.apply:()Ljava/lang/Object;, implementation=invokeStatic p1/C.$anonfun$test3$1:()Ljava/lang/String;, instantiatedMethodType=()Ljava/lang/String;, numCaptured=0]
    List()
    ```
    
    If you need to access the bytecode of such a lambda to try to find unused fields, things get harder. The nuclear option is the have your users compile with `-Ydelambdafy:inline`, which avoids the use of `LambdaMetafactory` and spins up the lambda anonymous classes ahead-of-time in `scalac`.
    
    But I would imagine you have the same problems for Java or for Scala, unless there is a problem in Scala's lambda translation where we still capture more, potentially non-serializable or huge, than the equivalent Java code. So it would be good to discuss a concrete problematic example, to see if we can fix something in the compiler.



---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19675: [SPARK-14540][BUILD] Support Scala 2.12 closures and Jav...

Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:

    https://github.com/apache/spark/pull/19675
  
    @retronym thanks for joining this thread -- good to see you here.
    
    In short, as you probably know, Spark needs to serialize lambdas to execute them remotely. Sometimes, the lambda transitively refers to some value somewhere that isn't needed and isn't serializable either -- I think this mostly arises because the inner class implementing the closure refers to the outer class, containing lots of things like a SparkContext.
    
    The existing code has a pretty good process for finding out what fields the actual byte code references, and then nulling out references that aren't required.
    
    That implementation changes in Scala 2.12 of course to use the Java 8 lambda mechanism. Yes, it's not specific to Scala. It's just a matter of being able to modify the implementation of the closure at runtime, in the same way it was done with anonymous classes before.
    
    Would `-Ydelambdafy:inline` basically go back to implementing closures as anonymous classes? That is probably, at least, a workaround, though one that requires specific behavior on the user end.
    
    I didn't get too far investigating what the equivalent implementation is in Java 8 lambdas. I believe it involves working with MethodHandles et al but I had punted when I looked at it, as I don't yet know much about how this works.
    
    The good news was that, well, everything else worked with Scala 2.12. And, almost all tests pass in Scala 2.12 even without a 'fix' for the above. It kind of made sense to me as the original problem arose, almost entirely, from an artifact of how non-anonymous inner classes work. And they're not inner classes anymore. There may be a narrow, simple, different thing that would resolve the remaining test failures.
    
    One can try it by just setting up the build for 2.12 with `./dev/change-scala-version 2.12` and building and testing with `-Pscala-2.12`. Unless something else has re-broken since then, one ought to find it compiles and passes most tests. The few that are left are either testing the old closure-cleaning mechanism, or seem to be the corner cases that are still issues.
    
    That's where I left it after I finished fixing up the rest for 2.3.0 and hadn't been back since.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19675: [SPARK-14540][BUILD] Support Scala 2.12 closures and Jav...

Posted by jvican <gi...@git.apache.org>.
Github user jvican commented on the issue:

    https://github.com/apache/spark/pull/19675
  
    Is there any ticket that describes what needs to be done? Otherwise, could you elaborate on it a that I have a clearer idea of what you need to have a 100% working 2.12-compatible Spark? Is the 1% of test failures the only thing preventing a release?


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org