8273026: Slow LoginContext.login() on multi threading application #5748

Larry-N · 2021-09-29T10:45:41Z

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.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8273026: Slow LoginContext.login() on multi threading application

Reviewers

Weijun Wang (@wangweij - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5748/head:pull/5748
$ git checkout pull/5748

Update a local copy of the PR:
$ git checkout pull/5748
$ git pull https://git.openjdk.java.net/jdk pull/5748/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5748

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5748.diff

bridgekeeper · 2021-09-29T10:46:49Z

👋 Welcome back Larry-N! 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 · 2021-09-29T10:50:51Z

@Larry-N 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 · 2021-09-29T10:55:06Z

Webrevs

wangweij · 2021-10-04T13:59:43Z

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

+                        (PrivilegedAction<ServiceLoader<LoginModule>>)
+                                () -> java.util.ServiceLoader.load(
+                                    LoginModule.class, contextClassLoader));
+                Set<Provider<LoginModule>> lmProviders = sc.stream().collect(Collectors.toSet());


It's probably necessary to put the collect call above inside doPriv as well. The actual class loading might happen later.

I'm seeing a JCK failure on this change. Will investigate more. At the same time, is it possible to merge this synchronized block into the one below?

wangweij · 2021-10-04T14:18:45Z

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

+                        Set<Provider<LoginModule>> lmp = cacheServiceProviders.get(contextClassLoader);
+                        for ( Provider<LoginModule> lm: lmp){
+                            if (lm.type().getName().equals(name)){
+                                moduleStack[i].module = lm.get();


There is a small behavior change here. Originally the login module class is loaded first and then its name is checked. With this change, the name is first checked and then class loaded. This is certainly a performance boost but unfortunately the test at test/jdk/javax/security/auth/spi/Loader.java would fail. That test was written to ensure that services provided through a service loader are checked first and then fallback to Class.forName(). Either you need to enhance the test to confirm this in a new way, or it would have to be removed.

Larry-N · 2021-10-27T13:02:05Z

@wangweij With the new commit, I've moved filling ServiceProviders cache filling to login() method. And addressed your notice about correction for spi/Loader.java test. Could you please take a look when you have a chance? Thank you.

wangweij

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

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.

wangweij · 2021-10-27T13:37:06Z

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

@@ -222,6 +226,7 @@

    private static final sun.security.util.Debug debug =
        sun.security.util.Debug.getInstance("logincontext", "\t[LoginContext]");
+    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

wangweij · 2021-10-27T13:41:17Z

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

+                                moduleStack[i].module = lm.get();
+                                if (debug != null) {
+                                    debug.println(name + " loaded as a service");
+                                }


Please de-indent the if block above.

Larry-N · 2021-10-27T18:36:45Z

Thanks for the review, I'll update the code according to your notices.

Sorry, I didn't catch the point with the test.
Why the lc.login(); succeed if SecondLoginModule is compiled or not?

"so that the login succeeds only after there exists a SecondLoginModule"

maybe you mean the test Loader.java itself? I have rolled back changes in Loader.java and FirstLoginModule.java, added directives that you proposed to Loader.java, but the test fails.

wangweij · 2021-10-27T19:56:30Z

Have you removed the isLoaded flags (in both login modules)? If they are still referenced in Loader, the two files will always be compiled before running the test.

Larry-N · 2021-10-28T15:36:24Z

Probably i do something wrong, but I've removed the isLoaded from both login modules classes, so the test looks like this:

 * @build FirstLoginModule
 * @run main/othervm/fail Loader
 * @build SecondLoginModule
 * @run main/othervm Loader
 */
public class Loader {

    public static void main(String[] args) throws Exception {

        System.setProperty("java.security.auth.login.config",
                new File(System.getProperty("test.src"), "sl.conf").toString());
        LoginContext lc = new LoginContext("me");

        lc.login();

    }
}

and the jtreg says:
Failed. Execution passed unexpectedly:

wangweij · 2021-10-28T16:01:33Z

On my machine, the 1st @run fails as expected with "java.util.ServiceConfigurationError: javax.security.auth.spi.LoginModule: Provider SecondLoginModule not found". When it passed on your machine, can you find SecondLoginModule.class in jtreg's working directory? If I stop at the first @run I only see Loader.class and FirstLoginModule.class. Maybe you need to clean up the working directory first?

Larry-N · 2021-10-28T17:31:26Z

Thank you for the explanations. When I cleaned up the working directory all pass ok. ( And fails when I submitted the test a second time)

wangweij · 2021-10-28T17:40:53Z

Thank you for the explanations. When I cleaned up the working directory all pass ok. ( And fails when I submitted the test a second time)

Let's hope the directory is always clean when the test is actually launched. I have no other comments. Thanks for the patience.

openjdk · 2021-10-28T17:42:13Z

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

8273026: Slow LoginContext.login() on multi threading application

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

15fd8a3: 8276102: JDK-8245095 integration reverted JDK-8247980
e89b2c0: 8276086: Increase size of metaspace mappings
24cf480: 8276047: G1: refactor G1CardSetArrayLocker
e922023: 8275909: [JVMCI] c2v_readFieldValue use long instead of jlong for the offset parameter
157e1d5: 8275856: Remove MetaspaceHandleDeallocations debug switch
c9e65f8: 8275440: Remove VirtualSpaceList::is_full()
de93b1d: 8257722: Improve "keytool -printcert -jarfile" output
21da218: 8274848: LambdaMetaFactory::metafactory on REF_invokeSpecial impl method has incorrect behavior
48f3fca: 8275308: Add valueOf(Runtime.Version) factory to SourceVersion
c6339cb: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle
... and 387 more: https://git.openjdk.java.net/jdk/compare/79865cd797737f22cd4efe7e9c03ddbb86095e64...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 (@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).

wangweij · 2021-10-28T17:45:15Z

You might want to see if the "clean" action is useful here. See https://openjdk.java.net/jtreg/tag-spec.html.

Update: I just tried. Adding a @clean SecondLoginModule before the 1st @run makes sure the class is removed.

Larry-N · 2021-10-28T18:50:51Z

Thanks, I've added @clean as you propose.

Larry-N · 2021-10-29T10:27:32Z

/integrate

openjdk · 2021-10-29T10:28:32Z

@Larry-N
Your change (at version 842eb37) is now ready to be sponsored by a Committer.

wangweij · 2021-10-29T11:36:42Z

/sponsor

openjdk · 2021-10-29T11:37:58Z

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

15fd8a3: 8276102: JDK-8245095 integration reverted JDK-8247980
e89b2c0: 8276086: Increase size of metaspace mappings
24cf480: 8276047: G1: refactor G1CardSetArrayLocker
e922023: 8275909: [JVMCI] c2v_readFieldValue use long instead of jlong for the offset parameter
157e1d5: 8275856: Remove MetaspaceHandleDeallocations debug switch
c9e65f8: 8275440: Remove VirtualSpaceList::is_full()
de93b1d: 8257722: Improve "keytool -printcert -jarfile" output
21da218: 8274848: LambdaMetaFactory::metafactory on REF_invokeSpecial impl method has incorrect behavior
48f3fca: 8275308: Add valueOf(Runtime.Version) factory to SourceVersion
c6339cb: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle
... and 387 more: https://git.openjdk.java.net/jdk/compare/79865cd797737f22cd4efe7e9c03ddbb86095e64...master

Your commit was automatically rebased without conflicts.

openjdk · 2021-10-29T11:38:21Z

@wangweij @Larry-N Pushed as commit c0cda1d.

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

efge · 2021-11-02T20:39:47Z

Could the original JDK-8230297 be closed as a duplicate please?

wangweij · 2021-11-02T23:15:54Z

@efge Closed. Thanks for reminding.

8273026: Slow LoginContext.login() on multi threading application

026a41c

openjdk bot added the rfr Pull request is ready for review label Sep 29, 2021

openjdk bot added the security security-dev@openjdk.org label Sep 29, 2021

wangweij reviewed Oct 4, 2021

View reviewed changes

1. Address JCK failure 2.Address review notes

0b181a3

openjdk bot removed the rfr Pull request is ready for review label Oct 27, 2021

Corrected trailing whitespaces

fbe99ed

openjdk bot added the rfr Pull request is ready for review label Oct 27, 2021

wangweij reviewed Oct 27, 2021

View reviewed changes

Address review notices

c4d744a

openjdk bot removed the rfr Pull request is ready for review label Oct 27, 2021

Trailing spaces

a6cc615

openjdk bot added the rfr Pull request is ready for review label Oct 27, 2021

A Corrected the Loader.java testcase

9ab2090

wangweij approved these changes Oct 28, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 28, 2021

Address review notes

842eb37

wangweij approved these changes Oct 28, 2021

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Oct 29, 2021

openjdk bot closed this Oct 29, 2021

openjdk bot added integrated Pull request has been integrated and 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 Oct 29, 2021

Larry-N deleted the JDK-8273026 branch October 29, 2021 13:05

8273026: Slow LoginContext.login() on multi threading application #5748

8273026: Slow LoginContext.login() on multi threading application #5748

Larry-N commented Sep 29, 2021 •

edited by openjdk bot

bridgekeeper bot commented Sep 29, 2021

openjdk bot commented Sep 29, 2021

mlbridge bot commented Sep 29, 2021 •

edited

wangweij Oct 4, 2021

wangweij Oct 5, 2021

wangweij Oct 4, 2021

Larry-N commented Oct 27, 2021

wangweij left a comment •

edited

wangweij Oct 27, 2021

wangweij Oct 27, 2021

Larry-N commented Oct 27, 2021

wangweij commented Oct 27, 2021 •

edited

Larry-N commented Oct 28, 2021

wangweij commented Oct 28, 2021 •

edited

Larry-N commented Oct 28, 2021

wangweij commented Oct 28, 2021

openjdk bot commented Oct 28, 2021 •

edited

wangweij commented Oct 28, 2021 •

edited

Larry-N commented Oct 28, 2021

Larry-N commented Oct 29, 2021

openjdk bot commented Oct 29, 2021

wangweij commented Oct 29, 2021

openjdk bot commented Oct 29, 2021

openjdk bot commented Oct 29, 2021

efge commented Nov 2, 2021

wangweij commented Nov 2, 2021

8273026: Slow LoginContext.login() on multi threading application #5748

8273026: Slow LoginContext.login() on multi threading application #5748

Conversation

Larry-N commented Sep 29, 2021 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Sep 29, 2021

openjdk bot commented Sep 29, 2021

mlbridge bot commented Sep 29, 2021 • edited

Webrevs

wangweij Oct 4, 2021

Choose a reason for hiding this comment

wangweij Oct 5, 2021

Choose a reason for hiding this comment

wangweij Oct 4, 2021

Choose a reason for hiding this comment

Larry-N commented Oct 27, 2021

wangweij left a comment • edited

Choose a reason for hiding this comment

wangweij Oct 27, 2021

Choose a reason for hiding this comment

wangweij Oct 27, 2021

Choose a reason for hiding this comment

Larry-N commented Oct 27, 2021

wangweij commented Oct 27, 2021 • edited

Larry-N commented Oct 28, 2021

wangweij commented Oct 28, 2021 • edited

Larry-N commented Oct 28, 2021

wangweij commented Oct 28, 2021

openjdk bot commented Oct 28, 2021 • edited

wangweij commented Oct 28, 2021 • edited

Larry-N commented Oct 28, 2021

Larry-N commented Oct 29, 2021

openjdk bot commented Oct 29, 2021

wangweij commented Oct 29, 2021

openjdk bot commented Oct 29, 2021

openjdk bot commented Oct 29, 2021

efge commented Nov 2, 2021

wangweij commented Nov 2, 2021

Larry-N commented Sep 29, 2021 •

edited by openjdk bot

mlbridge bot commented Sep 29, 2021 •

edited

wangweij left a comment •

edited

wangweij commented Oct 27, 2021 •

edited

wangweij commented Oct 28, 2021 •

edited

openjdk bot commented Oct 28, 2021 •

edited

wangweij commented Oct 28, 2021 •

edited