Skip to content

Commit

Permalink
[SPARK-45508][CORE] Add "--add-opens=java.base/jdk.internal.ref=ALL-U…
Browse files Browse the repository at this point in the history
…NNAMED" so Platform can access Cleaner on Java 9+

### What changes were proposed in this pull request?

This PR adds `--add-opens=java.base/jdk.internal.ref=ALL-UNNAMED` to our JVM flags so that we can access `jdk.internal.ref.Cleaner` in JDK 9+.

### Why are the changes needed?

This allows Spark to allocate direct memory while ignoring the JVM's MaxDirectMemorySize limit. Spark uses JDK internal APIs to directly construct DirectByteBuffers while bypassing that limit, but there is a fallback path at https://github.com/apache/spark/blob/v3.5.0/common/unsafe/src/main/java/org/apache/spark/unsafe/Platform.java#L213 that is used if we cannot reflectively access the `Cleaner` API.

### Does this PR introduce _any_ user-facing change?

No.

### How was this patch tested?

Added a unit test in `PlatformUtilSuite`.

### Was this patch authored or co-authored using generative AI tooling?

No.

Closes apache#43344 from JoshRosen/SPARK-45508.

Authored-by: Josh Rosen <[email protected]>
Signed-off-by: yangjie01 <[email protected]>
  • Loading branch information
JoshRosen authored and LuciferYang committed Oct 13, 2023
1 parent f1f856d commit 96bac6c
Show file tree
Hide file tree
Showing 5 changed files with 16 additions and 1 deletion.
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ public final class Platform {
Method createMethod = cleanerClass.getMethod("create", Object.class, Runnable.class);
// Accessing jdk.internal.ref.Cleaner should actually fail by default in JDK 9+,
// unfortunately, unless the user has allowed access with something like
// --add-opens java.base/java.lang=ALL-UNNAMED If not, we can't really use the Cleaner
// --add-opens java.base/jdk.internal.ref=ALL-UNNAMED If not, we can't use the Cleaner
// hack below. It doesn't break, just means the user might run into the default JVM limit
// on off-heap memory and increase it or set the flag above. This tests whether it's
// available:
Expand All @@ -118,6 +118,11 @@ public final class Platform {
}
}

// Visible for testing
public static boolean cleanerCreateMethodIsDefined() {
return CLEANER_CREATE_METHOD != null;
}

/**
* @return true when running JVM is having sun's Unsafe package available in it and underlying
* system having unaligned-access capability.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -157,4 +157,11 @@ public void heapMemoryReuse() {
Assertions.assertEquals(1024 * 1024 + 7, onheap4.size());
Assertions.assertEquals(obj3, onheap4.getBaseObject());
}

@Test
public void cleanerCreateMethodIsDefined() {
// Regression test for SPARK-45508: we don't expect the "no cleaner" fallback
// path to be hit in normal usage.
Assertions.assertTrue(Platform.cleanerCreateMethodIsDefined());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ public class JavaModuleOptions {
"--add-opens=java.base/java.util=ALL-UNNAMED",
"--add-opens=java.base/java.util.concurrent=ALL-UNNAMED",
"--add-opens=java.base/java.util.concurrent.atomic=ALL-UNNAMED",
"--add-opens=java.base/jdk.internal.ref=ALL-UNNAMED",
"--add-opens=java.base/sun.nio.ch=ALL-UNNAMED",
"--add-opens=java.base/sun.nio.cs=ALL-UNNAMED",
"--add-opens=java.base/sun.security.action=ALL-UNNAMED",
Expand Down
1 change: 1 addition & 0 deletions pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -311,6 +311,7 @@
--add-opens=java.base/java.util=ALL-UNNAMED
--add-opens=java.base/java.util.concurrent=ALL-UNNAMED
--add-opens=java.base/java.util.concurrent.atomic=ALL-UNNAMED
--add-opens=java.base/jdk.internal.ref=ALL-UNNAMED
--add-opens=java.base/sun.nio.ch=ALL-UNNAMED
--add-opens=java.base/sun.nio.cs=ALL-UNNAMED
--add-opens=java.base/sun.security.action=ALL-UNNAMED
Expand Down
1 change: 1 addition & 0 deletions project/SparkBuild.scala
Original file line number Diff line number Diff line change
Expand Up @@ -1585,6 +1585,7 @@ object TestSettings {
"--add-opens=java.base/java.util=ALL-UNNAMED",
"--add-opens=java.base/java.util.concurrent=ALL-UNNAMED",
"--add-opens=java.base/java.util.concurrent.atomic=ALL-UNNAMED",
"--add-opens=java.base/jdk.internal.ref=ALL-UNNAMED",
"--add-opens=java.base/sun.nio.ch=ALL-UNNAMED",
"--add-opens=java.base/sun.nio.cs=ALL-UNNAMED",
"--add-opens=java.base/sun.security.action=ALL-UNNAMED",
Expand Down

0 comments on commit 96bac6c

Please sign in to comment.