Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SPARK-7288] Suppress compiler warnings due to use of sun.misc.Unsafe; add facade in front of Unsafe; remove use of Unsafe.setMemory #5814

Closed

Conversation

JoshRosen
Copy link
Contributor

This patch suppresses compiler warnings due to our use of sun.misc.Unsafe (introduced in #5725). These warnings can only be suppressed via the -XDignore.symbol.file javac flag; the @SuppressWarnings annotation won't work for these.

In order to restrict uses of this compiler flag to the unsafe module, I placed a facade in front of Unsafe so that other modules won't call it directly. This facade also will also help us to avoid accidental usage of deprecated Unsafe methods or methods that aren't supported in Java 6.

I also removed an unnecessary use of Unsafe.setMemory, which isn't present in certain versions of Java 6, and excluded the new unsafe module from Javadoc.

@rxin
Copy link
Contributor

rxin commented Apr 30, 2015

Maybe you've already figured it out, but is it possible to suppress warning only for the unsafe module?

@tgravescs
Copy link
Contributor

trying it out

@JoshRosen
Copy link
Contributor Author

Won't we get warnings in other modules that call PlatformDependent.UNSAFE.* methods, like UnsafeRow in SQL?

@JoshRosen
Copy link
Contributor Author

I think that we'll be able to restrict this compiler flag to the unsafe module if we place a facade in front of sun.misc.Unsafe so that other modules don't directly call those methods. This should be pretty easy, so I'll take a shot at it now.

@tgravescs
Copy link
Contributor

it built fine for me with jdk6 and this patch.

@JoshRosen
Copy link
Contributor Author

Also checking to see whether I need to change unidoc compiler options...

@JoshRosen
Copy link
Contributor Author

Alright, pushed a new commit that puts a facade in front of Unsafe and reduced the scope of the compiler flag to only affect the unsafe module. I'm not super-proficient with Maven, so it would be good for someone to double-check that the way I'm merging in the additional compiler argument configs is correct.

@SparkQA
Copy link

SparkQA commented Apr 30, 2015

Test build #31442 has finished for PR 5814 at commit 50230c0.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.
  • This patch does not change any dependencies.

@JoshRosen
Copy link
Contributor Author

When I ran my one of my aggregation hash map benchmarks with -XX:+UnlockDiagnosticVMOptions -XX:+PrintInlining enabled it looks like the static methods in the UNSAFE facade are being inlined. I grepped the output for "org.apache.spark.unsafe.PlatformDependent$UNSAFE::" and it looks like all call sites are being inlined. @rxin, is this a methodologically sound way to check that the facade didn't add any overhead?

@JoshRosen JoshRosen changed the title Suppress compiler warnings due to use of sun.misc.Unsafe; remove use of Unsafe.setMemory Suppress compiler warnings due to use of sun.misc.Unsafe; add facade in front of Unsafe; remove use of Unsafe.setMemory Apr 30, 2015
@rxin
Copy link
Contributor

rxin commented Apr 30, 2015

LGTM.

@SparkQA
Copy link

SparkQA commented Apr 30, 2015

Test build #31447 has finished for PR 5814 at commit 9e8c483.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • public static final class UNSAFE
  • This patch does not change any dependencies.

@JoshRosen JoshRosen changed the title Suppress compiler warnings due to use of sun.misc.Unsafe; add facade in front of Unsafe; remove use of Unsafe.setMemory [SPARK-7288] Suppress compiler warnings due to use of sun.misc.Unsafe; add facade in front of Unsafe; remove use of Unsafe.setMemory Apr 30, 2015
@asfgit asfgit closed this in 07a8620 Apr 30, 2015
@JoshRosen JoshRosen deleted the unsafe-compiler-warnings-fixes branch April 30, 2015 22:22
jeanlyn pushed a commit to jeanlyn/spark that referenced this pull request May 28, 2015
…; add facade in front of Unsafe; remove use of Unsafe.setMemory

This patch suppresses compiler warnings due to our use of `sun.misc.Unsafe` (introduced in apache#5725).  These warnings can only be suppressed via the `-XDignore.symbol.file` javac flag; the `SuppressWarnings` annotation won't work for these.

In order to restrict uses of this compiler flag to the `unsafe` module, I placed a facade in front of `Unsafe` so that other modules won't call it directly. This facade also will also help us to avoid accidental usage of deprecated Unsafe methods or methods that aren't supported in Java 6.

I also removed an unnecessary use of `Unsafe.setMemory`, which isn't present in certain versions of Java 6, and excluded the new `unsafe` module from Javadoc.

Author: Josh Rosen <[email protected]>

Closes apache#5814 from JoshRosen/unsafe-compiler-warnings-fixes and squashes the following commits:

9e8c483 [Josh Rosen] Exclude new unsafe module from Javadoc
ba75ecf [Josh Rosen] Only apply -XDignore.symbol.file flag in unsafe project.
7403345 [Josh Rosen] Put facade in front of Unsafe.
50230c0 [Josh Rosen] Remove usage of Unsafe.setMemory
96d41c9 [Josh Rosen] Use -XDignore.symbol.file to suppress warnings about sun.misc.Unsafe usage
jeanlyn pushed a commit to jeanlyn/spark that referenced this pull request Jun 12, 2015
…; add facade in front of Unsafe; remove use of Unsafe.setMemory

This patch suppresses compiler warnings due to our use of `sun.misc.Unsafe` (introduced in apache#5725).  These warnings can only be suppressed via the `-XDignore.symbol.file` javac flag; the `SuppressWarnings` annotation won't work for these.

In order to restrict uses of this compiler flag to the `unsafe` module, I placed a facade in front of `Unsafe` so that other modules won't call it directly. This facade also will also help us to avoid accidental usage of deprecated Unsafe methods or methods that aren't supported in Java 6.

I also removed an unnecessary use of `Unsafe.setMemory`, which isn't present in certain versions of Java 6, and excluded the new `unsafe` module from Javadoc.

Author: Josh Rosen <[email protected]>

Closes apache#5814 from JoshRosen/unsafe-compiler-warnings-fixes and squashes the following commits:

9e8c483 [Josh Rosen] Exclude new unsafe module from Javadoc
ba75ecf [Josh Rosen] Only apply -XDignore.symbol.file flag in unsafe project.
7403345 [Josh Rosen] Put facade in front of Unsafe.
50230c0 [Josh Rosen] Remove usage of Unsafe.setMemory
96d41c9 [Josh Rosen] Use -XDignore.symbol.file to suppress warnings about sun.misc.Unsafe usage
nemccarthy pushed a commit to nemccarthy/spark that referenced this pull request Jun 19, 2015
…; add facade in front of Unsafe; remove use of Unsafe.setMemory

This patch suppresses compiler warnings due to our use of `sun.misc.Unsafe` (introduced in apache#5725).  These warnings can only be suppressed via the `-XDignore.symbol.file` javac flag; the `SuppressWarnings` annotation won't work for these.

In order to restrict uses of this compiler flag to the `unsafe` module, I placed a facade in front of `Unsafe` so that other modules won't call it directly. This facade also will also help us to avoid accidental usage of deprecated Unsafe methods or methods that aren't supported in Java 6.

I also removed an unnecessary use of `Unsafe.setMemory`, which isn't present in certain versions of Java 6, and excluded the new `unsafe` module from Javadoc.

Author: Josh Rosen <[email protected]>

Closes apache#5814 from JoshRosen/unsafe-compiler-warnings-fixes and squashes the following commits:

9e8c483 [Josh Rosen] Exclude new unsafe module from Javadoc
ba75ecf [Josh Rosen] Only apply -XDignore.symbol.file flag in unsafe project.
7403345 [Josh Rosen] Put facade in front of Unsafe.
50230c0 [Josh Rosen] Remove usage of Unsafe.setMemory
96d41c9 [Josh Rosen] Use -XDignore.symbol.file to suppress warnings about sun.misc.Unsafe usage
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants