You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "LuciferYang (via GitHub)" <gi...@apache.org> on 2025/05/27 16:53:01 UTC
Re: [PR] [SPARK-52088][CORE] Refactor the `ClosureCleaner#clean` and avoid using the `-Djdk.reflect.useDirectMethodHandle=false` [spark]
LuciferYang commented on code in PR #51028:
URL: https://github.com/apache/spark/pull/51028#discussion_r2109699439
##########
common/utils/src/main/scala/org/apache/spark/util/ClosureCleaner.scala:
##########
@@ -499,72 +503,45 @@ private[spark] object ClosureCleaner extends Logging {
s"${accessedAmmCmdFields.size} classes")
accessedAmmCmdFields.foreach { f => logTrace(" " + f) }
- val cmdClones = Map[Class[_], AnyRef]()
- for ((cmdClass, _) <- ammCmdInstances if !cmdClass.getName.contains("Helper")) {
- logDebug(s" + Cloning instance of Ammonite command class ${cmdClass.getName}")
- cmdClones(cmdClass) = instantiateClass(cmdClass, enclosingObject = null)
- }
- for ((cmdHelperClass, cmdHelperInstance) <- ammCmdInstances
- if cmdHelperClass.getName.contains("Helper")) {
- val cmdHelperOuter = cmdHelperClass.getDeclaredFields
- .find(_.getName == "$outer")
- .map { field =>
- field.setAccessible(true)
- field.get(cmdHelperInstance)
- }
- val outerClone = cmdHelperOuter.flatMap(o => cmdClones.get(o.getClass)).orNull
- logDebug(s" + Cloning instance of Ammonite command helper class ${cmdHelperClass.getName}")
- cmdClones(cmdHelperClass) =
- instantiateClass(cmdHelperClass, enclosingObject = outerClone)
- }
+ // Instead of cloning and trying to update lambda reference, directly modify the original
+ // REPL object to null out unused fields. This works because REPL objects are not hidden
+ // classes and their fields can be modified.
+ val capturingClass = outerThis.getClass
+ if (accessedFields(capturingClass).size < capturingClass.getDeclaredFields.length) {
+ val fieldsToNull = capturingClass.getDeclaredFields.filterNot { field =>
+ accessedFields(capturingClass).contains(field.getName)
+ }
- // set accessed fields
- for ((_, cmdClone) <- cmdClones) {
- val cmdClass = cmdClone.getClass
- val accessedFields = accessedAmmCmdFields(cmdClass)
- for (field <- cmdClone.getClass.getDeclaredFields
- // outer fields were initialized during clone construction
- if accessedFields.contains(field.getName) && field.getName != "$outer") {
- // get command clone if exists, otherwise use an original field value
- val value = cmdClones.getOrElse(field.getType, {
+ for (field <- fieldsToNull) {
+ try {
field.setAccessible(true)
- field.get(ammCmdInstances(cmdClass))
- })
- setFieldAndIgnoreModifiers(cmdClone, field, value)
+ field.set(outerThis, null)
+ } catch {
+ case _: Exception =>
+ // Ignore failures to set fields - this is a best-effort cleanup
+ }
}
}
- val outerThisClone = if (!isAmmoniteCommandOrHelper(outerThis.getClass)) {
- // if outer class is not Ammonite helper / command object then is was not cloned
- // in the code above. We still need to clone it and update accessed fields
- logDebug(s" + Cloning instance of lambda capturing class ${outerThis.getClass.getName}")
- val clone = cloneAndSetFields(parent = null, outerThis, outerThis.getClass, accessedFields)
- // making sure that the code below will update references to Ammonite objects if they exist
- for (field <- outerThis.getClass.getDeclaredFields) {
- field.setAccessible(true)
- cmdClones.get(field.getType).foreach { value =>
- setFieldAndIgnoreModifiers(clone, field, value)
+ // Also clean up Ammonite command fields if any
+ for ((cmdClass, cmdInstance) <- ammCmdInstances) {
+ val cmdAccessedFields = accessedAmmCmdFields.getOrElse(cmdClass, Set.empty)
+ if (cmdAccessedFields.size < cmdClass.getDeclaredFields.length) {
+ val fieldsToNull = cmdClass.getDeclaredFields.filterNot { field =>
+ cmdAccessedFields.contains(field.getName) || field.getName == "$outer"
+ }
+
+ for (field <- fieldsToNull) {
+ try {
+ field.setAccessible(true)
+ field.set(cmdInstance, null)
+ } catch {
+ case _: Exception =>
+ // Ignore failures to set fields - this is a best-effort cleanup
Review Comment:
ditto
##########
common/utils/src/main/scala/org/apache/spark/util/ClosureCleaner.scala:
##########
@@ -418,22 +418,26 @@ private[spark] object ClosureCleaner extends Logging {
logDebug(s" + fields accessed by starting closure: ${accessedFields.size} classes")
accessedFields.foreach { f => logDebug(" " + f) }
+ // Instead of cloning and trying to update lambda reference, directly modify the original
+ // REPL object to null out unused fields. This works because REPL objects are not hidden
+ // classes and their fields can be modified.
if (accessedFields(capturingClass).size < capturingClass.getDeclaredFields.length) {
- // clone and clean the enclosing `this` only when there are fields to null out
- logDebug(s" + cloning instance of REPL class ${capturingClass.getName}")
- val clonedOuterThis = cloneAndSetFields(
- parent = null, outerThis, capturingClass, accessedFields)
-
- val outerField = func.getClass.getDeclaredField("arg$1")
- // SPARK-37072: When Java 17 is used and `outerField` is read-only,
- // the content of `outerField` cannot be set by reflect api directly.
- // But we can remove the `final` modifier of `outerField` before set value
- // and reset the modifier after set value.
- setFieldAndIgnoreModifiers(func, outerField, clonedOuterThis)
+ val fieldsToNull = capturingClass.getDeclaredFields.filterNot { field =>
+ accessedFields(capturingClass).contains(field.getName)
+ }
+
+ for (field <- fieldsToNull) {
+ try {
+ field.setAccessible(true)
+ field.set(outerThis, null)
+ } catch {
+ case _: Exception =>
+ // Ignore failures to set fields - this is a best-effort cleanup
Review Comment:
Logs should be added
##########
.github/workflows/build_and_test.yml:
##########
@@ -25,7 +25,7 @@ on:
java:
required: false
type: string
- default: 17
+ default: 21
Review Comment:
The modifications made to this file need to be reverted before merging.
##########
common/utils/src/main/scala/org/apache/spark/util/ClosureCleaner.scala:
##########
@@ -499,72 +503,45 @@ private[spark] object ClosureCleaner extends Logging {
s"${accessedAmmCmdFields.size} classes")
accessedAmmCmdFields.foreach { f => logTrace(" " + f) }
- val cmdClones = Map[Class[_], AnyRef]()
- for ((cmdClass, _) <- ammCmdInstances if !cmdClass.getName.contains("Helper")) {
- logDebug(s" + Cloning instance of Ammonite command class ${cmdClass.getName}")
- cmdClones(cmdClass) = instantiateClass(cmdClass, enclosingObject = null)
- }
- for ((cmdHelperClass, cmdHelperInstance) <- ammCmdInstances
- if cmdHelperClass.getName.contains("Helper")) {
- val cmdHelperOuter = cmdHelperClass.getDeclaredFields
- .find(_.getName == "$outer")
- .map { field =>
- field.setAccessible(true)
- field.get(cmdHelperInstance)
- }
- val outerClone = cmdHelperOuter.flatMap(o => cmdClones.get(o.getClass)).orNull
- logDebug(s" + Cloning instance of Ammonite command helper class ${cmdHelperClass.getName}")
- cmdClones(cmdHelperClass) =
- instantiateClass(cmdHelperClass, enclosingObject = outerClone)
- }
+ // Instead of cloning and trying to update lambda reference, directly modify the original
+ // REPL object to null out unused fields. This works because REPL objects are not hidden
+ // classes and their fields can be modified.
+ val capturingClass = outerThis.getClass
+ if (accessedFields(capturingClass).size < capturingClass.getDeclaredFields.length) {
+ val fieldsToNull = capturingClass.getDeclaredFields.filterNot { field =>
+ accessedFields(capturingClass).contains(field.getName)
+ }
- // set accessed fields
- for ((_, cmdClone) <- cmdClones) {
- val cmdClass = cmdClone.getClass
- val accessedFields = accessedAmmCmdFields(cmdClass)
- for (field <- cmdClone.getClass.getDeclaredFields
- // outer fields were initialized during clone construction
- if accessedFields.contains(field.getName) && field.getName != "$outer") {
- // get command clone if exists, otherwise use an original field value
- val value = cmdClones.getOrElse(field.getType, {
+ for (field <- fieldsToNull) {
+ try {
field.setAccessible(true)
- field.get(ammCmdInstances(cmdClass))
- })
- setFieldAndIgnoreModifiers(cmdClone, field, value)
+ field.set(outerThis, null)
+ } catch {
+ case _: Exception =>
+ // Ignore failures to set fields - this is a best-effort cleanup
Review Comment:
ditto
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org