8324648: Avoid NoSuchMethodError when instantiating NativePRNG #17560

olivergillespie · 2024-01-24T15:42:05Z

A typical call to new SecureRandom() is slowed down by looking for a constructor in NativePRNG which takes java.security.SecureRandomParameters. NativePRNG does not have such a constructor, so the search fails here, incurring all the cost of the lookup and creating a subsequent exception.

Creating a dummy constructor which takes and ignores this parameter will speed up new SecureRandom() calls significantly.

The benchmark from #17559 shows around 80% reduction in time taken to create a new SecureRandom with NativePRNG (default on my machine).

Before
SecureRandomBench.newSecureRandom  avgt  2930 ± 50 ns/op

After
SecureRandomBench.newSecureRandom  avgt  510 ± 16 ns/op

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-8324648: Avoid NoSuchMethodError when instantiating NativePRNG (Enhancement - P4)

Reviewers

Aleksey Shipilev (@shipilev - Reviewer) ⚠️ Review applies to d537b80c
Weijun Wang (@wangweij - Reviewer) ⚠️ Review applies to de4d83ac
Chen Liang (@liach - Author) ⚠️ Review applies to 44958eff
Valerie Peng (@valeriepeng - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 17560

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-01-24T15:43:11Z

👋 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:43:42Z

@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:46:47Z

Webrevs

liach · 2024-01-24T19:50:55Z

I see that the existing caller code has a check for ctorParamClass. Why must we declare SecureRandomParameters.class as the ctorParamClass for these 3 randoms instead of using null?

olivergillespie · 2024-01-25T12:10:56Z

I see that the existing caller code has a check for ctorParamClass. Why must we declare SecureRandomParameters.class as the ctorParamClass for these 3 randoms instead of using null?

From lookup code:

// Looking for the constructor with a params first and fallback
// to one without if not found. This is to support the enhanced
// SecureRandom where both styles of constructors are supported.
// Before jdk9, there was no params support (only getInstance(alg))
// and an impl only had the params-less constructor. Since jdk9,
// there is getInstance(alg,params) and an impl can contain
// an Impl(params) constructor.

The EngineDescription is looked up by type which in this case is SecureRandom, there's no way right now to choose specific parameters for different implementers. Are you proposing that I extend the engine lookup to include algorithm, so we can have specific EngineDescription for SecureRandom->NativePRNG? I don't know this code well enough to know if that's feasible, but it definitely seems more complex.

shipilev

Seems like a good workaround. Attn @valeriepeng ;)

/reviewers 2

openjdk · 2024-01-29T11:19:38Z

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

8324648: Avoid NoSuchMethodError when instantiating NativePRNG

Reviewed-by: shade, weijun, liach, 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 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, @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 · 2024-01-29T11:19:43Z

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

valeriepeng · 2024-01-30T23:44:39Z

For NativePRNG, should the dummy constructor just ignores the parameters? If passed with a non-null parameter, should it accept it? Max and Brad is more familiar with SecureRandom than me, so it's better that they chime in on this. @bradfordwetmore @wangweij

wangweij · 2024-01-31T17:21:56Z

I think Valerie is correct. Otherwise, SecureRandom.getInstance("NativePRNG", DrbgParameters.instantiation(...)) will return something that it actually does not support.

olivergillespie · 2024-01-31T17:46:20Z

Good point, thanks, I should have thought of that. I see DBRG has this for when the params are not appropriate:

throw new IllegalArgumentException("Unsupported params: " + params.getClass());

I'll add something similar in my dummy constructors, unless someone objects.

olivergillespie · 2024-02-01T11:22:55Z

The more I think about it, adding a new public constructor seems a bit ugly and potentially confusing.

So I'll offer a few more ideas. Which do you think is best?

New public constructor with IllegalArgumentException if params != null

// constructor, unused argument to speed up lookups from Provider
public NativePRNG(SecureRandomParameters params) {
    this();
    if (params != null) {
        throw new IllegalArgumentException("Unsupported params: " + params.getClass());
    }
}

Use getConstructors() instead of getConstructor(...), and manually look for the match (basically implement our own tryGetConstructor). This way, we avoid the exception which is the main cost of the current approach, but don't need to add any special cases. Benchmark says this is only a little slower than 1. (550ns vs 510ns).

// new style with params
for (Constructor<?> c : getImplClass().getConstructors()) {
    Class<?>[] args = c.getParameterTypes();
    if (args.length == 1 && args[0].equals(ctrParamClz)) {
        // found match
        return c.newInstance(ctorParamObj);
    }
}
// old style without params
if (ctorParamObj == null) {
    return newInstanceOf();
} else {
    throw new NoSuchMethodException("...");
}

Add explicit handling of NativePRNG inside Provider.

static HashSet<String> NO_ARG_SECURE_RANDOMS = new HashSet<>();
static {
    NO_ARG_SECURE_RANDOMS.add("sun.security.provider.NativePRNG");
    NO_ARG_SECURE_RANDOMS.add("sun.security.provider.NativePRNG$Blocking");
    NO_ARG_SECURE_RANDOMS.add("sun.security.provider.NativePRNG$NonBlocking");
};

private Object newInstanceUtil(Class<?> ctrParamClz, Object ctorParamObj)
        throws Exception {
    if (ctrParamClz == null) {
        return newInstanceOf();
    } else if (ctorParamObj == null && NO_ARG_SECURE_RANDOMS.contains(getImplClass().getName())) {
        return newInstanceOf();
    } else {

Add some kind of caching in Provider to only pay the cost once (not coded up yet, potentially a bit annoying to also save the NSME if we want to keep that behaviour).

wangweij · 2024-02-02T18:50:56Z

No matter if we touch the Provider class, I think we can remove the old constructors. With the params class defined for SecureRandom, those old constructors should be useless. And they step 3 above is not needed.

olivergillespie · 2024-02-06T10:38:36Z

Thanks. Updated with option 1 and removed old constructors.

wangweij

Looks good to me.

Please update the copyright year. Thanks.

valeriepeng · 2024-02-06T18:46:30Z

src/java.base/unix/classes/sun/security/provider/NativePRNG.java

@@ -203,11 +203,14 @@ static boolean isAvailable() {
    }

    // constructor, called by the JCA framework
-    public NativePRNG() {
+    public NativePRNG(SecureRandomParameters params) {
        super();


nit: maybe super() not needed? Same goes for the Blocking and NonBlocking ctors.

It was already there before, but yes it doesn't do anything useful. The other impls I've checked don't use it, so I'll remove.

olivergillespie · 2024-02-09T14:19:13Z

/integrate

Thanks for the reviews, all!

openjdk · 2024-02-09T14:19:32Z

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

wangweij · 2024-02-09T14:36:01Z

/sponsor

openjdk · 2024-02-09T14:37:00Z

Going to push as commit 69b2674.
Since your change was applied there have been 192 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

Your commit was automatically rebased without conflicts.

openjdk · 2024-02-09T14:37:06Z

@wangweij @olivergillespie Pushed as commit 69b2674.

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

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

/backport jdk17u

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

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

src/java.base/unix/classes/sun/security/provider/NativePRNG.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-69b2674c

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

# Backport the commit
$ git cherry-pick --no-commit 69b2674c6861fdb7d9f9cb39e07d50515c73e33a
# Resolve conflicts now

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

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

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

Hi all,

This pull request contains a backport of commit 69b2674c from the openjdk/jdk repository.

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

Thanks!

olivergillespie · 2024-02-13T11:24:54Z

/backport jdk21u-dev

openjdk · 2024-02-13T11:26:16Z

@olivergillespie the backport was successfully created on the branch backport-olivergillespie-69b2674c 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 69b2674c from the openjdk/jdk repository.

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

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-69b2674c:backport-olivergillespie-69b2674c
$ git checkout backport-olivergillespie-69b2674c
# 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-69b2674c

olivergillespie · 2024-03-12T11:54:49Z

/backport jdk22u

openjdk · 2024-03-12T11:55:22Z

@olivergillespie the backport was successfully created on the branch backport-olivergillespie-69b2674c 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 69b2674c from the openjdk/jdk repository.

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

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-69b2674c:backport-olivergillespie-69b2674c
$ git checkout backport-olivergillespie-69b2674c
# 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-69b2674c

⚠️ @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.

8324648: Avoid NoSuchMethodError when instantiating NativePRNG

d537b80

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

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

Throw exception if params non-null, remove original constructors

de4d83a

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

44958ef

liach approved these changes Feb 6, 2024

View reviewed changes

valeriepeng reviewed Feb 6, 2024

View reviewed changes

Remove super() calls

e51350b

valeriepeng approved these changes Feb 7, 2024

View reviewed changes

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 8324648 branch February 13, 2024 11:19

fguallini mentioned this pull request Aug 15, 2024

8338395: Add test coverage for instantiating NativePRNG with SecureRandomParameters #20592

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8324648: Avoid NoSuchMethodError when instantiating NativePRNG #17560

8324648: Avoid NoSuchMethodError when instantiating NativePRNG #17560

olivergillespie commented Jan 24, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jan 24, 2024

openjdk bot commented Jan 24, 2024

mlbridge bot commented Jan 24, 2024 •

edited

Loading

liach commented Jan 24, 2024

olivergillespie commented Jan 25, 2024

shipilev left a comment

openjdk bot commented Jan 29, 2024 •

edited

Loading

openjdk bot commented Jan 29, 2024

valeriepeng commented Jan 30, 2024

wangweij commented Jan 31, 2024

olivergillespie commented Jan 31, 2024

olivergillespie commented Feb 1, 2024

wangweij commented Feb 2, 2024 •

edited

Loading

olivergillespie commented Feb 6, 2024

wangweij left a comment

valeriepeng Feb 6, 2024 •

edited

Loading

olivergillespie Feb 7, 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

8324648: Avoid NoSuchMethodError when instantiating NativePRNG #17560

8324648: Avoid NoSuchMethodError when instantiating NativePRNG #17560

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

liach commented Jan 24, 2024

olivergillespie commented Jan 25, 2024

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

valeriepeng commented Jan 30, 2024

wangweij commented Jan 31, 2024

olivergillespie commented Jan 31, 2024

olivergillespie commented Feb 1, 2024

wangweij commented Feb 2, 2024 • edited Loading

olivergillespie commented Feb 6, 2024

wangweij left a comment

Choose a reason for hiding this comment

valeriepeng Feb 6, 2024 • edited Loading

Choose a reason for hiding this comment

olivergillespie Feb 7, 2024

Choose a reason for hiding this comment

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 Jan 24, 2024 •

edited by openjdk bot

Loading

mlbridge bot commented Jan 24, 2024 •

edited

Loading

openjdk bot commented Jan 29, 2024 •

edited

Loading

wangweij commented Feb 2, 2024 •

edited

Loading

valeriepeng Feb 6, 2024 •

edited

Loading