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 2018/11/09 17:11:56 UTC
[GitHub] spark pull request #22993: [SPARK-24421][BUILD][CORE] Accessing sun.misc.Cle...
GitHub user srowen opened a pull request:
https://github.com/apache/spark/pull/22993
[SPARK-24421][BUILD][CORE] Accessing sun.misc.Cleaner in JDK11
…. Other related changes to get JDK 11 working, to test
## What changes were proposed in this pull request?
- Access `sun.misc.Cleaner` (Java 8) and `jdk.internal.ref.Cleaner` (JDK 9+) by reflection (note: the latter only works if illegal reflective access is allowed)
- Access `sun.misc.Unsafe.invokeCleaner` in Java 9+ instead of `sun.misc.Cleaner` (Java 8)
In order to test anything on JDK 11, I also fixed a few small things, which I include here:
- Fix minor JDK 11 compile issues
- Update scala plugin, Jetty for JDK 11, to facilitate tests too
This doesn't mean JDK 11 tests all pass now, but lots do. Note also that the JDK 9+ solution for the Cleaner has a big caveat.
## How was this patch tested?
Existing tests. Manually tested JDK 11 build and tests, and tests covering this change appear to pass. All Java 8 tests should still pass, but this change alone does not achieve full JDK 11 compatibility.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/srowen/spark SPARK-24421
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/22993.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 #22993
----
commit ebf280d8c577f697ba679ec1f76296b524f23a23
Author: Sean Owen <se...@...>
Date: 2018-11-09T17:07:33Z
Avoid sun.misc.Cleaner in JDK 9+ while retaining Java 8 compatibility. Other related changes to get JDK 11 working, to test
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22993: [SPARK-24421][BUILD][CORE] Accessing sun.misc.Cleaner in...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22993
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 pull request #22993: [SPARK-24421][BUILD][CORE] Accessing sun.misc.Cle...
Posted by skonto <gi...@git.apache.org>.
Github user skonto commented on a diff in the pull request:
https://github.com/apache/spark/pull/22993#discussion_r232847703
--- Diff: core/src/main/scala/org/apache/spark/storage/StorageUtils.scala ---
@@ -193,6 +194,36 @@ private[spark] class StorageStatus(
/** Helper methods for storage-related objects. */
private[spark] object StorageUtils extends Logging {
+
+ // In Java 8, the type of DirectBuffer.cleaner() was sun.misc.Cleaner, and it was possible
+ // to access the method sun.misc.Cleaner.clean() to invoke it. The type changed to
+ // jdk.internal.ref.Cleaner in later JDKs, and the .clean() method is not accessible even with
+ // reflection. However sun.misc.Unsafe added a invokeCleaner() method in JDK 9+ and this is
+ // still accessible with reflection.
+ private val bufferCleaner: DirectBuffer => Unit =
+ // Split java.version on non-digit chars:
+ if (System.getProperty("java.version").split("\\D+").head.toInt < 9) {
--- End diff --
Is this java detection method used elsewhere? Maybe other parts of the codebase could benefit from this.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22993: [SPARK-24421][BUILD][CORE] Accessing sun.misc.Cle...
Posted by felixcheung <gi...@git.apache.org>.
Github user felixcheung commented on a diff in the pull request:
https://github.com/apache/spark/pull/22993#discussion_r232477875
--- Diff: common/unsafe/src/main/java/org/apache/spark/unsafe/Platform.java ---
@@ -67,6 +67,59 @@
unaligned = _unaligned;
}
+ // Access fields and constructors once and store them, for performance:
+
+ private static final Constructor<?> DBB_CONSTRUCTOR;
+ private static final Field DBB_CLEANER_FIELD;
+ static {
+ try {
+ Class<?> cls = Class.forName("java.nio.DirectByteBuffer");
+ Constructor<?> constructor = cls.getDeclaredConstructor(Long.TYPE, Integer.TYPE);
+ constructor.setAccessible(true);
+ Field cleanerField = cls.getDeclaredField("cleaner");
+ cleanerField.setAccessible(true);
+ DBB_CONSTRUCTOR = constructor;
+ DBB_CLEANER_FIELD = cleanerField;
+ } catch (ClassNotFoundException | NoSuchMethodException | NoSuchFieldException e) {
+ throw new IllegalStateException(e);
+ }
+ }
+
+ private static final Method CLEANER_CREATE_METHOD;
+ static {
+ // The implementation of Cleaner changed from JDK 8 to 9
+ int majorVersion = Integer.parseInt(System.getProperty("java.version").split("\\.")[0]);
--- End diff --
is there a defined fixed format for this?
we are doing some java version check and found very different format from different JDK sources (Oracle vs OpenJDK vs IBM ...)
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22993: [SPARK-24421][BUILD][CORE] Accessing sun.misc.Cle...
Posted by skonto <gi...@git.apache.org>.
Github user skonto commented on a diff in the pull request:
https://github.com/apache/spark/pull/22993#discussion_r232980913
--- Diff: core/src/main/scala/org/apache/spark/storage/StorageUtils.scala ---
@@ -193,6 +194,36 @@ private[spark] class StorageStatus(
/** Helper methods for storage-related objects. */
private[spark] object StorageUtils extends Logging {
+
+ // In Java 8, the type of DirectBuffer.cleaner() was sun.misc.Cleaner, and it was possible
+ // to access the method sun.misc.Cleaner.clean() to invoke it. The type changed to
+ // jdk.internal.ref.Cleaner in later JDKs, and the .clean() method is not accessible even with
+ // reflection. However sun.misc.Unsafe added a invokeCleaner() method in JDK 9+ and this is
+ // still accessible with reflection.
+ private val bufferCleaner: DirectBuffer => Unit =
+ // Split java.version on non-digit chars:
+ if (System.getProperty("java.version").split("\\D+").head.toInt < 9) {
--- End diff --
I think so how about then:
import org.apache.commons.lang3.JavaVersion
SystemUtils.isJavaVersionAtLeast(JavaVersion.JAVA_9)
We include that package anyway, and that method is based on that property:
https://commons.apache.org/proper/commons-lang/apidocs/org/apache/commons/lang3/JavaVersion.html
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22993: [SPARK-24421][BUILD][CORE] Accessing sun.misc.Cleaner in...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22993
**[Test build #98804 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/98804/testReport)** for PR 22993 at commit [`2ee58b2`](https://github.com/apache/spark/commit/2ee58b2ba66fbf497e819feff9beb786d4884acd).
* This patch passes all 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 #22993: [SPARK-24421][BUILD][CORE] Accessing sun.misc.Cleaner in...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22993
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/4929/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22993: [SPARK-24421][BUILD][CORE] Accessing sun.misc.Cleaner in...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22993
**[Test build #98653 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/98653/testReport)** for PR 22993 at commit [`ebf280d`](https://github.com/apache/spark/commit/ebf280d8c577f697ba679ec1f76296b524f23a23).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22993: [SPARK-24421][BUILD][CORE] Accessing sun.misc.Cleaner in...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22993
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/4890/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22993: [SPARK-24421][BUILD][CORE] Accessing sun.misc.Cle...
Posted by skonto <gi...@git.apache.org>.
Github user skonto commented on a diff in the pull request:
https://github.com/apache/spark/pull/22993#discussion_r232852693
--- Diff: common/unsafe/src/main/java/org/apache/spark/unsafe/Platform.java ---
@@ -67,6 +67,59 @@
unaligned = _unaligned;
}
+ // Access fields and constructors once and store them, for performance:
+
+ private static final Constructor<?> DBB_CONSTRUCTOR;
+ private static final Field DBB_CLEANER_FIELD;
+ static {
+ try {
+ Class<?> cls = Class.forName("java.nio.DirectByteBuffer");
+ Constructor<?> constructor = cls.getDeclaredConstructor(Long.TYPE, Integer.TYPE);
+ constructor.setAccessible(true);
+ Field cleanerField = cls.getDeclaredField("cleaner");
+ cleanerField.setAccessible(true);
+ DBB_CONSTRUCTOR = constructor;
+ DBB_CLEANER_FIELD = cleanerField;
+ } catch (ClassNotFoundException | NoSuchMethodException | NoSuchFieldException e) {
+ throw new IllegalStateException(e);
+ }
+ }
+
+ private static final Method CLEANER_CREATE_METHOD;
+ static {
+ // The implementation of Cleaner changed from JDK 8 to 9
+ int majorVersion = Integer.parseInt(System.getProperty("java.version").split("\\.")[0]);
--- End diff --
How about using java.specification.version and parse it as double? Then check if greater than 2.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22993: [SPARK-24421][BUILD][CORE] Accessing sun.misc.Cleaner in...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22993
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 #22993: [SPARK-24421][BUILD][CORE] Accessing sun.misc.Cleaner in...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22993
**[Test build #98700 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/98700/testReport)** for PR 22993 at commit [`7f58ae6`](https://github.com/apache/spark/commit/7f58ae61262d7c2f2d70c24d051c63e8830d5062).
* This patch passes all 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 #22993: [SPARK-24421][BUILD][CORE] Accessing sun.misc.Cleaner in...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22993
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 pull request #22993: [SPARK-24421][BUILD][CORE] Accessing sun.misc.Cle...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/22993#discussion_r232500600
--- Diff: common/unsafe/src/main/java/org/apache/spark/unsafe/Platform.java ---
@@ -67,6 +67,59 @@
unaligned = _unaligned;
}
+ // Access fields and constructors once and store them, for performance:
+
+ private static final Constructor<?> DBB_CONSTRUCTOR;
+ private static final Field DBB_CLEANER_FIELD;
+ static {
+ try {
+ Class<?> cls = Class.forName("java.nio.DirectByteBuffer");
+ Constructor<?> constructor = cls.getDeclaredConstructor(Long.TYPE, Integer.TYPE);
+ constructor.setAccessible(true);
+ Field cleanerField = cls.getDeclaredField("cleaner");
+ cleanerField.setAccessible(true);
+ DBB_CONSTRUCTOR = constructor;
+ DBB_CLEANER_FIELD = cleanerField;
+ } catch (ClassNotFoundException | NoSuchMethodException | NoSuchFieldException e) {
+ throw new IllegalStateException(e);
+ }
+ }
+
+ private static final Method CLEANER_CREATE_METHOD;
+ static {
+ // The implementation of Cleaner changed from JDK 8 to 9
+ int majorVersion = Integer.parseInt(System.getProperty("java.version").split("\\.")[0]);
--- End diff --
That's right, but Java versions <= 8 are always 1.8, 1.7, etc. We're just detecting Java 9+ here, and for Java 8 and earlier, the first digit is 1 and so will compare as less than 9.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22993: [SPARK-24421][BUILD][CORE] Accessing sun.misc.Cleaner in...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22993
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 pull request #22993: [SPARK-24421][BUILD][CORE] Accessing sun.misc.Cle...
Posted by kiszk <gi...@git.apache.org>.
Github user kiszk commented on a diff in the pull request:
https://github.com/apache/spark/pull/22993#discussion_r232488912
--- Diff: common/unsafe/src/main/java/org/apache/spark/unsafe/Platform.java ---
@@ -67,6 +67,59 @@
unaligned = _unaligned;
}
+ // Access fields and constructors once and store them, for performance:
+
+ private static final Constructor<?> DBB_CONSTRUCTOR;
+ private static final Field DBB_CLEANER_FIELD;
+ static {
+ try {
+ Class<?> cls = Class.forName("java.nio.DirectByteBuffer");
+ Constructor<?> constructor = cls.getDeclaredConstructor(Long.TYPE, Integer.TYPE);
+ constructor.setAccessible(true);
+ Field cleanerField = cls.getDeclaredField("cleaner");
+ cleanerField.setAccessible(true);
+ DBB_CONSTRUCTOR = constructor;
+ DBB_CLEANER_FIELD = cleanerField;
+ } catch (ClassNotFoundException | NoSuchMethodException | NoSuchFieldException e) {
+ throw new IllegalStateException(e);
+ }
+ }
+
+ private static final Method CLEANER_CREATE_METHOD;
+ static {
+ // The implementation of Cleaner changed from JDK 8 to 9
+ int majorVersion = Integer.parseInt(System.getProperty("java.version").split("\\.")[0]);
--- End diff --
From Java 9, here is a [new definition](https://docs.oracle.com/javase/9/migrate/toc.htm#JSMIG-GUID-3A71ECEF-5FC5-46FE-9BA9-88CBFCE828CB).
I confirmed it can work for OpenJDK, OpenJ9, and IBM JDK 8 by running the following code
```
public class Version {
public static void main(String[] args){
System.out.println("jave.specification.version=" + System.getProperty("java.specification.version"));
System.out.println("jave.version=" + System.getProperty("java.version"));
System.out.println("jave.version.split(\".\")[0]=" + System.getProperty("java.version").split("\\.")[0]);
}
}
```
OpenJDK
```
$ ../OpenJDK-8/java -version
java version "1.8.0_162"
Java(TM) SE Runtime Environment (build 1.8.0_162-b12)
Java HotSpot(TM) 64-Bit Server VM (build 25.162-b12, mixed mode)
$ ../OpenJDK-8/java Version
jave.specification.version=1.8
jave.version=1.8.0_162
jave.version.split(".")[0]=1
$ ../OpenJDK-9/java -version
openjdk version "9"
OpenJDK Runtime Environment (build 9+181)
OpenJDK 64-Bit Server VM (build 9+181, mixed mode)
$ ../OpenJDK-9/java Version
jave.specification.version=9
jave.version=9
jave.version.split(".")[0]=9
$ ../OpenJDK-11/java -version
openjdk version "11.0.1" 2018-10-16
OpenJDK Runtime Environment 18.9 (build 11.0.1+13)
OpenJDK 64-Bit Server VM 18.9 (build 11.0.1+13, mixed mode)
$ ../OpenJDK-11/java Version
jave.specification.version=11
jave.version=11.0.1
jave.version.split(".")[0]=11
```
OpenJ9
```
$ ../OpenJ9-8/java -version
openjdk version "1.8.0_192"
OpenJDK Runtime Environment (build 1.8.0_192-b12)
Eclipse OpenJ9 VM (build openj9-0.11.0, JRE 1.8.0 Windows 10 amd64-64-Bit Compressed References 20181019_105 (JIT enabled, AOT enabled)
OpenJ9 - 090ff9dc
OMR - ea548a66
JCL - 51609250b5 based on jdk8u192-b12)
$ ../OpenJ9-8/java Version
jave.specification.version=1.8
jave.version=1.8.0_192
jave.version.split(".")[0]=1
$ ../OpenJ9-9/java -version
openjdk version "9.0.4-adoptopenjdk"
OpenJDK Runtime Environment (build 9.0.4-adoptopenjdk+12)
Eclipse OpenJ9 VM (build openj9-0.9.0, JRE 9 Windows 8.1 amd64-64-Bit Compressed References 20180814_161 (JIT enabled, AOT enabled)
OpenJ9 - 24e53631
OMR - fad6bf6e
JCL - feec4d2ae based on jdk-9.0.4+12)
$ ../OpenJ9-9/java Version
jave.specification.version=9
jave.version=9.0.4-adoptopenjdk
jave.version.split(".")[0]=9
$ ../OpenJ9-11/java -version
openjdk version "11.0.1" 2018-10-16
OpenJDK Runtime Environment AdoptOpenJDK (build 11.0.1+13)
Eclipse OpenJ9 VM AdoptOpenJDK (build openj9-0.11.0, JRE 11 Windows 10 amd64-64-Bit Compressed References 20181020_83 (JIT enabled, AOT enabled)
OpenJ9 - 090ff9dc
OMR - ea548a66
JCL - f62696f378 based on jdk-11.0.1+13)
$ ../OpenJ9-11/java Version
jave.specification.version=11
jave.version=11.0.1
jave.version.split(".")[0]=11
```
IBM JDK
```
$ ../IBMJDK-8/java -version
java version "1.8.0"
Java(TM) SE Runtime Environment (build pwa6480-20150129_02)
IBM J9 VM (build 2.8, JRE 1.8.0 Windows 8.1 amd64-64 Compressed References 20150116_231420 (JIT enabled, AOT enabled)
J9VM - R28_Java8_GA_20150116_2030_B231420
JIT - tr.r14.java_20150109_82886.02
GC - R28_Java8_GA_20150116_2030_B231420_CMPRSS
J9CL - 20150116_231420)
JCL - 20150123_01 based on Oracle jdk8u31-b12
$ ../IBMJDK-8/java Version
jave.specification.version=1.8
jave.version=1.8.0
jave.version.split(".")[0]=1
```
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22993: [SPARK-24421][BUILD][CORE] Accessing sun.misc.Cle...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/22993#discussion_r233124864
--- Diff: core/src/main/scala/org/apache/spark/storage/StorageUtils.scala ---
@@ -193,6 +194,36 @@ private[spark] class StorageStatus(
/** Helper methods for storage-related objects. */
private[spark] object StorageUtils extends Logging {
+
+ // In Java 8, the type of DirectBuffer.cleaner() was sun.misc.Cleaner, and it was possible
+ // to access the method sun.misc.Cleaner.clean() to invoke it. The type changed to
+ // jdk.internal.ref.Cleaner in later JDKs, and the .clean() method is not accessible even with
+ // reflection. However sun.misc.Unsafe added a invokeCleaner() method in JDK 9+ and this is
+ // still accessible with reflection.
+ private val bufferCleaner: DirectBuffer => Unit =
+ // Split java.version on non-digit chars:
+ if (System.getProperty("java.version").split("\\D+").head.toInt < 9) {
--- End diff --
That's fine. We can't use it in Platform, and have to make sure this dependency is managed up to a version that handles later Java versions (may already be). So we have diverging approaches for related code here, but, they don't strictly do the same thing nor do the two changes here have to go hand-in-hand. I'm not against it either but that's why I was hesitant about it.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22993: [SPARK-24421][BUILD][CORE] Accessing sun.misc.Cle...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/22993#discussion_r232849634
--- Diff: core/src/main/scala/org/apache/spark/storage/StorageUtils.scala ---
@@ -193,6 +194,36 @@ private[spark] class StorageStatus(
/** Helper methods for storage-related objects. */
private[spark] object StorageUtils extends Logging {
+
+ // In Java 8, the type of DirectBuffer.cleaner() was sun.misc.Cleaner, and it was possible
+ // to access the method sun.misc.Cleaner.clean() to invoke it. The type changed to
+ // jdk.internal.ref.Cleaner in later JDKs, and the .clean() method is not accessible even with
+ // reflection. However sun.misc.Unsafe added a invokeCleaner() method in JDK 9+ and this is
+ // still accessible with reflection.
+ private val bufferCleaner: DirectBuffer => Unit =
+ // Split java.version on non-digit chars:
+ if (System.getProperty("java.version").split("\\D+").head.toInt < 9) {
--- End diff --
In the Platform.java class we can't rely on any other libs or methods, so I wrote something like this. I just followed suit here. My reasoning was that this is a simple check of major version number, specific to 8 vs 9. I don't mind using a library method here, but we can't in the same context in Platform.java.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22993: [SPARK-24421][BUILD][CORE] Accessing sun.misc.Cle...
Posted by skonto <gi...@git.apache.org>.
Github user skonto commented on a diff in the pull request:
https://github.com/apache/spark/pull/22993#discussion_r232846926
--- Diff: common/unsafe/src/main/java/org/apache/spark/unsafe/Platform.java ---
@@ -67,6 +67,60 @@
unaligned = _unaligned;
}
+ // Access fields and constructors once and store them, for performance:
+
+ private static final Constructor<?> DBB_CONSTRUCTOR;
+ private static final Field DBB_CLEANER_FIELD;
+ static {
+ try {
+ Class<?> cls = Class.forName("java.nio.DirectByteBuffer");
+ Constructor<?> constructor = cls.getDeclaredConstructor(Long.TYPE, Integer.TYPE);
+ constructor.setAccessible(true);
+ Field cleanerField = cls.getDeclaredField("cleaner");
+ cleanerField.setAccessible(true);
+ DBB_CONSTRUCTOR = constructor;
+ DBB_CLEANER_FIELD = cleanerField;
+ } catch (ClassNotFoundException | NoSuchMethodException | NoSuchFieldException e) {
+ throw new IllegalStateException(e);
+ }
+ }
+
+ private static final Method CLEANER_CREATE_METHOD;
+ static {
+ // The implementation of Cleaner changed from JDK 8 to 9
--- End diff --
is this same in java 11? Why do we need to reference java 9, its dead right and it will have no LTS AFAIK.
Shouldnt we only support LTS?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22993: [SPARK-24421][BUILD][CORE] Accessing sun.misc.Cleaner in...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22993
**[Test build #98686 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/98686/testReport)** for PR 22993 at commit [`2db719c`](https://github.com/apache/spark/commit/2db719c2896c96793dfc96791bbadb2bc8a3f4ab).
* This patch passes all 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 #22993: [SPARK-24421][BUILD][CORE] Accessing sun.misc.Cleaner in...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22993
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/4891/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22993: [SPARK-24421][BUILD][CORE] Accessing sun.misc.Cleaner in...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22993
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/4917/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22993: [SPARK-24421][BUILD][CORE] Accessing sun.misc.Cle...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/22993#discussion_r232492843
--- Diff: common/unsafe/src/main/java/org/apache/spark/unsafe/Platform.java ---
@@ -67,6 +67,59 @@
unaligned = _unaligned;
}
+ // Access fields and constructors once and store them, for performance:
+
+ private static final Constructor<?> DBB_CONSTRUCTOR;
+ private static final Field DBB_CLEANER_FIELD;
+ static {
+ try {
+ Class<?> cls = Class.forName("java.nio.DirectByteBuffer");
+ Constructor<?> constructor = cls.getDeclaredConstructor(Long.TYPE, Integer.TYPE);
+ constructor.setAccessible(true);
+ Field cleanerField = cls.getDeclaredField("cleaner");
+ cleanerField.setAccessible(true);
+ DBB_CONSTRUCTOR = constructor;
+ DBB_CLEANER_FIELD = cleanerField;
+ } catch (ClassNotFoundException | NoSuchMethodException | NoSuchFieldException e) {
+ throw new IllegalStateException(e);
+ }
+ }
+
+ private static final Method CLEANER_CREATE_METHOD;
+ static {
+ // The implementation of Cleaner changed from JDK 8 to 9
+ int majorVersion = Integer.parseInt(System.getProperty("java.version").split("\\.")[0]);
--- End diff --
This should be safe, but I recall that some early-access Java 9 builds had a version like "9-ea". Maybe I should make the regex grab the leading digits only to be safe.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22993: [SPARK-24421][BUILD][CORE] Accessing sun.misc.Cleaner in...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22993
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 pull request #22993: [SPARK-24421][BUILD][CORE] Accessing sun.misc.Cle...
Posted by skonto <gi...@git.apache.org>.
Github user skonto commented on a diff in the pull request:
https://github.com/apache/spark/pull/22993#discussion_r233216872
--- Diff: core/src/main/scala/org/apache/spark/storage/StorageUtils.scala ---
@@ -193,6 +194,36 @@ private[spark] class StorageStatus(
/** Helper methods for storage-related objects. */
private[spark] object StorageUtils extends Logging {
+
+ // In Java 8, the type of DirectBuffer.cleaner() was sun.misc.Cleaner, and it was possible
+ // to access the method sun.misc.Cleaner.clean() to invoke it. The type changed to
+ // jdk.internal.ref.Cleaner in later JDKs, and the .clean() method is not accessible even with
+ // reflection. However sun.misc.Unsafe added a invokeCleaner() method in JDK 9+ and this is
+ // still accessible with reflection.
+ private val bufferCleaner: DirectBuffer => Unit =
+ // Split java.version on non-digit chars:
+ if (System.getProperty("java.version").split("\\D+").head.toInt < 9) {
+ // scalastyle:off classforname
+ val cleanerMethod = Class.forName("sun.misc.Cleaner").getMethod("clean")
+ // scalastyle:on classforname
+ (buffer: DirectBuffer) => {
+ // Careful to avoid the return type of .cleaner(), which changes with JDK
+ val cleaner: AnyRef = buffer.cleaner()
+ if (cleaner != null) {
+ cleanerMethod.invoke(cleaner)
+ }
+ }
+ } else {
+ // scalastyle:off classforname
+ val cleanerMethod =
+ Class.forName("sun.misc.Unsafe").getMethod("invokeCleaner", classOf[ByteBuffer])
--- End diff --
ok, in general I cant think of something failing at the moment, just wanted to provide some feedback that this mixes strategies.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22993: [SPARK-24421][BUILD][CORE] Accessing sun.misc.Cleaner in...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22993
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 pull request #22993: [SPARK-24421][BUILD][CORE] Accessing sun.misc.Cle...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/22993#discussion_r232848389
--- Diff: common/unsafe/src/main/java/org/apache/spark/unsafe/Platform.java ---
@@ -67,6 +67,60 @@
unaligned = _unaligned;
}
+ // Access fields and constructors once and store them, for performance:
+
+ private static final Constructor<?> DBB_CONSTRUCTOR;
+ private static final Field DBB_CLEANER_FIELD;
+ static {
+ try {
+ Class<?> cls = Class.forName("java.nio.DirectByteBuffer");
+ Constructor<?> constructor = cls.getDeclaredConstructor(Long.TYPE, Integer.TYPE);
+ constructor.setAccessible(true);
+ Field cleanerField = cls.getDeclaredField("cleaner");
+ cleanerField.setAccessible(true);
+ DBB_CONSTRUCTOR = constructor;
+ DBB_CLEANER_FIELD = cleanerField;
+ } catch (ClassNotFoundException | NoSuchMethodException | NoSuchFieldException e) {
+ throw new IllegalStateException(e);
+ }
+ }
+
+ private static final Method CLEANER_CREATE_METHOD;
+ static {
+ // The implementation of Cleaner changed from JDK 8 to 9
--- End diff --
Yes, this comment is just noting that the change happened between Java 8 and 9. We are targeting support for Java 11. However I expect virtually all of the changes that we have to make are due to changes in Java 9. (And I have no reason to believe Java 9-10 wouldn't work; we want them to work too)
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22993: [SPARK-24421][BUILD][CORE] Accessing sun.misc.Cleaner in...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22993
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 #22993: [SPARK-24421][BUILD][CORE] Accessing sun.misc.Cleaner in...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22993
**[Test build #98686 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/98686/testReport)** for PR 22993 at commit [`2db719c`](https://github.com/apache/spark/commit/2db719c2896c96793dfc96791bbadb2bc8a3f4ab).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22993: [SPARK-24421][BUILD][CORE] Accessing sun.misc.Cleaner in...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22993
**[Test build #4422 has finished](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/4422/testReport)** for PR 22993 at commit [`f137de7`](https://github.com/apache/spark/commit/f137de748e092315cc11e66deaafbcb469dd5764).
* This patch **fails to build**.
* 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 pull request #22993: [SPARK-24421][BUILD][CORE] Accessing sun.misc.Cle...
Posted by srowen <gi...@git.apache.org>.
Github user srowen closed the pull request at:
https://github.com/apache/spark/pull/22993
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22993: [SPARK-24421][BUILD][CORE] Accessing sun.misc.Cleaner in...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/22993
Merged to master
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22993: [SPARK-24421][BUILD][CORE] Accessing sun.misc.Cle...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/22993#discussion_r233124434
--- Diff: core/src/main/scala/org/apache/spark/storage/StorageUtils.scala ---
@@ -193,6 +194,36 @@ private[spark] class StorageStatus(
/** Helper methods for storage-related objects. */
private[spark] object StorageUtils extends Logging {
+
+ // In Java 8, the type of DirectBuffer.cleaner() was sun.misc.Cleaner, and it was possible
+ // to access the method sun.misc.Cleaner.clean() to invoke it. The type changed to
+ // jdk.internal.ref.Cleaner in later JDKs, and the .clean() method is not accessible even with
+ // reflection. However sun.misc.Unsafe added a invokeCleaner() method in JDK 9+ and this is
+ // still accessible with reflection.
+ private val bufferCleaner: DirectBuffer => Unit =
+ // Split java.version on non-digit chars:
+ if (System.getProperty("java.version").split("\\D+").head.toInt < 9) {
+ // scalastyle:off classforname
+ val cleanerMethod = Class.forName("sun.misc.Cleaner").getMethod("clean")
+ // scalastyle:on classforname
+ (buffer: DirectBuffer) => {
+ // Careful to avoid the return type of .cleaner(), which changes with JDK
+ val cleaner: AnyRef = buffer.cleaner()
+ if (cleaner != null) {
+ cleanerMethod.invoke(cleaner)
+ }
+ }
+ } else {
+ // scalastyle:off classforname
+ val cleanerMethod =
+ Class.forName("sun.misc.Unsafe").getMethod("invokeCleaner", classOf[ByteBuffer])
--- End diff --
The problem is, we can't change the usage in Platform.java, so we'd be mixing strategies here. There are already usages of Class.forName for this reason. I don't really object to changing one instance here; I have no reason to believe either of them fails. But is that meeting your goal?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22993: [SPARK-24421][BUILD][CORE] Accessing sun.misc.Cleaner in...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/22993
@felixcheung the JVM flag would be something like `--add-opens java.base/java.lang=ALL-UNNAMED`. Everything should still work without it, it's just that this 'hack' no longer works to ignore `MaxDirectMemorySize`. That default appears to be equal to the size of the JVM heap, which isn't small, so also should work in many situations. For those apps that use a large amount of direct memory, another alternative is to set `-XX:MaxDirectMemorySize=` directly.
I don't know of a better solution right now, and that one's not bad, but I will write up provisional release notes in the JIRA that explain this.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22993: [SPARK-24421][BUILD][CORE] Accessing sun.misc.Cleaner in...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/22993
Any more comments? I hope this much is safe and gets Java 11 compiling and most tests passing.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22993: [SPARK-24421][BUILD][CORE] Accessing sun.misc.Cleaner in...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22993
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 pull request #22993: [SPARK-24421][BUILD][CORE] Accessing sun.misc.Cle...
Posted by skonto <gi...@git.apache.org>.
Github user skonto commented on a diff in the pull request:
https://github.com/apache/spark/pull/22993#discussion_r232847435
--- Diff: core/src/main/scala/org/apache/spark/storage/StorageUtils.scala ---
@@ -193,6 +194,36 @@ private[spark] class StorageStatus(
/** Helper methods for storage-related objects. */
private[spark] object StorageUtils extends Logging {
+
+ // In Java 8, the type of DirectBuffer.cleaner() was sun.misc.Cleaner, and it was possible
+ // to access the method sun.misc.Cleaner.clean() to invoke it. The type changed to
+ // jdk.internal.ref.Cleaner in later JDKs, and the .clean() method is not accessible even with
+ // reflection. However sun.misc.Unsafe added a invokeCleaner() method in JDK 9+ and this is
+ // still accessible with reflection.
+ private val bufferCleaner: DirectBuffer => Unit =
+ // Split java.version on non-digit chars:
+ if (System.getProperty("java.version").split("\\D+").head.toInt < 9) {
+ // scalastyle:off classforname
+ val cleanerMethod = Class.forName("sun.misc.Cleaner").getMethod("clean")
+ // scalastyle:on classforname
+ (buffer: DirectBuffer) => {
+ // Careful to avoid the return type of .cleaner(), which changes with JDK
+ val cleaner: AnyRef = buffer.cleaner()
+ if (cleaner != null) {
+ cleanerMethod.invoke(cleaner)
+ }
+ }
+ } else {
+ // scalastyle:off classforname
+ val cleanerMethod =
+ Class.forName("sun.misc.Unsafe").getMethod("invokeCleaner", classOf[ByteBuffer])
--- End diff --
Should we use Utils forname method here?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22993: [SPARK-24421][BUILD][CORE] Accessing sun.misc.Cleaner in...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22993
**[Test build #98802 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/98802/testReport)** for PR 22993 at commit [`9d775bb`](https://github.com/apache/spark/commit/9d775bb9650307a507a848146e05c2b8826630f6).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22993: [SPARK-24421][BUILD][CORE] Accessing sun.misc.Cleaner in...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22993
**[Test build #98654 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/98654/testReport)** for PR 22993 at commit [`f137de7`](https://github.com/apache/spark/commit/f137de748e092315cc11e66deaafbcb469dd5764).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22993: [SPARK-24421][BUILD][CORE] Accessing sun.misc.Cle...
Posted by skonto <gi...@git.apache.org>.
Github user skonto commented on a diff in the pull request:
https://github.com/apache/spark/pull/22993#discussion_r233002403
--- Diff: core/src/main/scala/org/apache/spark/storage/StorageUtils.scala ---
@@ -193,6 +194,36 @@ private[spark] class StorageStatus(
/** Helper methods for storage-related objects. */
private[spark] object StorageUtils extends Logging {
+
+ // In Java 8, the type of DirectBuffer.cleaner() was sun.misc.Cleaner, and it was possible
+ // to access the method sun.misc.Cleaner.clean() to invoke it. The type changed to
+ // jdk.internal.ref.Cleaner in later JDKs, and the .clean() method is not accessible even with
+ // reflection. However sun.misc.Unsafe added a invokeCleaner() method in JDK 9+ and this is
+ // still accessible with reflection.
+ private val bufferCleaner: DirectBuffer => Unit =
+ // Split java.version on non-digit chars:
+ if (System.getProperty("java.version").split("\\D+").head.toInt < 9) {
+ // scalastyle:off classforname
+ val cleanerMethod = Class.forName("sun.misc.Cleaner").getMethod("clean")
+ // scalastyle:on classforname
+ (buffer: DirectBuffer) => {
+ // Careful to avoid the return type of .cleaner(), which changes with JDK
+ val cleaner: AnyRef = buffer.cleaner()
+ if (cleaner != null) {
+ cleanerMethod.invoke(cleaner)
+ }
+ }
+ } else {
+ // scalastyle:off classforname
+ val cleanerMethod =
+ Class.forName("sun.misc.Unsafe").getMethod("invokeCleaner", classOf[ByteBuffer])
--- End diff --
Probably it is safer not to mix strategies (https://www.javaworld.com/article/2077344/core-java/find-a-way-out-of-the-classloader-maze.html). Utils forName method first checks the thread context class loader and from what I see this method is called in numerous places. Spark creates custom classloaders AFAIK so the question is from which classloader chain you want this class loading to happen. I think both should work.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22993: [SPARK-24421][BUILD][CORE] Accessing sun.misc.Cleaner in...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22993
**[Test build #98804 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/98804/testReport)** for PR 22993 at commit [`2ee58b2`](https://github.com/apache/spark/commit/2ee58b2ba66fbf497e819feff9beb786d4884acd).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22993: [SPARK-24421][BUILD][CORE] Accessing sun.misc.Cleaner in...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22993
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/98804/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22993: [SPARK-24421][BUILD][CORE] Accessing sun.misc.Cleaner in...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22993
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/98686/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22993: [SPARK-24421][BUILD][CORE] Accessing sun.misc.Cle...
Posted by felixcheung <gi...@git.apache.org>.
Github user felixcheung commented on a diff in the pull request:
https://github.com/apache/spark/pull/22993#discussion_r232499645
--- Diff: common/unsafe/src/main/java/org/apache/spark/unsafe/Platform.java ---
@@ -67,6 +67,59 @@
unaligned = _unaligned;
}
+ // Access fields and constructors once and store them, for performance:
+
+ private static final Constructor<?> DBB_CONSTRUCTOR;
+ private static final Field DBB_CLEANER_FIELD;
+ static {
+ try {
+ Class<?> cls = Class.forName("java.nio.DirectByteBuffer");
+ Constructor<?> constructor = cls.getDeclaredConstructor(Long.TYPE, Integer.TYPE);
+ constructor.setAccessible(true);
+ Field cleanerField = cls.getDeclaredField("cleaner");
+ cleanerField.setAccessible(true);
+ DBB_CONSTRUCTOR = constructor;
+ DBB_CLEANER_FIELD = cleanerField;
+ } catch (ClassNotFoundException | NoSuchMethodException | NoSuchFieldException e) {
+ throw new IllegalStateException(e);
+ }
+ }
+
+ private static final Method CLEANER_CREATE_METHOD;
+ static {
+ // The implementation of Cleaner changed from JDK 8 to 9
+ int majorVersion = Integer.parseInt(System.getProperty("java.version").split("\\.")[0]);
--- End diff --
looks like it could be `java.version=1.8.0_192` or `java.version=11.0.1`
ie. first integer or second integer (1.8 => 8?)
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22993: [SPARK-24421][BUILD][CORE] Accessing sun.misc.Cle...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/22993#discussion_r232849773
--- Diff: core/src/main/scala/org/apache/spark/storage/StorageUtils.scala ---
@@ -193,6 +194,36 @@ private[spark] class StorageStatus(
/** Helper methods for storage-related objects. */
private[spark] object StorageUtils extends Logging {
+
+ // In Java 8, the type of DirectBuffer.cleaner() was sun.misc.Cleaner, and it was possible
+ // to access the method sun.misc.Cleaner.clean() to invoke it. The type changed to
+ // jdk.internal.ref.Cleaner in later JDKs, and the .clean() method is not accessible even with
+ // reflection. However sun.misc.Unsafe added a invokeCleaner() method in JDK 9+ and this is
+ // still accessible with reflection.
+ private val bufferCleaner: DirectBuffer => Unit =
+ // Split java.version on non-digit chars:
+ if (System.getProperty("java.version").split("\\D+").head.toInt < 9) {
+ // scalastyle:off classforname
+ val cleanerMethod = Class.forName("sun.misc.Cleaner").getMethod("clean")
+ // scalastyle:on classforname
+ (buffer: DirectBuffer) => {
+ // Careful to avoid the return type of .cleaner(), which changes with JDK
+ val cleaner: AnyRef = buffer.cleaner()
+ if (cleaner != null) {
+ cleanerMethod.invoke(cleaner)
+ }
+ }
+ } else {
+ // scalastyle:off classforname
+ val cleanerMethod =
+ Class.forName("sun.misc.Unsafe").getMethod("invokeCleaner", classOf[ByteBuffer])
--- End diff --
I wasn't sure whether it was good or not to use the Spark classloader, as that method does. If there's any good reason to, sure. This is kind of a special situation, but don't know if it must use Class directly.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22993: [SPARK-24421][BUILD][CORE] Accessing sun.misc.Cleaner in...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22993
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 #22993: [SPARK-24421][BUILD][CORE] Accessing sun.misc.Cleaner in...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22993
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/98700/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22993: [SPARK-24421][BUILD][CORE] Accessing sun.misc.Cleaner in...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22993
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/98653/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22993: [SPARK-24421][BUILD][CORE] Accessing sun.misc.Cleaner in...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22993
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/5005/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22993: [SPARK-24421][BUILD][CORE] Accessing sun.misc.Cleaner in...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22993
**[Test build #4422 has started](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/4422/testReport)** for PR 22993 at commit [`f137de7`](https://github.com/apache/spark/commit/f137de748e092315cc11e66deaafbcb469dd5764).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22993: [SPARK-24421][BUILD][CORE] Accessing sun.misc.Cleaner in...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22993
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/98802/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22993: [SPARK-24421][BUILD][CORE] Accessing sun.misc.Cleaner in...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22993
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 pull request #22993: [SPARK-24421][BUILD][CORE] Accessing sun.misc.Cle...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/22993#discussion_r233222902
--- Diff: core/src/main/scala/org/apache/spark/storage/StorageUtils.scala ---
@@ -193,6 +194,36 @@ private[spark] class StorageStatus(
/** Helper methods for storage-related objects. */
private[spark] object StorageUtils extends Logging {
+
+ // In Java 8, the type of DirectBuffer.cleaner() was sun.misc.Cleaner, and it was possible
+ // to access the method sun.misc.Cleaner.clean() to invoke it. The type changed to
+ // jdk.internal.ref.Cleaner in later JDKs, and the .clean() method is not accessible even with
+ // reflection. However sun.misc.Unsafe added a invokeCleaner() method in JDK 9+ and this is
+ // still accessible with reflection.
+ private val bufferCleaner: DirectBuffer => Unit =
+ // Split java.version on non-digit chars:
+ if (System.getProperty("java.version").split("\\D+").head.toInt < 9) {
--- End diff --
I like it. I made both changes. Actually there's just one other location where java.version is parsed where we can use lang3, so I just changed that too. That's more convincing as it's more consistent.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22993: [SPARK-24421][BUILD][CORE] Accessing sun.misc.Cleaner in...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22993
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/98654/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22993: [SPARK-24421][BUILD][CORE] Accessing sun.misc.Cleaner in...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22993
**[Test build #98700 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/98700/testReport)** for PR 22993 at commit [`7f58ae6`](https://github.com/apache/spark/commit/7f58ae61262d7c2f2d70c24d051c63e8830d5062).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22993: [SPARK-24421][BUILD][CORE] Accessing sun.misc.Cleaner in...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22993
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 #22993: [SPARK-24421][BUILD][CORE] Accessing sun.misc.Cleaner in...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22993
**[Test build #98654 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/98654/testReport)** for PR 22993 at commit [`f137de7`](https://github.com/apache/spark/commit/f137de748e092315cc11e66deaafbcb469dd5764).
* This patch **fails to build**.
* 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 pull request #22993: [SPARK-24421][BUILD][CORE] Accessing sun.misc.Cle...
Posted by skonto <gi...@git.apache.org>.
Github user skonto commented on a diff in the pull request:
https://github.com/apache/spark/pull/22993#discussion_r233166698
--- Diff: core/src/main/scala/org/apache/spark/storage/StorageUtils.scala ---
@@ -193,6 +194,36 @@ private[spark] class StorageStatus(
/** Helper methods for storage-related objects. */
private[spark] object StorageUtils extends Logging {
+
+ // In Java 8, the type of DirectBuffer.cleaner() was sun.misc.Cleaner, and it was possible
+ // to access the method sun.misc.Cleaner.clean() to invoke it. The type changed to
+ // jdk.internal.ref.Cleaner in later JDKs, and the .clean() method is not accessible even with
+ // reflection. However sun.misc.Unsafe added a invokeCleaner() method in JDK 9+ and this is
+ // still accessible with reflection.
+ private val bufferCleaner: DirectBuffer => Unit =
+ // Split java.version on non-digit chars:
+ if (System.getProperty("java.version").split("\\D+").head.toInt < 9) {
--- End diff --
It is a source of inspiration though for how to parse the string of that property even if you dont rely on this dependency.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22993: [SPARK-24421][BUILD][CORE] Accessing sun.misc.Cle...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/22993#discussion_r232867047
--- Diff: core/src/main/scala/org/apache/spark/storage/StorageUtils.scala ---
@@ -193,6 +194,36 @@ private[spark] class StorageStatus(
/** Helper methods for storage-related objects. */
private[spark] object StorageUtils extends Logging {
+
+ // In Java 8, the type of DirectBuffer.cleaner() was sun.misc.Cleaner, and it was possible
+ // to access the method sun.misc.Cleaner.clean() to invoke it. The type changed to
+ // jdk.internal.ref.Cleaner in later JDKs, and the .clean() method is not accessible even with
+ // reflection. However sun.misc.Unsafe added a invokeCleaner() method in JDK 9+ and this is
+ // still accessible with reflection.
+ private val bufferCleaner: DirectBuffer => Unit =
+ // Split java.version on non-digit chars:
+ if (System.getProperty("java.version").split("\\D+").head.toInt < 9) {
--- End diff --
As to using java.specification.version, I admit I had not seen that used before, not in Spark at least. Is it more reliable? I had always seen java.version parsed. Here we really just care about the initial digit in any event.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22993: [SPARK-24421][BUILD][CORE] Accessing sun.misc.Cleaner in...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22993
**[Test build #98802 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/98802/testReport)** for PR 22993 at commit [`9d775bb`](https://github.com/apache/spark/commit/9d775bb9650307a507a848146e05c2b8826630f6).
* This patch **fails build dependency 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 #22993: [SPARK-24421][BUILD][CORE] Accessing sun.misc.Cleaner in...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22993
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 #22993: [SPARK-24421][BUILD][CORE] Accessing sun.misc.Cleaner in...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22993
**[Test build #98653 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/98653/testReport)** for PR 22993 at commit [`ebf280d`](https://github.com/apache/spark/commit/ebf280d8c577f697ba679ec1f76296b524f23a23).
* This patch **fails build dependency 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 #22993: [SPARK-24421][BUILD][CORE] Accessing sun.misc.Cleaner in...
Posted by felixcheung <gi...@git.apache.org>.
Github user felixcheung commented on the issue:
https://github.com/apache/spark/pull/22993
what settings we need to allow `illegal reflective access`
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22993: [SPARK-24421][BUILD][CORE] Accessing sun.misc.Cleaner in...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22993
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/5007/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org