8296190: TestMD5Intrinsics and TestMD5MultiBlockIntrinsics don't test the intrinsics #10954

yftsai · 2022-11-02T17:30:19Z

Increase WARMUP_ITERATIONS to get MD5 hash from the intrinsic.

Both TestMD5Intrinsics and TestMD5MultiBlockIntrinsics are run with options "-XX:CompileThreshold=500 -XX:Tier4InvocationThreshold=500", so the current value is too low for the intrinsic to be executed.

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-8296190: TestMD5Intrinsics and TestMD5MultiBlockIntrinsics don't test the intrinsics

Reviewers

Evgeny Astigeevich (@eastig - Committer)
Paul Hohensee (@phohensee - Reviewer)
Volker Simonis (@simonis - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 10954

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

Using diff file

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

… the intrinsics

bridgekeeper · 2022-11-02T17:31:36Z

👋 Welcome back yftsai! 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-11-02T17:34:14Z

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

hotspot-compiler

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.

mlbridge · 2022-11-02T17:37:17Z

Webrevs

01: Full - Incremental (b11fe5a3)
00: Full (b1ff22d2)

eastig

Lgtm

phohensee

Lgtm, and trivial.

openjdk · 2022-11-02T18:17:24Z

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

8296190: TestMD5Intrinsics and TestMD5MultiBlockIntrinsics don't test the intrinsics

Reviewed-by: eastigeevich, phh, 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 706 new commits pushed to the master branch:

b685fc2: 8295685: Update Libpng to 1.6.38
94eb25a: 8256072: Eliminate JVMTI tagmap rehashing
4338f52: 8294241: Deprecate URL public constructors
68209ad: 8288232: Address typos in jar man page
2ff9d3a: 8294858: XMLStreamReader does not respect jdk.xml.maxXMLNameLimit=0 for namespace names
72f74df: 8292427: Improve specification of InflaterInputStream.fill()
53905e6: 8178355: IdentityHashMap uses identity-based comparison for values everywhere except remove(K,V) and replace(K,V,V)
25dfcbd: 8289689: (fs) Re-examine the need for normalization to Unicode Normalization Format D (macOS)
b7442d1: 8295653: Add a graph of the sealed class hierarchy for marked classes
59a13b1: 8295872: [PPC64] JfrGetCallTrace: Need pc == nullptr check before frame constructor
... and 696 more: https://git.openjdk.org/jdk/compare/d36abbe8dab3618a08a8a87f53411ed92823d471...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 (@eastig, @phohensee, @simonis) 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).

yftsai · 2022-11-02T18:21:11Z

/integrate

simonis · 2022-11-02T18:22:37Z

I'm a little surprised that this isn't detected. The tests you mention have a verification step (i.e. compiler.testlibrary.intrinsics.Verifier) which should detect this. Can you please check why that is not the case and if this also affects other intrinsic tests?

Also, it is a little unfortunate, that the new warmup value still implicitly depends on the -XX:CompileThreshold=500 setting in the actual test. It would be more stable to get that value explicitly (e.g. by using the WhiteBox API) and use a strictly bigger warmup value.

Finally, I think the actual tests should be run with -Xbatch to be sure that the corresponding methods have really been compiled.

openjdk · 2022-11-02T18:23:35Z

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

simonis

I'm a little surprised that this isn't detected. The tests you mention have a verification step (i.e. compiler.testlibrary.intrinsics.Verifier) which should detect this. Can you please check why that is not the case and if this also affects other intrinsic tests?

Also, it is a little unfortunate, that the new warmup value still implicitly depends on the -XX:CompileThreshold=500 setting in the actual test. It would be more stable to get that value explicitly (e.g. by using the WhiteBox API) and use a strictly bigger warmup value.

Finally, I think the actual tests should be run with -Xbatch to be sure that the corresponding methods have really been compiled.

phohensee · 2022-11-03T00:12:40Z

Hi, Volker. Perhaps we could let this patch go in to fix the immediate problem, and have Yifan file an issue for a definitive fix?

yftsai · 2022-11-03T00:29:54Z

The verifier detects the intrinsic compiled/inlined somewhere, but it doesn't get executed. In other words, the verifier alone didn't guarantee the intrinsic tested. Other tests like TestSHA512Intrinsics have the same problem and are fixed as well.

WARMUP_ITERATIONS is now set strictly larger than Tier4InvocationThreshold.

Both TestMD5Intrinsics and TestMD5MultiBlockIntrinsics are run with -Xbatch.

eastig · 2022-11-03T03:26:41Z

test/hotspot/jtreg/compiler/intrinsics/sha/sanity/DigestSanityTestBase.java

@@ -54,7 +54,7 @@ public class DigestSanityTestBase {
    private static final int MSG_SIZE = 1024;
    private static final int OFFSET = 0;
    private static final int ITERATIONS = 10000;
-    private static final int WARMUP_ITERATIONS = 550;
+    private static final int WARMUP_ITERATIONS = WHITE_BOX.getIntxVMFlag("Tier4InvocationThreshold").intValue() + 50;


If the flag Tier4InvocationThreshold is not set, what will happen?

The default value, 5000, would be returned.

simonis

Hi Yi-Fan,

Thanks for doing the additional changes and for confirming that the test run with -Xbatch.

Looks good now.

Best regards,
Volker

yftsai · 2022-11-03T18:22:42Z

/integrate

openjdk · 2022-11-03T18:24:10Z

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

phohensee · 2022-11-03T19:22:40Z

/sponsor

phohensee

Lgtm.

openjdk · 2022-11-03T19:26:36Z

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

b685fc2: 8295685: Update Libpng to 1.6.38
94eb25a: 8256072: Eliminate JVMTI tagmap rehashing
4338f52: 8294241: Deprecate URL public constructors
68209ad: 8288232: Address typos in jar man page
2ff9d3a: 8294858: XMLStreamReader does not respect jdk.xml.maxXMLNameLimit=0 for namespace names
72f74df: 8292427: Improve specification of InflaterInputStream.fill()
53905e6: 8178355: IdentityHashMap uses identity-based comparison for values everywhere except remove(K,V) and replace(K,V,V)
25dfcbd: 8289689: (fs) Re-examine the need for normalization to Unicode Normalization Format D (macOS)
b7442d1: 8295653: Add a graph of the sealed class hierarchy for marked classes
59a13b1: 8295872: [PPC64] JfrGetCallTrace: Need pc == nullptr check before frame constructor
... and 696 more: https://git.openjdk.org/jdk/compare/d36abbe8dab3618a08a8a87f53411ed92823d471...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-11-03T19:27:03Z

@phohensee @yftsai Pushed as commit f43bb9f.

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

8296190: TestMD5Intrinsics and TestMD5MultiBlockIntrinsics don't test…

b1ff22d

… the intrinsics

openjdk bot added the rfr Pull request is ready for review label Nov 2, 2022

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Nov 2, 2022

eastig approved these changes Nov 2, 2022

View reviewed changes

phohensee approved these changes Nov 2, 2022

View reviewed changes

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

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

simonis suggested changes Nov 2, 2022

View reviewed changes

Set WARMUP_ITERATIONS based on Tier4InvocationThreshold

b11fe5a

openjdk bot removed the sponsor Pull request is ready to be sponsored label Nov 3, 2022

eastig reviewed Nov 3, 2022

View reviewed changes

simonis approved these changes Nov 3, 2022

View reviewed changes

eastig approved these changes Nov 3, 2022

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Nov 3, 2022

phohensee approved these changes Nov 3, 2022

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Nov 3, 2022

openjdk bot closed this Nov 3, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Nov 3, 2022

yftsai mentioned this pull request Jan 5, 2023

8299158: Improve MD5 intrinsic on AArch64 openjdk/jdk17u-dev#1037

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8296190: TestMD5Intrinsics and TestMD5MultiBlockIntrinsics don't test the intrinsics #10954

8296190: TestMD5Intrinsics and TestMD5MultiBlockIntrinsics don't test the intrinsics #10954

yftsai commented Nov 2, 2022 •

edited by openjdk bot

Loading

bridgekeeper bot commented Nov 2, 2022

openjdk bot commented Nov 2, 2022

mlbridge bot commented Nov 2, 2022 •

edited

Loading

eastig left a comment

phohensee left a comment

openjdk bot commented Nov 2, 2022 •

edited

Loading

yftsai commented Nov 2, 2022

simonis commented Nov 2, 2022 •

edited

Loading

openjdk bot commented Nov 2, 2022

simonis left a comment

phohensee commented Nov 3, 2022

yftsai commented Nov 3, 2022 •

edited

Loading

eastig Nov 3, 2022

yftsai Nov 3, 2022

simonis left a comment

yftsai commented Nov 3, 2022

openjdk bot commented Nov 3, 2022

phohensee commented Nov 3, 2022

phohensee left a comment

openjdk bot commented Nov 3, 2022

openjdk bot commented Nov 3, 2022

8296190: TestMD5Intrinsics and TestMD5MultiBlockIntrinsics don't test the intrinsics #10954

8296190: TestMD5Intrinsics and TestMD5MultiBlockIntrinsics don't test the intrinsics #10954

Conversation

yftsai commented Nov 2, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Nov 2, 2022

openjdk bot commented Nov 2, 2022

mlbridge bot commented Nov 2, 2022 • edited Loading

Webrevs

eastig left a comment

Choose a reason for hiding this comment

phohensee left a comment

Choose a reason for hiding this comment

openjdk bot commented Nov 2, 2022 • edited Loading

yftsai commented Nov 2, 2022

simonis commented Nov 2, 2022 • edited Loading

openjdk bot commented Nov 2, 2022

simonis left a comment

Choose a reason for hiding this comment

phohensee commented Nov 3, 2022

yftsai commented Nov 3, 2022 • edited Loading

eastig Nov 3, 2022

Choose a reason for hiding this comment

yftsai Nov 3, 2022

Choose a reason for hiding this comment

simonis left a comment

Choose a reason for hiding this comment

yftsai commented Nov 3, 2022

openjdk bot commented Nov 3, 2022

phohensee commented Nov 3, 2022

phohensee left a comment

Choose a reason for hiding this comment

openjdk bot commented Nov 3, 2022

openjdk bot commented Nov 3, 2022

yftsai commented Nov 2, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Nov 2, 2022 •

edited

Loading

openjdk bot commented Nov 2, 2022 •

edited

Loading

simonis commented Nov 2, 2022 •

edited

Loading

yftsai commented Nov 3, 2022 •

edited

Loading