On Mon, 15 Mar 2021 23:31:33 GMT, Valerie Peng <valer...@openjdk.org> wrote:
>> I would definitely not call `new SecureRandom()` each time as that can have
>> real performance problems (especially on systems which pull from
>> `/dev/random` and may not have hardware entropy). The synchronization
>> worries me too.
>>
>> Could `java.security.Security` contain a "provider version" `int` (volatile,
>> atomic, or similar) which is incremented every time the provider list is
>> changed? Then this class (clearly using some non-public API) would check
>> that value and if it is the same, just return the cached instance. If it has
>> changed since the last time, then it would update `instance` and its
>> remembered value of the index. I think that we might be able to do it
>> without taking any locks in this class because even if the providers change
>> out from under us, we returned a reasonable value (subject to
>> multi-threading) and will fix things up when we're called the next time.
>>
>> // using this and name collision to highlight what's a field and what's local
>> int providerListVersion = Security.nonPublicGetProviderListVersion();
>> if (this.providerListVersion != providerListVersion) {
>> this.providerListVersion = providerListVersion;
>> this.instance = new SecureRandom();
>> }
>> return this.instance;
>
> Or, I can also separate out the instance returned by getSecureRandom() so
> that code path should be unaffected, e.g.:
> // cached SecureRandom instance
> private static class CachedSecureRandomHolder {
> public static SecureRandom instance = new SecureRandom();
> private static Provider[] cachedConfig = Security.getProviders();
> private static SecureRandom def = instance;
>
> public static SecureRandom getDefault() {
> synchronized (CachedSecureRandomHolder.class) {
> Provider[] currConfig = Security.getProviders();
> if (!Arrays.equals(cachedConfig, currConfig)) {
> def = new SecureRandom();
> cachedConfig = currConfig;
> }
> }
> return def;
> }
> }
>
> /**
> * Get a SecureRandom instance. This method should be used by JDK
> * internal code in favor of calling "new SecureRandom()". That needs to
> * iterate through the provider table to find the default SecureRandom
> * implementation, which is fairly inefficient.
> */
> public static SecureRandom getSecureRandom() {
> return CachedSecureRandomHolder.instance;
> }
>
> /**
> * Get the default SecureRandom instance. This method is the
> * optimized version of "new SecureRandom()" which re-uses the default
> * SecureRandom impl if the provider table is the same.
> */
> public static SecureRandom getDefSecureRandom() {
> return CachedSecureRandomHolder.getDefault();
> }
Doesn't that still incur a JVM-wide lock whenever
`CachedSecureRandomHolder.getDefSecureRandom()` is called? I recall a
different JVM-wide lock in the `getInstance` path
([JDK-7107615](https://bugs.openjdk.java.net/browse/JDK-7107615)) creating
significant performance issues.
-------------
PR: https://git.openjdk.java.net/jdk/pull/3018
