[SPARK-24421][BUILD][CORE] Accessing sun.misc.Cleaner in JDK11 #22993

srowen · 2018-11-09T17:11:54Z

…. 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.

…. Other related changes to get JDK 11 working, to test

SparkQA · 2018-11-09T17:17:03Z

Test build #98653 has finished for PR 22993 at commit ebf280d.

This patch fails build dependency tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-11-09T17:54:18Z

Test build #98654 has finished for PR 22993 at commit f137de7.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-11-10T05:51:55Z

Test build #4422 has finished for PR 22993 at commit f137de7.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-11-11T01:13:42Z

Test build #98686 has finished for PR 22993 at commit 2db719c.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

felixcheung · 2018-11-11T08:07:06Z

what settings we need to allow illegal reflective access

felixcheung · 2018-11-11T08:09:03Z

common/unsafe/src/main/java/org/apache/spark/unsafe/Platform.java

+  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]);


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 ...)

From Java 9, here is a new definition. In the following example, I found a few exceptions.

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

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.

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?)

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.

How about using java.specification.version and parse it as double? Then check if greater than 2.

srowen · 2018-11-11T15:24:46Z

@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.

SparkQA · 2018-11-11T19:56:37Z

Test build #98700 has finished for PR 22993 at commit 7f58ae6.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

skonto · 2018-11-12T23:25:47Z

common/unsafe/src/main/java/org/apache/spark/unsafe/Platform.java

+
+  private static final Method CLEANER_CREATE_METHOD;
+  static {
+    // The implementation of Cleaner changed from JDK 8 to 9


Why do we need to reference java 9, its dead right and it will have no LTS AFAIK.
Shouldnt we only support LTS?

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)

skonto · 2018-11-12T23:28:32Z

core/src/main/scala/org/apache/spark/storage/StorageUtils.scala

+    } else {
+      // scalastyle:off classforname
+      val cleanerMethod =
+        Class.forName("sun.misc.Unsafe").getMethod("invokeCleaner", classOf[ByteBuffer])


Should we use Utils forname method here?

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.

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, it is not a custom class or anything.

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?

ok, in general I cant think of something failing at the moment, just wanted to provide some feedback that this mixes strategies.

skonto · 2018-11-12T23:30:02Z

core/src/main/scala/org/apache/spark/storage/StorageUtils.scala

+  // 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) {


Is this java detection method used elsewhere? Maybe other parts of the codebase could benefit from this.

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.

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.

Apache commons is an example:

import org.apache.commons.lang3.JavaVersion; import org.apache.commons.lang3.SystemUtils; SystemUtils.isJavaVersionAtLeast(JavaVersion.JAVA_9);

That dependency already exists in the project, so it is for free, and that method is based on that property:
https://commons.apache.org/proper/commons-lang/apidocs/org/apache/commons/lang3/JavaVersion.html

Their implementation is here:
https://github.com/apache/commons-lang/blob/c4d0dbcb56b8980b1b3b7c85d00ad6540788c08e/src/main/java/org/apache/commons/lang3/JavaVersion.java#L228

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.

It is at least a source of inspiration for the implementation though, even if you dont rely on this dependency. It is updated by the way up to current versions from what I checked.

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.

SparkQA · 2018-11-13T21:16:18Z

Test build #98802 has finished for PR 22993 at commit 9d775bb.

This patch fails build dependency tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-11-14T03:04:20Z

Test build #98804 has finished for PR 22993 at commit 2ee58b2.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

srowen · 2018-11-14T15:30:01Z

Any more comments? I hope this much is safe and gets Java 11 compiling and most tests passing.

srowen · 2018-11-14T20:53:08Z

Merged to master

…. 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. Closes apache#22993 from srowen/SPARK-24421. Authored-by: Sean Owen <[email protected]> Signed-off-by: Sean Owen <[email protected]>

srowen · 2019-01-02T04:27:40Z

common/unsafe/src/main/java/org/apache/spark/unsafe/Platform.java

-      Cleaner cleaner = Cleaner.create(buffer, () -> freeMemory(memory));
-      cleanerField.set(buffer, cleaner);
+      ByteBuffer buffer = (ByteBuffer) DBB_CONSTRUCTOR.newInstance(memory, size);
+      if (CLEANER_CREATE_METHOD != null) {


See #23424 ; I now think this was an error.

… if Cleaner can't be set ## What changes were proposed in this pull request? In Java 9+ we can't use sun.misc.Cleaner by default anymore, and this was largely handled in #22993 However I think the change there left a significant problem. If a DirectByteBuffer is allocated using the reflective hack in Platform, now, we by default can't set a Cleaner. But I believe this means the memory isn't freed promptly or possibly at all. If a Cleaner can't be set, I think we need to use normal APIs to allocate the direct ByteBuffer. According to comments in the code, the downside is simply that the normal APIs will check and impose limits on how much off-heap memory can be allocated. Per the original review on #22993 this much seems fine, as either way in this case the user would have to add a JVM setting (increase max, or allow the reflective access). ## How was this patch tested? Existing tests. This resolved an OutOfMemoryError in Java 11 from TimSort tests without increasing test heap size. (See #23419 (comment) ) This suggests there is a problem and that this resolves it. Closes #23424 from srowen/SPARK-24421.2. Authored-by: Sean Owen <[email protected]> Signed-off-by: Sean Owen <[email protected]>

…. 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. Closes apache#22993 from srowen/SPARK-24421. Authored-by: Sean Owen <[email protected]> Signed-off-by: Sean Owen <[email protected]>

… if Cleaner can't be set ## What changes were proposed in this pull request? In Java 9+ we can't use sun.misc.Cleaner by default anymore, and this was largely handled in apache#22993 However I think the change there left a significant problem. If a DirectByteBuffer is allocated using the reflective hack in Platform, now, we by default can't set a Cleaner. But I believe this means the memory isn't freed promptly or possibly at all. If a Cleaner can't be set, I think we need to use normal APIs to allocate the direct ByteBuffer. According to comments in the code, the downside is simply that the normal APIs will check and impose limits on how much off-heap memory can be allocated. Per the original review on apache#22993 this much seems fine, as either way in this case the user would have to add a JVM setting (increase max, or allow the reflective access). ## How was this patch tested? Existing tests. This resolved an OutOfMemoryError in Java 11 from TimSort tests without increasing test heap size. (See apache#23419 (comment) ) This suggests there is a problem and that this resolves it. Closes apache#23424 from srowen/SPARK-24421.2. Authored-by: Sean Owen <[email protected]> Signed-off-by: Sean Owen <[email protected]>

…. 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. Closes apache#22993 from srowen/SPARK-24421. Authored-by: Sean Owen <[email protected]> Signed-off-by: Sean Owen <[email protected]> (cherry picked from commit 722369e)

Avoid sun.misc.Cleaner in JDK 9+ while retaining Java 8 compatibility…

ebf280d

…. Other related changes to get JDK 11 working, to test

Update deps

f137de7

Fix compile error in hive-thriftserver

2db719c

felixcheung reviewed Nov 11, 2018

View reviewed changes

Split java.version on non-digit char

7f58ae6

skonto reviewed Nov 12, 2018

View reviewed changes

Use Utils.classForName and lang3 for Java version

9d775bb

Update lang3 deps

2ee58b2

srowen closed this Nov 14, 2018

srowen deleted the SPARK-24421 branch November 14, 2018 20:54

srowen mentioned this pull request Jan 2, 2019

[SPARK-24421][CORE][FOLLOWUP] Use normal direct ByteBuffer allocation if Cleaner can't be set #23424

Closed

srowen commented Jan 2, 2019

View reviewed changes

srowen mentioned this pull request Mar 16, 2019

[SPARK-27180][BUILD][YARN] Fix testing issues with yarn module in Hadoop-3 #24115

Closed

jens-maus mentioned this pull request Oct 16, 2023

HMIPServer not starting anymore OpenCCU/OpenCCU#2470

Closed

[SPARK-24421][BUILD][CORE] Accessing sun.misc.Cleaner in JDK11 #22993

[SPARK-24421][BUILD][CORE] Accessing sun.misc.Cleaner in JDK11 #22993

Uh oh!

Conversation

srowen commented Nov 9, 2018

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Nov 9, 2018

Uh oh!

SparkQA commented Nov 9, 2018

Uh oh!

SparkQA commented Nov 10, 2018

Uh oh!

SparkQA commented Nov 11, 2018

Uh oh!

felixcheung commented Nov 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kiszk Nov 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

srowen commented Nov 11, 2018

Uh oh!

SparkQA commented Nov 11, 2018

Uh oh!

skonto Nov 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skonto Nov 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skonto Nov 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skonto Nov 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 13, 2018

Uh oh!

SparkQA commented Nov 14, 2018

Uh oh!

srowen commented Nov 14, 2018

Uh oh!

srowen commented Nov 14, 2018

Uh oh!

kiszk Nov 11, 2018 •

edited

Loading

skonto Nov 12, 2018 •

edited

Loading

skonto Nov 13, 2018 •

edited

Loading

skonto Nov 13, 2018 •

edited

Loading

skonto Nov 13, 2018 •

edited

Loading