8250902: Implement MD5 Intrinsics on x86 #806

phohensee · 2022-02-03T20:19:10Z

I'd like to backport MD5 intrinsification because it improves security related performance. I understand that it may be a bridge too far for 11u. In that case we (Amazon) intend to backport it to our internal 11u distro followed by Corretto.

Testing: tier1, including the new and existing sha tests.

Ludovic Henry did an 11u backport in 2020 (see JDK-8251319), but I can't find a review request on the jdk-updates-dev list. My backport is essentially identical to his, and is based on the backport from jdk15u. It was not clean.

Copyright date conflicts.
Context differences.
Changing SHAOptionsBase to DigestOptionsBase.
Rewriting Assembler::roll() to use emit_int8() because emit_int16() and emit_24() don't exist in 11.

This backport would be immediately followed by a backport of JDK-8251260 to fix two tests. After that, I'd like to backport the aarch64 version in JDK-8251216.

Progress

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

Issue

JDK-8250902: Implement MD5 Intrinsics on x86

Reviewers

Bernhard Urban-Forster (@lewurm - no project role) ⚠️ Review applies to 9fd44b5
Ludovic Henry (@luhenry - no project role) ⚠️ Review applies to 9fd44b5
Volker Simonis (@simonis - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk11u-dev pull/806/head:pull/806
$ git checkout pull/806

Update a local copy of the PR:
$ git checkout pull/806
$ git pull https://git.openjdk.java.net/jdk11u-dev pull/806/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 806

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk11u-dev/pull/806.diff

bridgekeeper · 2022-02-03T20:19:24Z

👋 Welcome back phh! 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 · 2022-02-03T20:20:25Z

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

mlbridge · 2022-02-03T20:26:50Z

Webrevs

01: Full - Incremental (d033727c)
00: Full (9fd44b55)

lewurm · 2022-02-04T14:39:53Z

There was a proposal to backport it to 11u, but to the wrong mailing list: http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2020-August/039412.html

I think it has baked enough on tip now 🙂 /cc @luhenry

lewurm

Looks good to me, but I'm not an official reviewer.

phohensee · 2022-02-04T15:32:07Z

Ludovic's the original author, which is as "official" as it gets. Thanks for the review, Ludovic!

phohensee · 2022-02-04T15:34:25Z

Thanks for the review, Bernhard!

simonis

Hi Paul,
thanks for doing this downport.
The changes look good except for one comment in CheckGraalIntrinsics.java
Best regards,
Volker

simonis · 2022-02-07T09:18:41Z

...raalvm.compiler.hotspot.test/src/org/graalvm/compiler/hotspot/test/CheckGraalIntrinsics.java

@@ -390,6 +390,11 @@ public CheckGraalIntrinsics() {
                            "jdk/jfr/internal/JVM.getEventWriter()Ljava/lang/Object;");
        }

+        if (isJDK16OrHigher()) {


If we downport the MD5 intrinsic to JDK11, shouldn't this be changed to isJDK11OrHigher()?

simonis · 2022-02-07T09:20:01Z

...raalvm.compiler.hotspot.test/src/org/graalvm/compiler/hotspot/test/CheckGraalIntrinsics.java

+    private static boolean isJDK16OrHigher() {
+        return JavaVersionUtil.JAVA_SPEC >= 16;
+    }
+


I don't think we need these checks here at all because the GraalVM in the JDK11 repository will probably never support JDK15 or 16.

phohensee · 2022-02-07T12:56:50Z

Fixed, see incremental webrev. Thanks for the review!

simonis

Thanks, looks good now.

openjdk · 2022-02-07T13:23:44Z

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

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

8250902: Implement MD5 Intrinsics on x86

Reviewed-by: burban, luhenry, simonis

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 no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

phohensee · 2022-02-07T14:12:55Z

Thanks, Volker. Tagged.

phohensee · 2022-02-17T16:32:24Z

JBS issue was tagged 10 days ago. May I have a decision please? March 1st is 11.0.15 rampdown and quite close.

phohensee · 2022-02-23T18:55:51Z

Backport rejected.

Backport 339016a

9fd44b5

openjdk bot changed the title ~~Backport 339016a0f2cbbbbc3560e50c6726e14afbf547f6~~ 8250902: Implement MD5 Intrinsics on x86 Feb 3, 2022

openjdk bot added backport rfr Pull request is ready for review labels Feb 3, 2022

lewurm approved these changes Feb 4, 2022

View reviewed changes

luhenry approved these changes Feb 4, 2022

View reviewed changes

simonis suggested changes Feb 7, 2022

View reviewed changes

Paul Hohensee added 3 commits February 7, 2022 12:38

8250902: Implement MD5 Intrinsics on x86

0ba8593

Merge branch 'master' into backport-339016a0

fddfd99

8250902: Implement MD5 Intrinsics on x86

d033727

simonis approved these changes Feb 7, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Feb 7, 2022

phohensee closed this Feb 23, 2022

phohensee deleted the backport-339016a0 branch February 24, 2022 14:45

8250902: Implement MD5 Intrinsics on x86 #806

8250902: Implement MD5 Intrinsics on x86 #806

phohensee commented Feb 3, 2022 •

edited by openjdk bot

bridgekeeper bot commented Feb 3, 2022

openjdk bot commented Feb 3, 2022

mlbridge bot commented Feb 3, 2022 •

edited

lewurm commented Feb 4, 2022 •

edited

lewurm left a comment

phohensee commented Feb 4, 2022 •

edited

phohensee commented Feb 4, 2022

simonis left a comment

simonis Feb 7, 2022

simonis Feb 7, 2022

phohensee commented Feb 7, 2022

simonis left a comment

openjdk bot commented Feb 7, 2022 •

edited

phohensee commented Feb 7, 2022

phohensee commented Feb 17, 2022 •

edited

phohensee commented Feb 23, 2022

8250902: Implement MD5 Intrinsics on x86 #806

8250902: Implement MD5 Intrinsics on x86 #806

Conversation

phohensee commented Feb 3, 2022 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Feb 3, 2022

openjdk bot commented Feb 3, 2022

mlbridge bot commented Feb 3, 2022 • edited

Webrevs

lewurm commented Feb 4, 2022 • edited

lewurm left a comment

Choose a reason for hiding this comment

phohensee commented Feb 4, 2022 • edited

phohensee commented Feb 4, 2022

simonis left a comment

Choose a reason for hiding this comment

simonis Feb 7, 2022

Choose a reason for hiding this comment

simonis Feb 7, 2022

Choose a reason for hiding this comment

phohensee commented Feb 7, 2022

simonis left a comment

Choose a reason for hiding this comment

openjdk bot commented Feb 7, 2022 • edited

phohensee commented Feb 7, 2022

phohensee commented Feb 17, 2022 • edited

phohensee commented Feb 23, 2022

phohensee commented Feb 3, 2022 •

edited by openjdk bot

mlbridge bot commented Feb 3, 2022 •

edited

lewurm commented Feb 4, 2022 •

edited

phohensee commented Feb 4, 2022 •

edited

openjdk bot commented Feb 7, 2022 •

edited

phohensee commented Feb 17, 2022 •

edited