8324646: Avoid Class.forName in SecureRandom constructor

[olivergillespie]

Improve performance of SecureRandom constructor by backporting (both clean) two changes:

1. JDK-8280970 removes some unused code in Provider.java. This is not really functionally needed, but it changes the same areas of code and makes the actual performance fix apply cleanly.
2. JDK-8324646 is the actual performance fix - avoiding Class.forName calls in every construction.

Note - because of the unused code still present in JDK17, there is actually an alternative fix which makes use of it, which is very simple:

diff --git a/src/java.base/share/classes/java/security/Provider.java b/src/java.base/share/classes/java/security/Provider.java
index af8ebeeda57..28bf642d0c8 100644
--- a/src/java.base/share/classes/java/security/Provider.java
+++ b/src/java.base/share/classes/java/security/Provider.java
@@ -1851,7 +1851,7 @@ public abstract class Provider extends Properties {
                         null : constructorParameter.getClass();
                 } else {
                     ctrParamClz = cap.constructorParameterClassName == null?
-                        null : Class.forName(cap.constructorParameterClassName);
+                        null : cap.getConstructorParameterClass(); // actually make use of the cached class!
                     if (constructorParameter != null) {
                         if (ctrParamClz == null) {
                             throw new InvalidParameterException

This has the same performance benefits as making the two backports. But, it means 17 will be diverged from later versions, and I think the backported fix is cleaner overall.

Benchmark results (make test TEST=micro:org.openjdk.bench.java.security.SecureRandomBench):

Before: 2614 ± 127  ns/op
 After: 2150 ± 116  ns/op

---

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
• JDK-8324646 needs maintainer approval
• JDK-8280970 needs maintainer approval

Issues

• JDK-8324646: Avoid Class.forName in SecureRandom constructor (Enhancement - P4 - Approved)
• JDK-8280970: Cleanup dead code in java.security.Provider (Enhancement - P5 - Approved)

Reviewers

• Paul Hohensee (@phohensee - Reviewer)
• Aleksey Shipilev (@shipilev - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 2310

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

Using diff file

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

Webrev

Link to Webrev Comment

[bridgekeeper]

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

[olivergillespie]

/issue JDK-8324646

[olivergillespie]

/issue JDK-8280970

[openjdk]

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

8324646: Avoid Class.forName in SecureRandom constructor
8280970: Cleanup dead code in java.security.Provider

Reviewed-by: phh, shade

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.

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, @shipilev) 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]

@olivergillespie This issue is referenced in the PR title - it will now be updated.

[openjdk]

@olivergillespie
Adding additional issue to issue list: 8280970: Cleanup dead code in java.security.Provider.

[openjdk]

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

[mlbridge]

Webrevs

• 02: Full - Incremental (0001824a)
• 01: Full - Incremental (280a1fcc)
• 00: Full (5b15fdb2)

[openjdk]

⚠️ @olivergillespie This change is now ready for you to apply for maintainer approval. This can be done directly in each associated issue or by using the /approval command.

[bridgekeeper]

@olivergillespie 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!

[olivergillespie]

Waiting for next release to ask approval, since this is similar in nature to https://bugs.openjdk.org/browse/JDK-8324648.

[bridgekeeper]

@olivergillespie 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!

[olivergillespie]

/approval 8280970 request Trivial dead code cleanup as a pre-requisite for JDK-8324646. Applies cleanly, no/low risk since it's dead code.

/approval 8324646 request Performance improvement in SecureRandom.<init>, which I have seen on the hot path in real applications. Clean backport, low risk in my opinion (simple change to skip Class.forName by using the class literal directly). It has been in tip for several months. Confirmed the benchmark results match tip.

[openjdk]

@olivergillespie
8280970: The approval request has been created successfully.

[openjdk]

@olivergillespie
8324646: The approval request has been created successfully.

[RealCLanger]

@olivergillespie just glanced over this - I think I would have preferred separate backports of both issues because the cleanup seems independent of the other change. But I'm going to approve this nevertheless😄

[olivergillespie]

Thanks, I wasn't sure which would be easier to review overall, but noted for next time!

[olivergillespie]

/integrate

[openjdk]

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

[shipilev]

/sponsor

[openjdk]

Going to push as commit 1ce0c63.

[openjdk]

@shipilev @olivergillespie Pushed as commit 1ce0c63.

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