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

8268276: Base64 Decoding optimization for x86 using AVX-512 #840

Closed
wants to merge 7 commits into from

Conversation

asgibbons
Copy link
Contributor

@asgibbons asgibbons commented Oct 25, 2022

This is a backport of JDK-8268276: Base64 Decoding optimization for x86 using AVX-512.

We have had numerous customer requests for this functionality to be backported due to the ~19x performance improvement.

Original patch does not apply cleanly to 17u, because of a bug that was identified in the original commit, but not fixed until the later Base64 Encode acceleration enhancement was complete. The fix is on line 6039 of src/hotspot/cpu/x86/stubGenerator_x86_64.cpp and has been included in this PR. All four "translatedX" registers needed to be checked for illegal characters in the input stream.

Risk: I view the risk of this backport to be minimal. This code has been in use for many months with no bugs reported.

Testing: x86_64 build, affected tests, tier1, tier2

Thanks,
--Scott


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-8268276: Base64 Decoding optimization for x86 using AVX-512

Reviewers

Contributors

  • Derek White <drwhite@openjdk.org>
  • Sandhya Viswanathan <sviswanathan@openjdk.org>

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk17u-dev pull/840/head:pull/840
$ git checkout pull/840

Update a local copy of the PR:
$ git checkout pull/840
$ git pull https://git.openjdk.org/jdk17u-dev pull/840/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 840

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk17u-dev/pull/840.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Oct 25, 2022

👋 Welcome back asgibbons! 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 openjdk bot changed the title Backport c37988d0793b24d98d285530dfda69999a227937 8268276: Base64 Decoding optimization for x86 using AVX-512 Oct 25, 2022
@openjdk
Copy link

openjdk bot commented Oct 25, 2022

This backport pull request has now been updated with issue from the original commit.

@openjdk openjdk bot added backport rfr Pull request is ready for review labels Oct 25, 2022
@mlbridge
Copy link

mlbridge bot commented Oct 25, 2022

Webrevs

@asgibbons
Copy link
Contributor Author

/label add hotspot

@openjdk
Copy link

openjdk bot commented Oct 25, 2022

@asgibbons
The label hotspot is not a valid label.
These labels are valid:

@dwhite-intel
Copy link

Hi Scott,

Can you post the diff between this backport and the original JDK-8268276 patch? I'm guessing it's pretty small, but it will be good to see.

Thanks!

  • Derek

__ vpternlogd(input0, 0xfe, input1, input2, Assembler::AVX_512bit);

__ vpternlogd(input3, 0xfe, translated0, translated1, Assembler::AVX_512bit);
__ vpternlogd(input0, 0xfe, translated2, translated3, Assembler::AVX_512bit);
Copy link
Contributor Author

@asgibbons asgibbons Oct 28, 2022

Choose a reason for hiding this comment

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

This line (6039) was changed from

  __ vpternlogd(input0, 0xfe, translated1, translated2, Assembler::AVX_512bit);

It was changed to:

  __ vpternlogd(input0, 0xfe, translated2, translated3, Assembler::AVX_512bit);

@openjdk
Copy link

openjdk bot commented Oct 31, 2022

⚠️ @asgibbons This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

__ vpermb(merged0, pack24bits, merged0, Assembler::AVX_512bit);

__ evmovdqub(Address(dest, dp), k2, merged0, true, Assembler::AVX_512bit);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lines 6097, 6098, and 6121 were hand-merged from JDK-8273108.

@asgibbons
Copy link
Contributor Author

@dwhite-intel Did I answer your question to your satisfaction? I'd appreciate a review if you have time. Thanks!

@asgibbons
Copy link
Contributor Author

So, the original patch applied cleanly, then I included the two bugfixes that were subsequently found (JDK-8273108 and JDK-8269404), one relating to improper illegal character discovery and the other relating to writing past end-of-string.

@dwhite-intel
Copy link

This looks good to me. Still need a (R)eviewer.

Copy link
Member

@phohensee phohensee left a comment

Choose a reason for hiding this comment

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

Looks like you didn't backport the change to .gitignore. Seems unrelated to the rest of the patch, but harmless enough.

Other than that, lgtm.

@asgibbons
Copy link
Contributor Author

Thanks, @phohensee . This is an unneeded artifact of me using VS for editing. I do not believe it affects anything.

Copy link
Member

@phohensee phohensee left a comment

Choose a reason for hiding this comment

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

Yes, it doesn't affect anything, but might be a merge conflict down the line. Let's see what the approvers say. Lgtm.

@openjdk
Copy link

openjdk bot commented Nov 2, 2022

@asgibbons 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:

8268276: Base64 Decoding optimization for x86 using AVX-512

Co-authored-by: Derek White <drwhite@openjdk.org>
Co-authored-by: Sandhya Viswanathan <sviswanathan@openjdk.org>
Reviewed-by: phh

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 30 new commits pushed to the master branch:

  • eef3c96: 8282600: SSLSocketImpl should not use user_canceled workaround when not necessary
  • e609398: 8287011: Improve container information
  • d5fedc5: 8244670: convert clhsdb "whatis" command from javascript to java
  • 5fa5392: 8273578: javax/swing/JMenu/4515762/bug4515762.java fails on macOS 12
  • 0698930: 8295288: Some vm_flags tests associate with a wrong BugID
  • 9fd15d5: 8233648: [TESTBUG] DefaultMenuBarTest.java failing on macos
  • 6a45f75: 8280948: Write a regression test for JDK-4659800
  • aa98d09: 8291456: com/sun/jdi/ClassUnloadEventTest.java failed with: Wrong number of class unload events: expected 10 got 4
  • 9732004: 8292879: com/sun/jdi/ClassUnloadEventTest.java failed due to classes not unloading
  • 8194120: 8292880: Improve debuggee logging for com/sun/jdi/ClassUnloadEventTest.java
  • ... and 20 more: https://git.openjdk.org/jdk17u-dev/compare/525b9fc903068c9e6518f00c7c6e6df20631f5f4...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 (@phohensee) 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).

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Nov 2, 2022
@openjdk
Copy link

openjdk bot commented Nov 2, 2022

@asgibbons dwhite-intel was not found in the census.

Syntax: /contributor (add|remove) [@user | openjdk-user | Full Name <email@address>]. For example:

  • /contributor add @openjdk-bot
  • /contributor add duke
  • /contributor add J. Duke <duke@openjdk.org>

User names can only be used for users in the census associated with this repository. For other contributors you need to supply the full name and email address.

@openjdk
Copy link

openjdk bot commented Nov 2, 2022

@asgibbons sviswa7 was not found in the census.

Syntax: /contributor (add|remove) [@user | openjdk-user | Full Name <email@address>]. For example:

  • /contributor add @openjdk-bot
  • /contributor add duke
  • /contributor add J. Duke <duke@openjdk.org>

User names can only be used for users in the census associated with this repository. For other contributors you need to supply the full name and email address.

@asgibbons
Copy link
Contributor Author

/contributor add drwhite

@openjdk
Copy link

openjdk bot commented Nov 2, 2022

@asgibbons
Contributor Derek White <drwhite@openjdk.org> successfully added.

@openjdk
Copy link

openjdk bot commented Nov 2, 2022

@asgibbons sandhya was not found in the census.

Syntax: /contributor (add|remove) [@user | openjdk-user | Full Name <email@address>]. For example:

  • /contributor add @openjdk-bot
  • /contributor add duke
  • /contributor add J. Duke <duke@openjdk.org>

User names can only be used for users in the census associated with this repository. For other contributors you need to supply the full name and email address.

@asgibbons
Copy link
Contributor Author

/contributor add sviswanathan

@openjdk
Copy link

openjdk bot commented Nov 2, 2022

@asgibbons
Contributor Sandhya Viswanathan <sviswanathan@openjdk.org> successfully added.

@asgibbons
Copy link
Contributor Author

/integrate defer

@openjdk openjdk bot added the deferred label Nov 2, 2022
@openjdk
Copy link

openjdk bot commented Nov 2, 2022

@asgibbons Integration of this pull request has been deferred and may be completed by any project committer using the /integrate pull request command.

@asgibbons
Copy link
Contributor Author

/integrate undefer
/integrate

@openjdk openjdk bot removed the deferred label Nov 2, 2022
@openjdk
Copy link

openjdk bot commented Nov 2, 2022

@asgibbons Integration of this pull request is no longer deferred and may only be integrated by the author (@asgibbons)using the /integrate pull request command.
This pull request may now only be integrated by the author

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Nov 2, 2022
@openjdk
Copy link

openjdk bot commented Nov 2, 2022

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

@asgibbons
Copy link
Contributor Author

/integrate defer

@openjdk openjdk bot added the deferred label Nov 2, 2022
@openjdk
Copy link

openjdk bot commented Nov 2, 2022

@asgibbons Integration of this pull request has been deferred and may be completed by any project committer using the /integrate pull request command.

@asgibbons
Copy link
Contributor Author

Trying to do dependent PRs instead of piecemealing. Let me know if I'm doing something wrong.

@asgibbons asgibbons closed this Nov 7, 2022
@asgibbons asgibbons deleted the Decode-backport branch November 7, 2022 19:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport deferred ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored
3 participants