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.

Sign up for GitHub

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-19999]: Workaround JDK-8165231 to identify PPC64 architectures as supporting unaligned access #17472

Closed
wants to merge 6 commits into from
Closed

[SPARK-19999]: Workaround JDK-8165231 to identify PPC64 architectures as supporting unaligned access #17472

Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
bcef03d
SPARK-19999 - fix jdk bug
samelamin Mar 29, 2017
8785664
indentation keeps changing because of intellij
samelamin Mar 29, 2017
799effa
set default indentation to 2
samelamin Mar 29, 2017
bf7cc24
move and clarify comment
samelamin Mar 29, 2017
77777e9
define arch befote if statement
samelamin Mar 30, 2017
632161b
use equals as per feedback from pr
selamin Mar 30, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 16 additions & 12 deletions common/unsafe/src/main/java/org/apache/spark/unsafe/Platform.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,18 +46,22 @@ public final class Platform {
private static final boolean unaligned;
static {
boolean _unaligned;
// use reflection to access unaligned field
try {
Class<?> bitsClass =
Class.forName("java.nio.Bits", false, ClassLoader.getSystemClassLoader());
Method unalignedMethod = bitsClass.getDeclaredMethod("unaligned");
unalignedMethod.setAccessible(true);
_unaligned = Boolean.TRUE.equals(unalignedMethod.invoke(null));
} catch (Throwable t) {
// We at least know x86 and x64 support unaligned access.
String arch = System.getProperty("os.arch", "");
//noinspection DynamicRegexReplaceableByCompiledPattern
_unaligned = arch.matches("^(i[3-6]86|x86(_64)?|x64|amd64|aarch64)$");
String arch = System.getProperty("os.arch", "");
if (arch.matches("^(ppc64le | ppc64)$")) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still, why not arch.startsWith("ppc64")?

Copy link
Contributor Author

@samelamin samelamin Mar 30, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Title changed, but I dont have access to change the jira title?

Re string matching - Thats fine but itll end up being arch.startsWith("ppc64") || arch.startsWith("ppc64le")

I thought this was less code, but startsWith is more readable

Are you happy with that approach?

Copy link
Member

@srowen srowen Mar 30, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both of those start with ppc64, what do you mean? I guess my question is, is there a big-endian PPC64 arch?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah sorry complete fail, didnt see that lol

With regards to a big-endian PPC64 arch , I dont think so but I am not 100% sure. that said as it stands it isn't supported and the test fails. So happy to change the PR or even close it if you do not see any value it adds

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I'm writing one thing and thinking another. I mean alignment, not endianness. Which architectures do you know allow unaligned access? I'd presume all PPC does, and I assume the JDK issue means "PPC64 (big-endian) but also PPC64 little-endian". OK, to be conservative, maybe just check the strings "ppc64" and "ppc64le" as you intended.

However your regex doesn't work. You have extra whitespace. Just instead check arch.equals(...) || arch.equals(...)

Also the PR title shoudl match the JIRA title. I was commenting that it differed here in referring to a flaky test, but it isn't. To be very clear, I propose you rename both to perhaps: "Workaround JDK-8165231 to identify PPC64 architectures as supporting unaligned access"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

haha its fine, called it rubber duck programming eh

Sure I will make those changes, but I have no rights to rename the JIRA

So I will rename this PR, can you get yourself or someone to rename the JIRA?

// Since java.nio.Bits.unaligned() doesn't return true on ppc (See JDK-8165231), but ppc64 and ppc64le support it
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is longer than 101 characters. It will fail the style test.

You can check it in your local environment using the command:

dev/lint-scala

_unaligned = true;
} else {
try {
Class<?> bitsClass =
Class.forName("java.nio.Bits", false, ClassLoader.getSystemClassLoader());
Method unalignedMethod = bitsClass.getDeclaredMethod("unaligned");
unalignedMethod.setAccessible(true);
_unaligned = Boolean.TRUE.equals(unalignedMethod.invoke(null));
} catch (Throwable t) {
// We at least know x86 and x64 support unaligned access.
//noinspection DynamicRegexReplaceableByCompiledPattern
_unaligned = arch.matches("^(i[3-6]86|x86(_64)?|x64|amd64|aarch64)$");
}
}
unaligned = _unaligned;
}
Expand Down