8316415: Parallelize sun/security/rsa/SignedObjectChain.java subtests #15860

msobiers · 2023-09-21T12:44:36Z

sun/security/rsa/SignedObjectChain.java is very slow when run with C1, I suspect because some crypto intrinsics are only implemented in C2. Commit contains changes made to parallelize it.

Comparison of before and after parallelization:
time make test TEST=jdk/sun/security/rsa/SignedObjectChain.java TEST_VM_OPTS="-XX:+UseParallelGC -XX:TieredStopAtLevel=1"
before: 270.72s user 4.88s system 108% cpu 4:14.43 total
after: 410.76s user 7.50s system 555% cpu 1:15.23 total
after second commit: 375.46s user 4.59s system 539% cpu 1:10.41 total

time make test TEST=jdk/sun/security/rsa/SignedObjectChain.java TEST_VM_OPTS="-XX:+UseParallelGC"
before: 63.67s user 4.67s system 161% cpu 42.424 total
after: 130.36s user 7.47s system 585% cpu 23.526 total
after second commit: 67.31s user 4.48s system 417% cpu 17.183 total

time make test TEST=jdk/sun/security/rsa/SignedObjectChain.java TEST_VM_OPTS="-XX:+UseShenandoahGC -XX:TieredStopAtLevel=1"
before: 281.99s user 5.54s system 108% cpu 4:24.09 total
after: 386.98s user 8.62s system 496% cpu 1:19.73 total
after second commit: 413.51s user 5.08s system 613% cpu 1:08.25 total

time make test TEST=jdk/sun/security/rsa/SignedObjectChain.java TEST_VM_OPTS="-XX:+UseShenandoahGC"
before: 65.86s user 5.05s system 156% cpu 45.215 total
after: 135.90s user 7.66s system 585% cpu 24.502 total
after second commit: 83.25s user 4.82s system 469% cpu 18.741 total

Progress

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

Issue

JDK-8316415: Parallelize sun/security/rsa/SignedObjectChain.java subtests (Enhancement - P4)

Reviewers

Aleksey Shipilev (@shipilev - Reviewer)
Rajan Halade (@rhalade - Reviewer) ⚠️ Review applies to 0040ba31
Valerie Peng (@valeriepeng - Reviewer) ⚠️ Review applies to 0040ba31

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 15860

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

Using diff file

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

Webrev

Link to Webrev Comment

sun/security/rsa/SignedObjectChain.java is very slow when run with C1, I suspect because some crypto intrinsics are only implemented in C2. Commit contains changes made to parallelize sun/security/rsa/SignedObjectChain.java Comparison of before and after parallelization: time make test TEST=jdk/sun/security/rsa/SignedObjectChain.java TEST_VM_OPTS="-XX:+UseParallelGC -XX:TieredStopAtLevel=1" before: 270.72s user 4.88s system 108% cpu 4:14.43 total after: 410.76s user 7.50s system 555% cpu 1:15.23 total time make test TEST=jdk/sun/security/rsa/SignedObjectChain.java TEST_VM_OPTS="-XX:+UseParallelGC" before: 63.67s user 4.67s system 161% cpu 42.424 total after: 130.36s user 7.47s system 585% cpu 23.526 total time make test TEST=jdk/sun/security/rsa/SignedObjectChain.java TEST_VM_OPTS="-XX:+UseShenandoahGC -XX:TieredStopAtLevel=1" before: 281.99s user 5.54s system 108% cpu 4:24.09 total after: 386.98s user 8.62s system 496% cpu 1:19.73 total time make test TEST=jdk/sun/security/rsa/SignedObjectChain.java TEST_VM_OPTS="-XX:+UseShenandoahGC" before: 65.86s user 5.05s system 156% cpu 45.215 total after: 135.90s user 7.66s system 585% cpu 24.502 total

msobiers · 2023-09-21T12:45:15Z

/covered

bridgekeeper · 2023-09-21T12:45:42Z

👋 Welcome back msobiers! 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.

bridgekeeper · 2023-09-21T12:45:45Z

You are already a known contributor!

openjdk · 2023-09-21T12:49:05Z

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

security

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 · 2023-09-21T12:53:10Z

Webrevs

time make test TEST=jdk/sun/security/rsa/SignedObjectChain.java TEST_VM_OPTS="-XX:+UseParallelGC -XX:TieredStopAtLevel=1" before: 270.72s user 4.88s system 108% cpu 4:14.43 total after: 375.46s user 4.59s system 539% cpu 1:10.41 total time make test TEST=jdk/sun/security/rsa/SignedObjectChain.java TEST_VM_OPTS="-XX:+UseParallelGC" before: 63.67s user 4.67s system 161% cpu 42.424 total after: 67.31s user 4.48s system 417% cpu 17.183 total time make test TEST=jdk/sun/security/rsa/SignedObjectChain.java TEST_VM_OPTS="-XX:+UseShenandoahGC -XX:TieredStopAtLevel=1" before: 281.99s user 5.54s system 108% cpu 4:24.09 total after: 413.51s user 5.08s system 613% cpu 1:08.25 total time make test TEST=jdk/sun/security/rsa/SignedObjectChain.java TEST_VM_OPTS="-XX:+UseShenandoahGC" before: 65.86s user 5.05s system 156% cpu 45.215 total after: 83.25s user 4.82s system 469% cpu 18.741 total

shipilev

Looks nice. A few stylistic comments:

test/jdk/sun/security/rsa/SignedObjectChain.java

shipilev · 2023-09-21T13:36:52Z

test/jdk/sun/security/rsa/SignedObjectChain.java

            System.out.println("All tests passed");
        } else {
            throw new RuntimeException("Some tests failed");
        }
    }
 }
+


Do we need a newline here?

+1, would be nice if this extra newline is removed.

shipilev · 2023-09-21T13:37:19Z

/reviewers 2

openjdk · 2023-09-21T13:37:47Z

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

8316415: Parallelize sun/security/rsa/SignedObjectChain.java subtests

Reviewed-by: shade, rhalade, valeriep

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

eeb63cd: 8316361: C2: assert(!failure) failed: Missed optimization opportunity in PhaseIterGVN with -XX:VerifyIterativeGVN=10
6948942: 8317327: Remove JT_JAVA dead code in jib-profiles.js
795e5dc: 8315503: G1: Code root scan causes long GC pauses due to imbalanced iteration
207819a: 8315604: IGV: dump and visualize node bottom and phase types
8fcf70e: 5066247: Refine the spec of equals() and hashCode() for j.text.Format classes
93f662f: 8317335: Build on windows fails after 8316645
b8fa6c2: 8316186: RISC-V: Remove PlatformCmpxchg<4>
fb055e7: 8316645: RISC-V: Remove dependency on libatomic by adding cmpxchg 1b
009f5e1: 8317141: Remove unused validIndex method from URLClassPath$JarLoader
47569a2: 8295919: java.security.MessageDigest.isEqual does not adhere to @implNote
... and 190 more: https://git.openjdk.org/jdk/compare/3828dc913a3ea28d622b69bd07f26949128eb5f7...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 (@shipilev, @rhalade, @valeriepeng) 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 · 2023-09-21T13:38:05Z

@shipilev
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

shipilev · 2023-09-21T14:05:26Z

Shows a very good improvement here as well:

% time CONF=linux-x86_64-server-fastdebug make test TEST=sun/security/rsa/SignedObjectChain.java TEST_VM_OPTS="-XX:TieredStopAtLevel=1"

Baseline:
CONF=linux-x86_64-server-fastdebug make test    333.34s user 5.04s system 111% cpu 5:02.33 total
CONF=linux-x86_64-server-fastdebug make test    292.03s user 5.27s system 114% cpu 4:20.71 total
CONF=linux-x86_64-server-fastdebug make test    330.51s user 5.23s system 111% cpu 5:00.18 total

Patched:
CONF=linux-x86_64-server-fastdebug make test    309.72s user 4.92s system 505% cpu 1:02.25 total
CONF=linux-x86_64-server-fastdebug make test    275.63s user 5.02s system 540% cpu   51.82 total
CONF=linux-x86_64-server-fastdebug make test    299.18s user 5.06s system 546% cpu   55.64 total

shipilev · 2023-09-25T17:52:54Z

@valeriepeng, would you like to take a look?

shipilev · 2023-09-29T07:41:08Z

Or maybe @rhalade?

valeriepeng · 2023-09-29T17:44:39Z

I will take a look, sorry has been busy the past few days...

valeriepeng · 2023-09-29T17:50:45Z

Looks good to me as well. Would be nice to apply the stylistic changes suggested by @shipilev before integrating. Thanks!

Co-authored-by: Aleksey Shipilëv <shipilev@amazon.de>

msobiers

Removing unnecessary line

msobiers · 2023-10-02T09:52:20Z

/integrate

openjdk · 2023-10-02T09:54:13Z

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

shipilev · 2023-10-02T09:55:10Z

/sponsor

openjdk · 2023-10-02T09:56:17Z

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

eeb63cd: 8316361: C2: assert(!failure) failed: Missed optimization opportunity in PhaseIterGVN with -XX:VerifyIterativeGVN=10
6948942: 8317327: Remove JT_JAVA dead code in jib-profiles.js
795e5dc: 8315503: G1: Code root scan causes long GC pauses due to imbalanced iteration
207819a: 8315604: IGV: dump and visualize node bottom and phase types
8fcf70e: 5066247: Refine the spec of equals() and hashCode() for j.text.Format classes
93f662f: 8317335: Build on windows fails after 8316645
b8fa6c2: 8316186: RISC-V: Remove PlatformCmpxchg<4>
fb055e7: 8316645: RISC-V: Remove dependency on libatomic by adding cmpxchg 1b
009f5e1: 8317141: Remove unused validIndex method from URLClassPath$JarLoader
47569a2: 8295919: java.security.MessageDigest.isEqual does not adhere to @implNote
... and 190 more: https://git.openjdk.org/jdk/compare/3828dc913a3ea28d622b69bd07f26949128eb5f7...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-10-02T09:56:25Z

@shipilev @msobiers Pushed as commit 5984792.

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

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

openjdk bot added the security security-dev@openjdk.org label Sep 21, 2023

shipilev approved these changes Sep 21, 2023

View reviewed changes

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

openjdk bot removed the ready Pull request is ready to be integrated label Sep 21, 2023

rhalade approved these changes Sep 29, 2023

View reviewed changes

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

valeriepeng approved these changes Sep 29, 2023

View reviewed changes

msobiers and others added 2 commits October 2, 2023 10:38

Update test/jdk/sun/security/rsa/SignedObjectChain.java

e39c326

Co-authored-by: Aleksey Shipilëv <shipilev@amazon.de>

Remove additional line

b3c885f

msobiers commented Oct 2, 2023

View reviewed changes

shipilev approved these changes Oct 2, 2023

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Oct 2, 2023

openjdk bot added the integrated Pull request has been integrated label Oct 2, 2023

openjdk bot closed this Oct 2, 2023

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 Oct 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8316415: Parallelize sun/security/rsa/SignedObjectChain.java subtests #15860

8316415: Parallelize sun/security/rsa/SignedObjectChain.java subtests #15860

msobiers commented Sep 21, 2023 •

edited by openjdk bot

Loading

msobiers commented Sep 21, 2023

bridgekeeper bot commented Sep 21, 2023

bridgekeeper bot commented Sep 21, 2023

openjdk bot commented Sep 21, 2023

mlbridge bot commented Sep 21, 2023 •

edited

Loading

shipilev left a comment

shipilev Sep 21, 2023

valeriepeng Sep 29, 2023

shipilev commented Sep 21, 2023

openjdk bot commented Sep 21, 2023 •

edited

Loading

openjdk bot commented Sep 21, 2023

shipilev commented Sep 21, 2023

shipilev commented Sep 25, 2023

shipilev commented Sep 29, 2023

valeriepeng commented Sep 29, 2023

valeriepeng commented Sep 29, 2023

msobiers left a comment

msobiers commented Oct 2, 2023

openjdk bot commented Oct 2, 2023

shipilev commented Oct 2, 2023

openjdk bot commented Oct 2, 2023

openjdk bot commented Oct 2, 2023

8316415: Parallelize sun/security/rsa/SignedObjectChain.java subtests #15860

8316415: Parallelize sun/security/rsa/SignedObjectChain.java subtests #15860

Conversation

msobiers commented Sep 21, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

msobiers commented Sep 21, 2023

bridgekeeper bot commented Sep 21, 2023

bridgekeeper bot commented Sep 21, 2023

openjdk bot commented Sep 21, 2023

mlbridge bot commented Sep 21, 2023 • edited Loading

Webrevs

shipilev left a comment

Choose a reason for hiding this comment

shipilev Sep 21, 2023

Choose a reason for hiding this comment

valeriepeng Sep 29, 2023

Choose a reason for hiding this comment

shipilev commented Sep 21, 2023

openjdk bot commented Sep 21, 2023 • edited Loading

openjdk bot commented Sep 21, 2023

shipilev commented Sep 21, 2023

shipilev commented Sep 25, 2023

shipilev commented Sep 29, 2023

valeriepeng commented Sep 29, 2023

valeriepeng commented Sep 29, 2023

msobiers left a comment

Choose a reason for hiding this comment

msobiers commented Oct 2, 2023

openjdk bot commented Oct 2, 2023

shipilev commented Oct 2, 2023

openjdk bot commented Oct 2, 2023

openjdk bot commented Oct 2, 2023

msobiers commented Sep 21, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Sep 21, 2023 •

edited

Loading

openjdk bot commented Sep 21, 2023 •

edited

Loading