8315970: Big-endian issues after JDK-8310929 #15652

wenshao · 2023-09-10T16:39:30Z

https://bugs.openjdk.org/browse/JDK-8310929

We're getting test failures on AIX:
compiler/intrinsics/Test8215792.java
compiler/intrinsics/string/TestStringIntrinsics.java
runtime/CompactStrings/TestMethodNames.java
runtime/StringIntrinsic/StringIndexOfChar.java
Is there a problem with Big Endian?

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8315970: Big-endian issues after JDK-8310929 (Enhancement - P4)

Reviewers

Martin Doerr (@TheRealMDoerr - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/15652/head:pull/15652
$ git checkout pull/15652

Update a local copy of the PR:
$ git checkout pull/15652
$ git pull https://git.openjdk.org/jdk.git pull/15652/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 15652

View PR using the GUI difftool:
$ git pr show -t 15652

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/15652.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2023-09-10T16:41:04Z

👋 Welcome back wenshao! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2023-09-10T16:42:54Z

@wenshao The following label will be automatically applied to this pull request:

core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

cl4es · 2023-09-10T17:21:13Z

Filed https://bugs.openjdk.org/browse/JDK-8315970 for this.

mlbridge · 2023-09-10T18:02:05Z

Webrevs

01: Full - Incremental (e1e925b8)
00: Full (911bbce8)

RogerRiggs

I'd suggest backing out the whole commit and resumitting after the fix and more complete testing.

wenshao · 2023-09-10T18:44:20Z

I don't have a big endian environment so I can't test it. I need help from @TheRealMDoerr

cl4es · 2023-09-10T22:27:48Z

src/java.base/share/classes/java/lang/StringUTF16.java

+
+    private static int inflatePacked(int v) {
+        int packed = (int) StringLatin1.PACKED_DIGITS[v];
+        return ((packed & 0xFF) << HI_BYTE_SHIFT)


I'm not sure this is correct.

Compare StringUTF16::putChar where these constants are used to shift right to extract the equivalent byte from a value:

val[index++] = (byte)(c >> HI_BYTE_SHIFT); val[index] = (byte)(c >> LO_BYTE_SHIFT);

I.e., when inflating a byte 0xaa to a char encoded into a byte[] we end up with 0xaa00 on big-endian. Inflating a short literal 0xaabb encoding two chars logically I think will need to consider each byte in isolation, ending up with 0xaa00bb00 (in little-endian notation). Or maybe it's 0xbb00aa00. Ugh..

Since HI_BYTE_SHIFT is 8 on big-endian and 0 on little-endian I guess this might just work:

return ((packed & 0xFF) << 16 + HI_BYTE_SHIFT) | ((packed & 0xFF00) << HI_BYTE_SHIFT)

.. but we really need to re-examine, prototype and test this out thoroughly on a big-endian system. I second @RogerRiggs notion that the best course of action right now is to back out #14699 and redo it with big-endianness issues resolved.

I'm also not sure if this PR is correct.

wenshao · 2023-09-10T23:52:13Z

Could it be caused by using VarHandle/ByteArrayLittleEndian?

TheRealMDoerr · 2023-09-11T08:47:03Z

This helps. I'll run more tests.

TheRealMDoerr

This looks reasonable. The tests have passed on linux Big Endian and AIX. Thanks for fixing it so quickly. (Otherwise, backout and re-do would have been a good option as well.)

openjdk · 2023-09-11T11:01:56Z

⚠️ @wenshao the full name on your profile does not match the author name in this pull requests' HEAD commit. If this pull request gets integrated then the author name from this pull requests' HEAD commit will be used for the resulting commit. If you wish to push a new commit with a different author name, then please run the following commands in a local repository of your personal fork:

$ git checkout fix_14699_big_endian
$ git commit --author='Preferred Full Name <you@example.com>' --allow-empty -m 'Update full name'
$ git push

openjdk · 2023-09-11T11:01:57Z

@wenshao This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8315970: Big-endian issues after JDK-8310929

Reviewed-by: mdoerr

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 28 new commits pushed to the master branch:

ae08143: 8315611: Open source swing text/html and tree test
7b3e697: 8315855: G1: Revise signature of set_humongous_candidate
1941290: 8315942: Sort platform enums and definitions after JDK-8304913 follow-ups
996b336: 8315781: Reduce the max value of GCDrainStackTargetSize
35bccac: 8315841: RISC-V: Check for hardware TSO support
a04c6c1: 8315609: Open source few more swing text/html tests
dab1c21: 8314491: Linux: jexec launched via PATH fails to find java
9a83d55: 8315373: Change VirtualThread to unmount after freezing, re-mount before thawing
68f6941: 8314452: Explicitly indicate inlining success/failure in PrintInlining
b482e6d: 8315580: Remove unused java_lang_String::set_value_raw()
... and 18 more: https://git.openjdk.org/jdk/compare/4b43c25fe382b5ee805a2d1b173fdd32d8da7fad...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@RogerRiggs, @TheRealMDoerr) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

wenshao · 2023-09-11T11:29:50Z

@TheRealMDoerr
Can you also help test the version Webrevs 00: Full (911bbce8) ?

TheRealMDoerr · 2023-09-11T12:50:43Z

Your earlier version didn't work. The one which I have successfully tested is after 2nd commit.

wenshao · 2023-09-11T13:29:43Z

/integrate

openjdk · 2023-09-11T13:30:23Z

@wenshao
Your change (at version e1e925b) is now ready to be sponsored by a Committer.

TheRealMDoerr · 2023-09-11T13:37:25Z

GHA Pre-submit test results look good. Tests on AIX as well. Let's ship it!
/sponsor

openjdk · 2023-09-11T13:39:31Z

Going to push as commit 4cb4637.
Since your change was applied there have been 28 commits pushed to the master branch:

ae08143: 8315611: Open source swing text/html and tree test
7b3e697: 8315855: G1: Revise signature of set_humongous_candidate
1941290: 8315942: Sort platform enums and definitions after JDK-8304913 follow-ups
996b336: 8315781: Reduce the max value of GCDrainStackTargetSize
35bccac: 8315841: RISC-V: Check for hardware TSO support
a04c6c1: 8315609: Open source few more swing text/html tests
dab1c21: 8314491: Linux: jexec launched via PATH fails to find java
9a83d55: 8315373: Change VirtualThread to unmount after freezing, re-mount before thawing
68f6941: 8314452: Explicitly indicate inlining success/failure in PrintInlining
b482e6d: 8315580: Remove unused java_lang_String::set_value_raw()
... and 18 more: https://git.openjdk.org/jdk/compare/4b43c25fe382b5ee805a2d1b173fdd32d8da7fad...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-09-11T13:39:40Z

@TheRealMDoerr @wenshao Pushed as commit 4cb4637.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

cl4es · 2023-09-11T13:40:32Z

Your earlier version didn't work. The one which I have successfully tested is after 2nd commit.

I think this looks OK.

This patch probably reverts performance numbers on little-endian on some measures to pre-JDK-8310929 levels. A follow-up could examine if we can recuperate, e.g. differentiate the logic on big-endian, e.g. something like:

        charPos -= 2;
        if (BIG_ENDIAN) {
            putPair(..);
        } else {
            int packed = (int) StringLatin1.PACKED_DIGITS[-i];
            int inflated = ((packed & 0xFF00) << 8) | (packed & 0xFF);
            ByteArrayLittleEndian.setInt(buf, charPos << 1, inflated);
        }

It might also work generally if we made int inflated = ((packed & 0xFF) << (16 + HI_BYTE_SHIFT)) | ((packed & 0xFF00) << HI_BYTE_SHIFT), but I have no way to test that and the performance of ByteArrayLittleEndian might be poor on AIX.

TheRealMDoerr · 2023-09-11T13:53:19Z

Shouldn't this get optimized by the JIT compilers? Why is ByteArrayLittleEndian supposed to be faster?
Note that there are still several Big Endian platforms: AIX, s390x and old linux ppc64 (still kept alive)

wenshao · 2023-09-11T14:01:57Z

It will be faster to use ByteArrayLittle or ByteArrayLittleEndian. ByteArrayLittleEndian has an Integer.reverseBytes operation under the bigendian endian platform.

cl4es · 2023-09-11T14:10:15Z

Shouldn't this get optimized by the JIT compilers? Why is ByteArrayLittleEndian supposed to be faster? Note that there are still several Big Endian platforms: AIX, s390x and old linux ppc64 (still kept alive)

And none of these are covered by Oracle-internal or GHA testing, sadly.

It'd be interesting to see performance numbers for putPair vs ByteArrayLittleEndian.setInt in isolation on x86. I.e. before and after this PR. When I've tested a similar thing for #15591 I saw a slight win on a two-byte pair (that might be due improved inlining decisions) but regressions on 4-byte or larger writes.

fix PR openjdk#14699 big endian error

911bbce

openjdk bot added the core-libs core-libs-dev@openjdk.org label Sep 10, 2023

wenshao mentioned this pull request Sep 10, 2023

8311207: Cleanup for Optimization for UUID.toString #14745

Closed

3 tasks

wenshao changed the title ~~fix PR 14699 big endian problem~~ 8315970: Big-endian issues after JDK-8310929 Sep 10, 2023

openjdk bot added the rfr Pull request is ready for review label Sep 10, 2023

RogerRiggs reviewed Sep 10, 2023

View reviewed changes

cl4es reviewed Sep 10, 2023

View reviewed changes

remove ByteArrayLittleEndian

e1e925b

TheRealMDoerr approved these changes Sep 11, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 11, 2023

openjdk bot added the sponsor Pull request is ready to be sponsored label Sep 11, 2023

openjdk bot added the integrated Pull request has been integrated label Sep 11, 2023

openjdk bot closed this Sep 11, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 11, 2023

openjdk bot removed the sponsor Pull request is ready to be sponsored label Sep 11, 2023

wenshao mentioned this pull request Oct 27, 2023

[Back port] 8310929: Optimization for integer/long toString dragonwell-project/dragonwell11#682

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8315970: Big-endian issues after JDK-8310929 #15652

8315970: Big-endian issues after JDK-8310929 #15652

wenshao commented Sep 10, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Sep 10, 2023

openjdk bot commented Sep 10, 2023

cl4es commented Sep 10, 2023

mlbridge bot commented Sep 10, 2023 •

edited

Loading

RogerRiggs left a comment

wenshao commented Sep 10, 2023

cl4es Sep 10, 2023 •

edited

Loading

wenshao Sep 10, 2023

wenshao commented Sep 10, 2023

TheRealMDoerr commented Sep 11, 2023

TheRealMDoerr left a comment

openjdk bot commented Sep 11, 2023

openjdk bot commented Sep 11, 2023 •

edited

Loading

wenshao commented Sep 11, 2023

TheRealMDoerr commented Sep 11, 2023

wenshao commented Sep 11, 2023

openjdk bot commented Sep 11, 2023

TheRealMDoerr commented Sep 11, 2023

openjdk bot commented Sep 11, 2023

openjdk bot commented Sep 11, 2023

cl4es commented Sep 11, 2023 •

edited

Loading

TheRealMDoerr commented Sep 11, 2023

wenshao commented Sep 11, 2023

cl4es commented Sep 11, 2023

8315970: Big-endian issues after JDK-8310929 #15652

8315970: Big-endian issues after JDK-8310929 #15652

Conversation

wenshao commented Sep 10, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Sep 10, 2023

openjdk bot commented Sep 10, 2023

cl4es commented Sep 10, 2023

mlbridge bot commented Sep 10, 2023 • edited Loading

Webrevs

RogerRiggs left a comment

Choose a reason for hiding this comment

wenshao commented Sep 10, 2023

cl4es Sep 10, 2023 • edited Loading

Choose a reason for hiding this comment

wenshao Sep 10, 2023

Choose a reason for hiding this comment

wenshao commented Sep 10, 2023

TheRealMDoerr commented Sep 11, 2023

TheRealMDoerr left a comment

Choose a reason for hiding this comment

openjdk bot commented Sep 11, 2023

openjdk bot commented Sep 11, 2023 • edited Loading

wenshao commented Sep 11, 2023

TheRealMDoerr commented Sep 11, 2023

wenshao commented Sep 11, 2023

openjdk bot commented Sep 11, 2023

TheRealMDoerr commented Sep 11, 2023

openjdk bot commented Sep 11, 2023

openjdk bot commented Sep 11, 2023

cl4es commented Sep 11, 2023 • edited Loading

TheRealMDoerr commented Sep 11, 2023

wenshao commented Sep 11, 2023

cl4es commented Sep 11, 2023

wenshao commented Sep 10, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Sep 10, 2023 •

edited

Loading

cl4es Sep 10, 2023 •

edited

Loading

openjdk bot commented Sep 11, 2023 •

edited

Loading

cl4es commented Sep 11, 2023 •

edited

Loading