RFR: 8273026: Slow LoginContext.login() on multi threading application [v3]

[Weijun Wang]

On Wed, 27 Oct 2021 13:06:54 GMT, Larry-N <duke at openjdk.java.net> wrote:

>> This fix adds a cache of service provider classes to LoginContext (in particular, it's a cache of LoginModules classes). The approach helps to increase the performance of the LoginContext.login() method significantly, especially in a multi-threading environment. Service Loader is used for polling on Service Provider classes, without instantiating LoginModules object if Service Provider name doesn't match record in .config file. The set of service providers is cached for each Context Loader separately.
>> This code passed successfully tier1 and tier2 tests on mac os.
>
> Larry-N has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Corrected trailing whitespaces

Source code change looks mostly fine to me. I'm running some tests now. Will get back once they finish.

As for the test, IMO, it was meant to show that `SecondLoginModule` is still needed even if it's not in the JAAS login config file. Your updated test only prove it's not really instantiated. How about this:

Change the test directives to

 * @build FirstLoginModule
 * @run main/othervm/fail Loader
 * @build SecondLoginModule
 * @run main/othervm Loader

so that the login succeeds only after there exists a `SecondLoginModule`. All those `isLoaded` flags should be removed so their enclosing classes are not compiled automatically.

src/java.base/share/classes/javax/security/auth/login/LoginContext.java line 229:

> 227:     private static final sun.security.util.Debug debug =
> 228:         sun.security.util.Debug.getInstance("logincontext", "\t[LoginContext]");
> 229:     private static final WeakHashMap<ClassLoader, Set<Provider<LoginModule>>> cacheServiceProviders = new WeakHashMap<>();

This line can be a little shorter. There are some other lines below which is also quite long. The original limit was 80 chars but please be at most 90.

src/java.base/share/classes/javax/security/auth/login/LoginContext.java line 704:

> 702:                         lmProviders = cacheServiceProviders.get(contextClassLoader);
> 703:                         if (lmProviders == null){
> 704:                             if (debug != null)

Please enclose the following line in a pair of braces.

src/java.base/share/classes/javax/security/auth/login/LoginContext.java line 717:

> 715:                                 }
> 716:                             cacheServiceProviders.put(contextClassLoader,lmProviders);
> 717:                         }

Is it necessary the lines below still be inside the synchronized block?

src/java.base/share/classes/javax/security/auth/login/LoginContext.java line 723:

> 721:                                 if (debug != null) {
> 722:                                     debug.println(name + " loaded as a service");
> 723:                                 }

Please de-indent the if block above.

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

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