8324646: Avoid Class.forName in SecureRandom constructor #17559

olivergillespie · 2024-01-24T15:25:28Z

Avoid expensive Class.forName call when constructing Providers such as SecureRandom which take constructor parameters. This can easily be cached in EngineDescription (this cache already existed before, it was removed in JDK-8280970 as unused, I'm bringing it back unchanged to support this new usage).

Benchmark results on my Linux x86 host show around a 20% reduction in time to create a new SecureRandom instance. Most of the remaining overhead is due to a failing constructor lookup - see JDK-8324648.

Before
newSecureRandom  avgt  2930 ± 50  ns/op

After
newSecureRandom  avgt  2400 ± 33  ns/op

I have seen multiple real-world applications which call new SecureRandom() on the hot path, so I believe efficiency here is important.

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-8324646: Avoid Class.forName in SecureRandom constructor (Enhancement - P4)

Reviewers

Aleksey Shipilev (@shipilev - Reviewer) ⚠️ Review applies to 45da78f9
Weijun Wang (@wangweij - Reviewer) ⚠️ Review applies to 45da78f9

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 17559

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-01-24T15:26:22Z

👋 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.

openjdk · 2024-01-24T15:27:38Z

@olivergillespie 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 · 2024-01-24T15:31:40Z

Webrevs

shipilev

This looks promising to me! But we need to polish this a bit:

src/java.base/share/classes/java/security/Provider.java

shipilev · 2024-01-24T17:45:06Z

src/java.base/share/classes/java/security/Provider.java

@@ -1557,12 +1557,22 @@ private static class EngineDescription {
        final String name;
        final boolean supportsParameter;
        final String constructorParameterClassName;
+        private volatile Class<?> constructorParameterClass;


Style: no need for private here, match what other fields are doing.

I don't disagree in principle but it was like this before the revert, and is still like this in 17.

Is volatile really needed? And there is some performance penalty and in practice the value will be the same even if recomputed.

Same thinking as above - this is how it was before, and how it is in 17. I'd rather not diverge, unless the reason is strong.

shipilev · 2024-01-24T17:45:43Z

src/java.base/share/classes/java/security/Provider.java

                    ctrParamClz = cap.constructorParameterClassName == null?
-                        null : Class.forName(cap.constructorParameterClassName);
+                        null : cap.getConstructorParameterClass();


Maybe we should move the ternary for cap.constructorParameterClassName == null into new method to begin with.

Thanks. Similar reasoning as above. If we decide to diverge from the original implementation then I'll update this too.

test/micro/org/openjdk/bench/java/security/SecureRandomBench.java

shipilev

Looks good to me, given our target is to backport this to other releases which have a similar code shape. But someone who is more familiar with this code should take a look, maybe @valeriepeng?

/reviewers 2

openjdk · 2024-01-29T11:18:08Z

@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

Reviewed-by: shade, weijun

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

52d4976: 8325437: Safepoint polling in monitor deflation can cause massive logs
8b70b8d: 8325440: Confusing error reported for octal literals with wrong digits
5daf622: 8325309: Amend "Listeners and Threads" in AWTThreadIssues.html
d165d12: 8325510: Serial: Remove redundant arg in non_clean_card_iterate
71b46c3: 8325471: CHeapBitMap(MEMFLAGS flags) constructor misleading use of super-constructor
e3dc6a7: 8314275: Incorrect stepping in switch
cc276ff: 8325516: Shenandoah: Move heap change tracking into ShenandoahHeap
8d9ad97: 8324641: [IR Framework] Add Setup method to provide custom arguments and set fields
b797652: 8322927: Unused code in LIR_Assembler::verify_oop_map
9936aee: 8324824: AArch64: Detect Ampere-1B core and update default options for Ampere CPUs
... and 182 more: https://git.openjdk.org/jdk/compare/bccd823c8e40863bed70ff5b24772843203871a5...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, @wangweij) 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 · 2024-01-29T11:18:13Z

@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).

wangweij · 2024-01-31T17:23:42Z

How about just using class literals? There is no need to call Class.forName, at least not now since they are all inside java.base.

olivergillespie · 2024-01-31T17:35:18Z

How about just using class literals? There is no need to call Class.forName, at least not now since they are all inside java.base.

Thanks :). That seems sensible if writing from scratch, but that part I'm just reviving from JDK-8280970 which is nice for keeping it in sync with 17 for backports. My preference is to keep it close to the original code/17 unless there's a strong reason not to.

valeriepeng · 2024-01-31T17:42:02Z

I second Max's suggestion of just using class literals. It's cleaner this way. I'd think it'd also be faster?

olivergillespie · 2024-01-31T17:49:23Z

The lookup only happens once per entry so I don't think performance is a big deal either way. What do you both think about my argument for re-using the code that stood for years and still stands in 17?

wangweij · 2024-01-31T18:05:49Z

You are converting from string to class anyway, and since this is only implementation detail inside one file, we can even backport it if it really helps.

valeriepeng · 2024-01-31T18:56:49Z

I still prefer class literals as it's cleaner and more straight forward than the pre-existing approach. Why having two fields when one is enough. In most if not all aspects, class literals seems a better solution... Backport should be doable as the scope of change is limited.

olivergillespie · 2024-02-01T10:36:00Z

Updated with class literals, thanks

shipilev

I like it even more with class literals.

wangweij

LGTM.

BTW, is there any change in benchmark scores?

olivergillespie · 2024-02-06T13:39:52Z

Thank you :) No change in benchmark score, or perhaps slight improvement. There seem to be two modes, one at 2400ns/op (same as original change) and one around 2200ns/op.

# Fork: 1 of 3
# Warmup Iteration   1: 3552.303 ns/op
# Warmup Iteration   2: 2906.823 ns/op
# Warmup Iteration   3: 2404.065 ns/op
# Warmup Iteration   4: 2396.532 ns/op
# Warmup Iteration   5: 2416.137 ns/op
Iteration   1: 2418.132 ns/op
Iteration   2: 2410.748 ns/op
Iteration   3: 2396.958 ns/op
Iteration   4: 2410.428 ns/op
Iteration   5: 2403.168 ns/op

# Fork: 2 of 3
# Warmup Iteration   1: 3156.931 ns/op
# Warmup Iteration   2: 2606.873 ns/op
# Warmup Iteration   3: 2175.693 ns/op
# Warmup Iteration   4: 2250.570 ns/op
# Warmup Iteration   5: 2152.489 ns/op
Iteration   1: 2190.243 ns/op
Iteration   2: 2153.819 ns/op
Iteration   3: 2175.183 ns/op
Iteration   4: 2179.866 ns/op
Iteration   5: 2150.761 ns/op


# Fork: 3 of 3
# Warmup Iteration   1: 3489.428 ns/op
# Warmup Iteration   2: 2851.548 ns/op
# Warmup Iteration   3: 2410.677 ns/op
# Warmup Iteration   4: 2427.988 ns/op
# Warmup Iteration   5: 2414.204 ns/op
Iteration   1: 2412.798 ns/op
Iteration   2: 2417.093 ns/op
Iteration   3: 2414.075 ns/op
Iteration   4: 2442.164 ns/op
Iteration   5: 2410.162 ns/op

olivergillespie · 2024-02-09T14:20:20Z

/integrate

Assuming the approvals still hold after the trivial copyright change, I think this is ready to go. Thanks all.

openjdk · 2024-02-09T14:20:59Z

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

wangweij · 2024-02-09T14:37:12Z

/sponsor

openjdk · 2024-02-09T14:38:11Z

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

69b2674: 8324648: Avoid NoSuchMethodError when instantiating NativePRNG
52d4976: 8325437: Safepoint polling in monitor deflation can cause massive logs
8b70b8d: 8325440: Confusing error reported for octal literals with wrong digits
5daf622: 8325309: Amend "Listeners and Threads" in AWTThreadIssues.html
d165d12: 8325510: Serial: Remove redundant arg in non_clean_card_iterate
71b46c3: 8325471: CHeapBitMap(MEMFLAGS flags) constructor misleading use of super-constructor
e3dc6a7: 8314275: Incorrect stepping in switch
cc276ff: 8325516: Shenandoah: Move heap change tracking into ShenandoahHeap
8d9ad97: 8324641: [IR Framework] Add Setup method to provide custom arguments and set fields
b797652: 8322927: Unused code in LIR_Assembler::verify_oop_map
... and 183 more: https://git.openjdk.org/jdk/compare/bccd823c8e40863bed70ff5b24772843203871a5...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-02-09T14:38:21Z

@wangweij @olivergillespie Pushed as commit 8ef918d.

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

olivergillespie · 2024-02-13T11:19:19Z

/backport jdk17u

openjdk · 2024-02-13T11:20:46Z

@olivergillespie Could not automatically backport 8ef918d6 to openjdk/jdk17u due to conflicts in the following files:

src/java.base/share/classes/java/security/Provider.java

Please fetch the appropriate branch/commit and manually resolve these conflicts by using the following commands in your personal fork of openjdk/jdk17u. Note: these commands are just some suggestions and you can use other equivalent commands you know.

# Fetch the up-to-date version of the target branch
$ git fetch --no-tags https://git.openjdk.org/jdk17u.git master:master

# Check out the target branch and create your own branch to backport
$ git checkout master
$ git checkout -b backport-olivergillespie-8ef918d6

# Fetch the commit you want to backport
$ git fetch --no-tags https://git.openjdk.org/jdk.git 8ef918d6678437a5b351b172bb4cf144eeaa975f

# Backport the commit
$ git cherry-pick --no-commit 8ef918d6678437a5b351b172bb4cf144eeaa975f
# Resolve conflicts now

# Commit the files you have modified
$ git add files/with/resolved/conflicts
$ git commit -m 'Backport 8ef918d6678437a5b351b172bb4cf144eeaa975f'

Once you have resolved the conflicts as explained above continue with creating a pull request towards the openjdk/jdk17u with the title Backport 8ef918d6678437a5b351b172bb4cf144eeaa975f.

Below you can find a suggestion for the pull request body:

Hi all,

This pull request contains a backport of commit 8ef918d6 from the openjdk/jdk repository.

The commit being backported was authored by Oli Gillespie on 9 Feb 2024 and was reviewed by Aleksey Shipilev and Weijun Wang.

Thanks!

olivergillespie · 2024-02-13T11:27:09Z

/backport jdk21u-dev

openjdk · 2024-02-13T11:28:34Z

@olivergillespie the backport was successfully created on the branch backport-olivergillespie-8ef918d6 in my personal fork of openjdk/jdk21u-dev. To create a pull request with this backport targeting openjdk/jdk21u-dev:master, just click the following link:

➡️ Create pull request

The title of the pull request is automatically filled in correctly and below you find a suggestion for the pull request body:

Hi all,

This pull request contains a backport of commit 8ef918d6 from the openjdk/jdk repository.

The commit being backported was authored by Oli Gillespie on 9 Feb 2024 and was reviewed by Aleksey Shipilev and Weijun Wang.

Thanks!

If you need to update the source branch of the pull then run the following commands in a local clone of your personal fork of openjdk/jdk21u-dev:

$ git fetch https://github.com/openjdk-bots/jdk21u-dev.git backport-olivergillespie-8ef918d6:backport-olivergillespie-8ef918d6
$ git checkout backport-olivergillespie-8ef918d6
# make changes
$ git add paths/to/changed/files
$ git commit --message 'Describe additional changes made'
$ git push https://github.com/openjdk-bots/jdk21u-dev.git backport-olivergillespie-8ef918d6

olivergillespie · 2024-03-12T11:45:43Z

/backport jdk22u-dev

openjdk · 2024-03-12T11:46:07Z

@olivergillespie The target repository jdk22u-dev is not a valid target for backports.
List of valid target repositories: openjdk/jdk, openjdk/jdk11u, openjdk/jdk11u-dev, openjdk/jdk17u, openjdk/jdk17u-dev, openjdk/jdk21u, openjdk/jdk21u-dev, openjdk/jdk22, openjdk/jdk22u, openjdk/jdk7u, openjdk/jdk8u, openjdk/jdk8u-dev, openjdk/jfx, openjdk/jfx17u, openjdk/jfx21u, openjdk/jfx22u, openjdk/lilliput-jdk17u, openjdk/lilliput-jdk21u, openjdk/shenandoah-jdk21u, openjdk/shenandoah-jdk8u.
Supplying the organization/group prefix is optional.

olivergillespie · 2024-03-12T11:50:52Z

/backport jdk22u

openjdk · 2024-03-12T11:52:00Z

@olivergillespie the backport was successfully created on the branch backport-olivergillespie-8ef918d6 in my personal fork of openjdk/jdk22u. To create a pull request with this backport targeting openjdk/jdk22u:master, just click the following link:

➡️ Create pull request

The title of the pull request is automatically filled in correctly and below you find a suggestion for the pull request body:

Hi all,

This pull request contains a backport of commit 8ef918d6 from the openjdk/jdk repository.

The commit being backported was authored by Oli Gillespie on 9 Feb 2024 and was reviewed by Aleksey Shipilev and Weijun Wang.

Thanks!

If you need to update the source branch of the pull then run the following commands in a local clone of your personal fork of openjdk/jdk22u:

$ git fetch https://github.com/openjdk-bots/jdk22u.git backport-olivergillespie-8ef918d6:backport-olivergillespie-8ef918d6
$ git checkout backport-olivergillespie-8ef918d6
# make changes
$ git add paths/to/changed/files
$ git commit --message 'Describe additional changes made'
$ git push https://github.com/openjdk-bots/jdk22u.git backport-olivergillespie-8ef918d6

⚠️ @olivergillespie You are not yet a collaborator in my fork openjdk-bots/jdk22u. An invite will be sent out and you need to accept it before you can proceed.

8324646 - Avoid Class.forName in SecureRandom constructor

93f0cc7

openjdk bot added the rfr Pull request is ready for review label Jan 24, 2024

openjdk bot added the security security-dev@openjdk.org label Jan 24, 2024

olivergillespie mentioned this pull request Jan 24, 2024

8324648: Avoid NoSuchMethodError when instantiating NativePRNG #17560

Closed

3 tasks

shipilev reviewed Jan 24, 2024

View reviewed changes

olivergillespie added 2 commits January 24, 2024 18:00

Add copyright header to new file

5818e0b

Rename newSecureRandom -> create

8d8f2d0

shipilev approved these changes Jan 29, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jan 29, 2024

openjdk bot removed the ready Pull request is ready to be integrated label Jan 29, 2024

Use class literals instead of forName

45da78f

shipilev approved these changes Feb 6, 2024

View reviewed changes

wangweij approved these changes Feb 6, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Feb 6, 2024

Update copyright year

719d321

openjdk bot added the sponsor Pull request is ready to be sponsored label Feb 9, 2024

openjdk bot added the integrated Pull request has been integrated label Feb 9, 2024

openjdk bot closed this Feb 9, 2024

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 Feb 9, 2024

olivergillespie deleted the 8324646 branch February 13, 2024 11:19

olivergillespie mentioned this pull request Mar 19, 2024

8324648: Avoid NoSuchMethodError when instantiating NativePRNG openjdk/jdk17u-dev#2308

Closed

3 tasks

8324646: Avoid Class.forName in SecureRandom constructor #17559

8324646: Avoid Class.forName in SecureRandom constructor #17559

Conversation

olivergillespie commented Jan 24, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Jan 24, 2024

openjdk bot commented Jan 24, 2024

mlbridge bot commented Jan 24, 2024 • edited Loading

Webrevs

shipilev left a comment

Choose a reason for hiding this comment

shipilev Jan 24, 2024

Choose a reason for hiding this comment

olivergillespie Jan 24, 2024

Choose a reason for hiding this comment

RogerRiggs Jan 24, 2024

Choose a reason for hiding this comment

olivergillespie Jan 25, 2024

Choose a reason for hiding this comment

shipilev Jan 24, 2024

Choose a reason for hiding this comment

olivergillespie Jan 24, 2024

Choose a reason for hiding this comment

shipilev left a comment

Choose a reason for hiding this comment

openjdk bot commented Jan 29, 2024 • edited Loading

openjdk bot commented Jan 29, 2024

wangweij commented Jan 31, 2024

olivergillespie commented Jan 31, 2024

valeriepeng commented Jan 31, 2024 • edited Loading

olivergillespie commented Jan 31, 2024

wangweij commented Jan 31, 2024

valeriepeng commented Jan 31, 2024 • edited Loading

olivergillespie commented Feb 1, 2024

shipilev left a comment

Choose a reason for hiding this comment

wangweij left a comment

Choose a reason for hiding this comment

olivergillespie commented Feb 6, 2024

olivergillespie commented Feb 9, 2024

openjdk bot commented Feb 9, 2024

wangweij commented Feb 9, 2024

openjdk bot commented Feb 9, 2024

openjdk bot commented Feb 9, 2024

olivergillespie commented Feb 13, 2024

openjdk bot commented Feb 13, 2024

olivergillespie commented Feb 13, 2024

openjdk bot commented Feb 13, 2024

olivergillespie commented Mar 12, 2024

openjdk bot commented Mar 12, 2024

olivergillespie commented Mar 12, 2024

openjdk bot commented Mar 12, 2024

olivergillespie commented Jan 24, 2024 •

edited by openjdk bot

Loading

mlbridge bot commented Jan 24, 2024 •

edited

Loading

openjdk bot commented Jan 29, 2024 •

edited

Loading

valeriepeng commented Jan 31, 2024 •

edited

Loading

valeriepeng commented Jan 31, 2024 •

edited

Loading