RFR: 8276660: Scalability bottleneck in java.security.Provider.getService() [v2]

[Weijun Wang]

On Wed, 24 Nov 2021 21:17:34 GMT, Valerie Peng <valeriep at openjdk.org> wrote:

>> It is observed that when running crypto benchmark with large number of threads, a lot of time is spent on the synchronized block inside the Provider.getService() method. The cause for this is that Provider.getService() method first uses the 'serviceMap' field to find the requested service. However, when the requested service is not supported by this provider, e.g. requesting Cipher.RSA from SUN provider, the impl continues to try searching the legacy registrations whose processing is guarded by the "synchronized" keyword. When apps use getInstance() calls without the provider argument, Provider class has to iterate through existing providers trying to find one that supports the requested service.
>> 
>> Now that the parent class of Provider no longer synchronizes all of its methods, Provider class should follow suit and de-synchronize its methods. Parsing of the legacy registration is done eagerly (at the time of put(...) calls) instead of lazily (at the time of getService(...) calls). This also makes "legacyStrings" redundant as the registration is parsed and stored directly into "legacyMap". 
>> 
>> The bug reporter has confirmed that the changes resolve the performance bottleneck and all regression tests pass.
>> 
>> Please review and thanks in advance,
>> Valerie
>
> Valerie Peng has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Updated to use pattern matching with instanceof operator.

Some comments. I'm more concerned about the `parseLegacy()` method which is called everywhere. Without the synchronized keyword, is it safe to call into this method by multiple threads at the same time? Do we have tests around this?

src/java.base/share/classes/java/security/Provider.java line 832:

> 830:     // NOTE: may need extra mechanism for providers to indicate their
> 831:     // preferred ordering of SecureRandom algorithms since registration
> 832:     // ordering info is lost once serialized

Why is the ordering info lost once serialized? Weren't all entries re-added again in their original order?

src/java.base/share/classes/java/security/Provider.java line 901:

> 899:             legacyChanged = true;
> 900:         }
> 901:         return true;

Better put this "return" line into the else block.

src/java.base/share/classes/java/security/Provider.java line 979:

> 977:                 parseLegacy(sk, sv, OPType.REPLACE);
> 978:             }
> 979:         }

If you are going through all the entries, should we also clean up the legacy sets and restart?

-------------

PR: https://git.openjdk.java.net/jdk/pull/6513