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

8271567: AArch64: AES Galois CounterMode (GCM) interleaved implementation using vector instructions #87

Closed
wants to merge 2 commits into from

Conversation

theRealAph
Copy link
Contributor

@theRealAph theRealAph commented Sep 24, 2021

This is a backport of the big AES/GCM patch from JDK head. It's a
major change and it's had very little time (almost a day) to mature in
head, so perhaps it shouldn't be backported for some time, However, there is a good reason for a
backport: OpenJDK on x86 has a major advantage. AES/GCM is an
important cipher, the current AArch64 implementation is much slower
than x86, and some workloads are severely impacted.

I'm open to all arguments about why this should or shouldn't be pushed,
and I'm quite happy to wait for another release cycle or two if people
think that's the best course of action.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8271567: AArch64: AES Galois CounterMode (GCM) interleaved implementation using vector instructions

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk17u pull/87/head:pull/87
$ git checkout pull/87

Update a local copy of the PR:
$ git checkout pull/87
$ git pull https://git.openjdk.java.net/jdk17u pull/87/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 87

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk17u/pull/87.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Sep 24, 2021

👋 Welcome back aph! 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 4f3b626a36319cbbbbdcb1c02a84486a3d4eddb6 8271567: AArch64: AES Galois CounterMode (GCM) interleaved implementation using vector instructions Sep 24, 2021
@openjdk
Copy link

openjdk bot commented Sep 24, 2021

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 Sep 24, 2021
@mlbridge
Copy link

mlbridge bot commented Sep 24, 2021

Webrevs

@theRealAph
Copy link
Contributor Author

Or perhaps include the patch but disable the intrinsic by default.

@bridgekeeper
Copy link

bridgekeeper bot commented Oct 22, 2021

@theRealAph This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@theRealAph
Copy link
Contributor Author

Hi! Can someone look at this, please?

@adinn
Copy link
Contributor

adinn commented Oct 25, 2021

I'd be happy with "include the patch but disable the intrinsic by default". That poses a pretty low risk for users who don't need this but, as you say, should a big difference for apps that need AES GCM -- enough for them to want it despite the limited upstream baking time.

@theRealAph
Copy link
Contributor Author

I'd be happy with "include the patch but disable the intrinsic by default". That poses a pretty low risk for users who don't need this but, as you say, should a big difference for apps that need AES GCM -- enough for them to want it despite the limited upstream baking time.

There's no point simply turning off acceleration for safety because this patch includes a partial rewrite of the code that gets executed even when the new accelerator is disabled, so it'd add risk without improving anything: a very bad bargain.
What I'll do instead is alter the patch to leave the old code unchanged when AES/GCM acceleration is disabled.

This is a bit of a "Physician, heal thyself!" moment -- it's what I'm forever asking back-porters to do for JDK 11u. Oops, my bad.

@theRealAph
Copy link
Contributor Author

I'm closing this PR. It's replaced by https://git.openjdk.java.net/jdk17u/pull/216

@theRealAph theRealAph closed this Oct 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport rfr Pull request is ready for review
Development

Successfully merging this pull request may close these issues.

2 participants