sarutak commented on code in PR #57710:
URL: https://github.com/apache/spark/pull/57710#discussion_r3703492548
##########
common/utils/src/main/scala/org/apache/spark/util/ClosureCleaner.scala:
##########
@@ -34,6 +35,39 @@ import org.apache.spark.internal.Logging
* A cleaner that renders closures serializable if they can be done so safely.
*/
private[spark] object ClosureCleaner extends Logging {
+ /**
+ * Per-class memo of which closure methods contain a non-local return, i.e.
allocate a
+ * `scala/runtime/NonLocalReturnControl`. The verdict is a pure function of
the class's
+ * immutable bytecode, so one ASM parse per class answers for every
`clean()` call.
+ */
+ private val methodsWithNonLocalReturn = new
ClassValue[immutable.Set[String]] {
+ override def computeValue(cls: Class[_]): immutable.Set[String] = {
+ val collector = new ReturnStatementCollector
+ val reader = getClassReader(cls)
+ if (reader != null) {
+ reader.accept(collector, 0)
+ } else {
+ logDebug(s"Cannot get class bytes for ${cls.getName}; skipping
return-statement check")
+ }
+ collector.found.toSet
+ }
+ }
+
+ /** Whether `implMethodName` (any closure method, if `None`) of `cls` has a
non-local return. */
+ private[util] def hasReturnStatement(cls: Class[_], implMethodName:
Option[String]): Boolean = {
+ val found = methodsWithNonLocalReturn.get(cls)
+ implMethodName match {
+ case None => found.nonEmpty
+ case Some(target) =>
+ // A method with suffix "$adapted" will be generated in cases like
+ // { _:Int => return; Seq()} but not { _:Int => return; true}
+ // closure passed is $anonfun$t$1$adapted while actual code resides in
$anonfun$s$1
+ // the class file only contains $anonfun$s$1, so we remove the suffix,
see
Review Comment:
I understand this comment was carried over from the existing code, but the
statement "the class file only contains `$anonfun$s$1"` appears inaccurate.
Given `TestAdapter` as follows, `javap` shows that both `$anonfun$main$1` and
`$anonfun$main$1$adapted` exist in the class file.
```scala
object TestAdapted {
def main(args: Array[String]): Unit = {
// This should produce $anonfun$main$1 and possibly
$anonfun$main$1$adapted
val f: Int => Seq[Int] = { _: Int => return; Seq() }
f(1)
}
}
```
```
$ javap -c TestAdapted$
...
public static final scala.collection.immutable.Seq
$anonfun$main$1(java.lang.Object, int);
Code:
0: new #77 // class
scala/runtime/NonLocalReturnControl$mcV$sp
3: dup
4: aload_0
5: getstatic #83 // Field
scala/runtime/BoxedUnit.UNIT:Lscala/runtime/BoxedUnit;
8: invokespecial #86 // Method
scala/runtime/NonLocalReturnControl$mcV$sp."<init>":(Ljava/lang/Object;Lscala/runtime/BoxedUnit;)V
11: athrow
public static final scala.collection.immutable.Seq
$anonfun$main$1$adapted(java.lang.Object, java.lang.Object);
Code:
0: aload_0
1: aload_1
2: invokestatic #92 // Method
scala/runtime/BoxesRunTime.unboxToInt:(Ljava/lang/Object;)I
5: invokestatic #94 // Method
$anonfun$main$1:(Ljava/lang/Object;I)Lscala/collection/immutable/Seq;
8: areturn
```
The actual reason for stripping the `$adapted` suffix is that the `NEW
NonLocalReturnControl` bytecode only appears in the non-adapted method body
(the adapted variant is a boxing bridge that delegates to it). Might be worth
fixing the comment while you're here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
