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

Improve handling of endianness #8294

Merged
merged 3 commits into from
Feb 7, 2020

Conversation

simonameng
Copy link
Contributor

@simonameng simonameng commented Jan 13, 2020

  • Add native function isBigEndian in JITHelpers.java following the implementation of Java_sun_misc_Unsafe_isBigEndian0.

  • Replace IS_PLATFORM_LITTLE_ENDIAN with the new implemented function isBigEndian in JITHelpers.java and remove IS_PLATFORM_LITTLE_ENDIAN relevant code.

  • Use putByteInArrayByIndex function to put the zero byte into the right index into char[], instead of checking the byteOrder and then doing &= 0x00FF or &= 0xFF00

  • Inline and fold away java/nio/ByteOrder.nativeOrder and java/nio/Bits.byteOrder. The reason I inline and fold away java/nio/Bits.byteOrder is that JDK8 calls Bits.byteOrder in ByteOrder.nativeOrder instead of ByteOrder.NATIVE_ORDER, which is a static field in JDK11.

Closes: #4741

@fjeremic
Copy link
Contributor

Changes look good! We'll need to carry out performance experiments once we get an approval from someone on the optimizer side.

Copy link
Contributor

@andrewcraik andrewcraik left a comment

Choose a reason for hiding this comment

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

I would appreciate a justification for the change to JITHelpers - adding the recognized methods so that we can try to inline them and do good things for the java/nio package seems reasonable, but the changes to JITHelpers seem like a step backwards since we cannot constant fold and add a complex dependency from JITHelpers which it has never previously had.

@fjeremic fjeremic self-assigned this Jan 29, 2020
@fjeremic
Copy link
Contributor

fjeremic commented Feb 3, 2020

Jenkins test sanity all jdk8,jdk11

@pshipton
Copy link
Member

pshipton commented Feb 3, 2020

@fjeremic pls don't run all for more than one jdk version at a time. I believe your request caused jenkins to restart, and we lost all the running builds.

@fjeremic
Copy link
Contributor

fjeremic commented Feb 3, 2020

@fjeremic pls don't run all for more than one jdk version at a time. I believe your request caused jenkins to restart, and we lost all the running builds.

Sorry about that. Will not do this in the future. Can we disable this combination on the infrastructure side to prevent people from launching the builds? Is there an issue tracking a fix? IIRC this used to work in the past.

@fjeremic
Copy link
Contributor

fjeremic commented Feb 3, 2020

Jenkins test sanity all jdk8

@pshipton
Copy link
Member

pshipton commented Feb 3, 2020

Can we disable this combination on the infrastructure side?

@AdamBrousseau ?

Is there an issue tracking a fix?

https://bugs.eclipse.org/bugs/show_bug.cgi?id=553326

@pshipton
Copy link
Member

pshipton commented Feb 3, 2020

IIRC this used to work in the past.

https://openj9.slack.com/archives/C8PQL5N65/p1575512682081300

@AdamBrousseau
Copy link
Contributor

We've disabled keywords all for versions and all for testing. That doesn't stop a user from spelling them all out. For platforms, all maps to a (large) subset of platforms (found here). One caveat is this only maps in a PR build right now, so personal builds (internally) which launch all platforms actually get all platforms, which is a different problem/discussion. I don't want to hardcode logic for "if you launch all, limit to 1 version", since we can run 4 versions simultaneously internally just fine.

@fjeremic
Copy link
Contributor

fjeremic commented Feb 3, 2020

Ok, I see so this is an external machine connection flakiness issue. I agree with not hardcoding. I'll keep an eye if I see people launching too many builds to inform them of this restriction. Thanks!

Jenkins test sanity all jdk11

@fjeremic
Copy link
Contributor

fjeremic commented Feb 5, 2020

@andrewcraik this change is ready. I'll wait for your approval before merging.

@andrewcraik
Copy link
Contributor

Can we please get an updated PR message? The commits need a squash too

Copy link
Contributor

@fjeremic fjeremic left a comment

Choose a reason for hiding this comment

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

Revoking approval to prevent myself from accidentally merging. @simonameng let's address the previous comment and we should be ready here.

@simonameng simonameng force-pushed the testEndian2 branch 4 times, most recently from 54938c0 to ca2381c Compare February 6, 2020 21:06
Use `putByteInArrayByIndex` function to put the zero byte into the
right index of `char[]`, instead of checking the `byteOrder` and
then doing `&= 0x00FF` or `&= 0xFF00`

Signed-off-by: simonameng <simonameng97@gmail.com>
Add `nativeOrder` and `byteOrder` to j9 method list, worth inlining
list and 'foldFinalFieldsIn' list.

Change copyright in relevant files.

Signed-off-by: simonameng <simonameng97@gmail.com>
Add native function 'isBigEndian' following the implementation
of 'Java_sun_misc_Unsafe_isBigEndian0'.

Deprecate `IS_PLATFORM_LITTLE_ENDIAN` with `isBigEndian` function.

Signed-off-by: simonameng <simonameng97@gmail.com>
@fjeremic
Copy link
Contributor

fjeremic commented Feb 6, 2020

Force push link says there is nothing different between this version and the last, sans the commit message fixup and squashing. For sanity I'll just kick off a single build, although it is not warranted given previous testing.

Jenkins test sanity xlinux jdk8,jdk11

@fjeremic fjeremic merged commit 9de47a9 into eclipse-openj9:master Feb 7, 2020
@simonameng simonameng deleted the testEndian2 branch February 18, 2020 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve handling of endianness in the VM and JIT
5 participants