[SPARK-19999]: Workaround JDK-8165231 to identify PPC64 architectures as supporting unaligned access by samelamin · Pull Request #17472 · apache/spark

samelamin · 2017-03-29T19:27:45Z

java.nio.Bits.unaligned() does not return true for the ppc64le arch.
see https://bugs.openjdk.java.net/browse/JDK-8165231

What changes were proposed in this pull request?

check architecture

How was this patch tested?

unit test

kiszk · 2017-03-29T20:05:15Z

Would it be possible to add appropriate space between words?

I think you can perform this check up top? you don't need to do all the reflection here if the arch matches. Also is it true that anything starting with "ppc64" should be special-cased?

moved up top.

kiszk · 2017-03-29T20:05:41Z

Spark uses two spaces for an indent.

samelamin · 2017-03-29T20:50:29Z

@kiszk @srowen updated per your commets

gatorsmile · 2017-03-29T21:07:38Z

Indent issues.
Please follow the style guide: https://github.com/databricks/scala-style-guide#indent

I think its to do with my local setup, i dont see the issues in Intellij, sorry! :(
ill revert my changes and try again

samelamin · 2017-03-29T21:29:14Z

@gatorsmile I think my Intellij has some annoying auto indenting changes. Does spark have some project wide settings I can auto import?

gatorsmile · 2017-03-29T21:43:21Z

I do not think we have such a default steup. You can manually change the indentation of InteiiJ

samelamin · 2017-03-29T21:48:27Z

ok @gatorsmile i set default indentation to 2, how does it look now?

gatorsmile · 2017-03-29T22:00:00Z

+ if (arch.matches("^(ppc64le | ppc64)$")) {
+ _unaligned = true;
+ } else {
+ //Since java.nio.Bits.unaligned() doesn't return true on ppc (See JDK-8165231)


Moving this comment to line 50?
Maybe you also need to explain it more clearly.
Since java.nio.Bits.unaligned() doesn't return true on ppc (See JDK-8165231), but ppc64 and ppc64le support it

samelamin · 2017-03-29T22:07:03Z

@gatorsmile moved the comment per your suggestion, but to be honest if the comment is unclear surly the first thing someone will do is check that JIRA?

gatorsmile · 2017-03-29T22:27:46Z

- //noinspection DynamicRegexReplaceableByCompiledPattern
- _unaligned = arch.matches("^(i[3-6]86|x86(_64)?|x64|amd64|aarch64)$");
+ if (arch.matches("^(ppc64le | ppc64)$")) {
+ // Since java.nio.Bits.unaligned() doesn't return true on ppc (See JDK-8165231), but ppc64 and ppc64le support it


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

gatorsmile · 2017-03-29T22:27:56Z

ok to test

gatorsmile · 2017-03-29T22:29:31Z

Clear code comments can help code reading. : )

SparkQA · 2017-03-29T22:36:09Z

Test build #75372 has finished for PR 17472 at commit bf7cc24.

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

kiszk · 2017-03-30T04:15:30Z

+ _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", "");


Should we define arch before the if statement, now?

your absolutely right, I got caught off while I was trying to fix the indentation issues. good catch!

SparkQA · 2017-03-30T06:37:32Z

Test build #75380 has started for PR 17472 at commit 77777e9.

samelamin · 2017-03-30T08:52:00Z

jenkins test this please

samelamin · 2017-03-30T08:52:50Z

Can someone guide me why the tests failed? is it another flakey test or? because the below message doesnt make sense to me :(

Traceback (most recent call last):
File "./dev/run-tests-jenkins.py", line 226, in
main()
File "./dev/run-tests-jenkins.py", line 213, in main
test_result_code, test_result_note = run_tests(tests_timeout)
File "./dev/run-tests-jenkins.py", line 140, in run_tests
test_result_note = ' * This patch fails %s.' % failure_note_by_errcode[test_result_code]
KeyError: -9

srowen

I think the test failure is unrelated. The title of the JIRA/PR should be changed; they aren't flaky tests. You're trying to accommodate a new architecture that isn't really supported.

srowen · 2017-03-30T09:21:33Z

- //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)$")) {


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

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?

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

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

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"

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?

SparkQA · 2017-03-30T12:44:45Z

Test build #75386 has finished for PR 17472 at commit 632161b.

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

samelamin · 2017-03-30T13:06:50Z

@gatorsmile @srowen how does it look now?

gatorsmile · 2017-03-30T14:33:01Z

LGTM

srowen · 2017-03-30T15:09:00Z

Merged to master

…PPC64 architectures as supporting unaligned access ## What changes were proposed in this pull request? This PR is backport of #17472 to Spark 2.1 java.nio.Bits.unaligned() does not return true for the ppc64le arch. see [https://bugs.openjdk.java.net/browse/JDK-8165231](https://bugs.openjdk.java.net/browse/JDK-8165231) Check architecture in Platform.java ## How was this patch tested? unit test Author: Kazuaki Ishizaki <ishizaki@jp.ibm.com> Closes#17509 from kiszk/branch-2.1.

kiszk reviewed Mar 29, 2017
View reviewed changes

Comment threadcommon/unsafe/src/main/java/org/apache/spark/unsafe/Platform.java Outdated
kiszkMar 29, 2017
Copy link
Copy Markdown
Member
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spark uses two spaces for an indent.

gatorsmile reviewed Mar 29, 2017
View reviewed changes

samelamin force-pushed the SPARK-19999 branch from 1cd8ffd to 6f2f3a6Compare March 29, 2017 21:25

SPARK-19999 - fix jdk bug
bcef03d

samelamin force-pushed the SPARK-19999 branch from 6f2f3a6 to bcef03dCompare March 29, 2017 21:26

indentation keeps changing because of intellij
8785664

set default indentation to 2
799effa

gatorsmile reviewed Mar 29, 2017
View reviewed changes

move and clarify comment
bf7cc24

gatorsmile reviewed Mar 29, 2017
View reviewed changes

kiszk reviewed Mar 30, 2017
View reviewed changes

define arch befote if statement
77777e9

srowen requested changes Mar 30, 2017
View reviewed changes

samelamin changed the title ~~[SPARK-19999]: Fix for flakey tests due to java.nio.Bits.unaligned() …~~[SPARK-19999]: Accommodate a new architecture that is not supported. Mar 30, 2017

samelamin changed the title ~~[SPARK-19999]: Accommodate a new architecture that is not supported.~~ [SPARK-19999]: Workaround JDK-8165231 to identify PPC64 architectures as supporting unaligned accessMar 30, 2017

use equals as per feedback from pr
632161b

srowen approved these changes Mar 30, 2017
View reviewed changes

asfgit closed this in 258bff2Mar 30, 2017

kiszk added a commit to kiszk/spark that referenced this pull request Apr 2, 2017
backportapache#17472
5c93197

kiszk mentioned this pull request Apr 2, 2017
[BACKPORT-2.1][SPARK-19999][core] Workaround JDK-8165231 to identify PPC64 architectures as supporting unaligned access #17509
Closed

Uh oh!

Conversation

samelamin commented Mar 29, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

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!

samelamin commented Mar 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samelamin commented Mar 29, 2017

Uh oh!

gatorsmile commented Mar 29, 2017

Uh oh!

samelamin commented Mar 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samelamin commented Mar 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gatorsmile commented Mar 29, 2017

Uh oh!

gatorsmile commented Mar 29, 2017

Uh oh!

SparkQA commented Mar 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Mar 30, 2017

Uh oh!

samelamin commented Mar 30, 2017

Uh oh!

samelamin commented Mar 30, 2017

Uh oh!

srowen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samelaminMar 30, 2017• 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!

srowenMar 30, 2017• 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!

SparkQA commented Mar 30, 2017

Uh oh!

samelamin commented Mar 30, 2017

Uh oh!

gatorsmile commented Mar 30, 2017

Uh oh!

srowen commented Mar 30, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

samelamin commented Mar 29, 2017•
edited
Loading

samelaminMar 30, 2017•
edited
Loading

srowenMar 30, 2017•
edited
Loading