Bug 57681 - Allow parallel class loading in web application class loader by synchronizing on class specific object
Allow parallel class loading in web application class loader by synchronizing...
Status: RESOLVED FIXED
Product: Tomcat 7
Classification: Unclassified
Component: Catalina
trunk
PC All
: P2 trivial (vote)
: ---
Assigned To: Tomcat Developers Mailing List
:
Depends on:
Blocks:
  Show dependency tree
 
Reported: 2015-03-09 22:34 UTC by Alex
Modified: 2015-09-08 14:20 UTC (History)
2 users (show)



Attachments
Parallel classloading port from Tc8.0 into Tc7.0 (821 bytes, patch)
2015-03-09 22:34 UTC, Alex
Details | Diff
Patch to work with JDK1.6 (2.76 KB, patch)
2015-03-10 16:08 UTC, Alex
Details | Diff
Lock free read for already loaded/cached classes (1.35 KB, patch)
2015-03-17 21:47 UTC, Alex
Details | Diff
Back port parallel class loading to tomcat 7. (289.84 KB, patch)
2015-07-22 13:45 UTC, Huxing Zhang
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex 2015-03-09 22:34:15 UTC
Created attachment 32553 [details]
Parallel classloading port from Tc8.0 into Tc7.0

Related to fix in Tomcat8.0:
Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=56530
Add a web application class loader implementation that supports the parallel loading of web application classes.

Source file to patch:
https://svn.apache.org/viewvc/tomcat/tc7.0.x/trunk/java/org/apache/catalina/loader/WebappClassLoader.java?revision=1661811&view=markup
Comment 1 Mark Thomas 2015-03-09 22:37:24 UTC

*** This bug has been marked as a duplicate of bug 56530 ***
Comment 2 Mark Thomas 2015-03-09 22:38:15 UTC
And for the record the patch is nowhere close to correct.
Comment 3 Alex 2015-03-09 22:44:53 UTC
The fix in bug 56530 is for Tomcat 8, not Tomcat 7. Why my request is marked as resolved?
Comment 4 Mark Thomas 2015-03-09 22:46:42 UTC
Because we don't have separate Bugzilla entries for exactly the same issue in different versions of Tomcat.
Comment 5 Alex 2015-03-10 16:08:28 UTC
Created attachment 32554 [details]
Patch to work with JDK1.6
Comment 6 Alex 2015-03-10 16:12:28 UTC
Attachment 'Parallel classloading port from Tc8.0 into Tc7.0' was incomplete and should not be used, apologies for attaching a wrong diff.

'Patch to work with JDK6' re-implements getClassLoadingLock() in WebappClassloader and turns serial WebappClassLoader into parallel.
Comment 7 Mark Thomas 2015-03-10 17:13:27 UTC
This patch is also wrong.

THe correct way to address this is described in bug 56530.
Comment 8 Alex 2015-03-17 21:42:43 UTC
Comment on attachment 32554 [details]
Patch to work with JDK1.6

--- WebappClassLoader_r1661811.java	2015-03-09 21:53:27.000000000 +0000
+++ WebappClassLoader_patch.java	2015-03-17 14:21:56.000000000 +0000
@@ -1599,7 +1599,25 @@ public class WebappClassLoader
      * @exception ClassNotFoundException if the class was not found
      */
     @Override
-    public synchronized Class<?> loadClass(String name, boolean resolve)
+    public Class<?> loadClass(String name, boolean resolve)
+        throws ClassNotFoundException {
+        // check local cache without entering the global synchronized block and return if the class is found
+        if (resourceEntries.containsKey(name)) {
+            Class<?> clazz = findLoadedClass0(name);
+            if (clazz != null) {
+                if (log.isDebugEnabled())
+                    log.debug("  Returning class from cache");
+                if (resolve)
+                    resolveClass(clazz);
+                return (clazz);
+            }
+            
+        }
+        // class is not found in local cache - call the original synchronized method
+        return loadClass0(name, resolve);
+    }
+
+    private synchronized Class<?> loadClass0(String name, boolean resolve)
         throws ClassNotFoundException {
 
         if (log.isDebugEnabled())
Comment 9 Alex 2015-03-17 21:47:44 UTC
Created attachment 32584 [details]
Lock free read for already loaded/cached classes
Comment 10 Mark Thomas 2015-03-17 22:01:48 UTC
(In reply to Alex Koturanov from comment #8)
> Comment on attachment 32554 [details]
> Patch to work with JDK1.6

Doesn't implement parallel loading.

Creates significant risk of a ConcurrentModificationException during the lock free read.

Ignores the discussion in bug 56530.
Comment 11 Huxing Zhang 2015-07-22 13:45:57 UTC
Created attachment 32927 [details]
Back port parallel class loading to tomcat 7.

Hi Mark, Christopher, and Konstantin:

I have back ported the parallel class loading feature to tomcat7. 

Could you please take some time to review this?

The basic idea here is:

1. Keep WebappClassLoader as default which is serial class loading enabled, and one can configure to use ParallelWebappClassloader by specifying loaderClass in context.xml (just as in Tomcat8). If user specified to use ParallelWebappClassloader,  parallel class loading will only be enabled if tomcat is running on JRE7 or above.
2. Call ClassLoader.registerAsParallelCapable() via reflection, so that jdk7 is still not required to build tomcat 7 core code.
3. Call ClassLoader.getClassLoadingLock() via reflection to obtain a dedicated object lock if parallel class loading enabled, or return the WebappClassLoader object lock if not.
4. Introduce JreCompat.getInstance().isJre7Supported() to determine whether tomcat is running on JRE7 (or above) or not.
5. Remove StandardClassLoader as well as MBean registration in Bootstrap, since
   1) it is deprecated
   2) it can avoid adding JRECompat to Bootstrap class loader's classpath
6. Add unit test to make sure parallel class loading is working as expected.
Comment 12 Huxing Zhang 2015-07-27 02:30:15 UTC
Hi, Any update on this?
Comment 13 Huxing Zhang 2015-07-30 01:51:51 UTC
Is there anything else I can do to push it forward?
The patch respects the discussion in bug 56530.
Comment 14 Mark Thomas 2015-09-04 08:20:07 UTC
Removing deprecated code is not an option since it may break existing code. That is why it was deprecated rather than deleted in the first place. The patch needs to be re-worked not to do this.

I've started to add some of the external plumbing required by this feature so the patch should be a little smaller.
Comment 15 Huxing Zhang 2015-09-04 10:37:58 UTC
To make StandardClassLoader parallel capable, we can simply add JRECompact to the Bootstrap class loader's class path, and then apply the static initialization of WebappClassLoaderBase's to it.
If we don't want to JreCompat to be in the Bootstrap ClassLoader's class path, I think adding the following code should work for StandardClassLoader:

public class StandardClassLoader {
    ...
    static {
        try {
            // parallel class loading capable
            final Method registerParallel =
                    ClassLoader.class.getDeclaredMethod("registerAsParallelCapable");
            AccessController.doPrivileged(new PrivilegedAction<Object>() {
                public Object run() {
                    registerParallel.setAccessible(true);
                    return null;
                }
            });
            registerParallel.invoke(null);
        } catch (NoSuchMethodException e) {
            // expect java 6 or lower
        } catch (Exception e) {
            // ignore
        }
    }
    ...
}

If there is anything else I can help, please feel free to let me know.
Comment 16 Mark Thomas 2015-09-08 11:04:00 UTC
I opted for the simpler replacement of StandardClassLoader with URLClassLoader wherever the StandardClassLoader was used.
Comment 17 Mark Thomas 2015-09-08 13:47:36 UTC
This has been fixed using a variation of this patch in 7.0.x for 7.0.65 onwards.

Many thanks for the patch.
Comment 18 Huxing Zhang 2015-09-08 14:17:58 UTC
Hi Mark,

Glad to hear the patch has been accepted! Thanks for your hard work!
BTW, I am interested in becoming a tomcat committer, any advice on that?
Comment 19 Mark Thomas 2015-09-08 14:20:09 UTC
(In reply to Huxing Zhang from comment #18)
> Hi Mark,
> 
> Glad to hear the patch has been accepted! Thanks for your hard work!
> BTW, I am interested in becoming a tomcat committer, any advice on that?

Keep contributing patches.