You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by re...@apache.org on 2001/09/07 03:23:25 UTC
cvs commit: jakarta-tomcat-4.0/catalina/src/share/org/apache/catalina/loader WebappClassLoader.java
remm 01/09/06 18:23:25
Modified: catalina/src/share/org/apache/catalina/loader
WebappClassLoader.java
Log:
- Should make the classloading fully thread safe (apparently synchronizing on
the defineClass is not enough).
- The critical path of findResourceInternal or findClassInternal should not be
synchronized for performance reasons, because it is called very often (unlike
the call to defineClass which only happens once).
- I assume Peter will tell me it still doesn't work (in which case I would appreciate
a test case) ;-)
Revision Changes Path
1.14 +23 -9 jakarta-tomcat-4.0/catalina/src/share/org/apache/catalina/loader/WebappClassLoader.java
Index: WebappClassLoader.java
===================================================================
RCS file: /home/cvs/jakarta-tomcat-4.0/catalina/src/share/org/apache/catalina/loader/WebappClassLoader.java,v
retrieving revision 1.13
retrieving revision 1.14
diff -u -r1.13 -r1.14
--- WebappClassLoader.java 2001/08/28 18:12:37 1.13
+++ WebappClassLoader.java 2001/09/07 01:23:25 1.14
@@ -1,7 +1,7 @@
/*
- * $Header: /home/cvs/jakarta-tomcat-4.0/catalina/src/share/org/apache/catalina/loader/WebappClassLoader.java,v 1.13 2001/08/28 18:12:37 remm Exp $
- * $Revision: 1.13 $
- * $Date: 2001/08/28 18:12:37 $
+ * $Header: /home/cvs/jakarta-tomcat-4.0/catalina/src/share/org/apache/catalina/loader/WebappClassLoader.java,v 1.14 2001/09/07 01:23:25 remm Exp $
+ * $Revision: 1.14 $
+ * $Date: 2001/09/07 01:23:25 $
*
* ====================================================================
*
@@ -123,7 +123,7 @@
*
* @author Remy Maucherat
* @author Craig R. McClanahan
- * @version $Revision: 1.13 $ $Date: 2001/08/28 18:12:37 $
+ * @version $Revision: 1.14 $ $Date: 2001/09/07 01:23:25 $
*/
public class WebappClassLoader
extends URLClassLoader
@@ -1482,11 +1482,17 @@
}
synchronized(this) {
- clazz = defineClass(name, entry.binaryContent, 0,
- entry.binaryContent.length, codeSource);
+ // Since all threads use the same ResourceEntry instance, it is
+ // the one which will contain the class
+ if (entry.loadedClass == null) {
+ clazz = defineClass(name, entry.binaryContent, 0,
+ entry.binaryContent.length, codeSource);
+ entry.loadedClass = clazz;
+ } else {
+ clazz = entry.loadedClass;
+ }
}
- entry.loadedClass = clazz;
return clazz;
@@ -1635,8 +1641,16 @@
entry.binaryContent = binaryContent;
// Add the entry in the local resource repository
- synchronized (resourceEntries) {
- resourceEntries.put(name, entry);
+ synchronized (this) {
+ // Ensures that all the threads which may be in a race to load
+ // a particular class all end up with the same ResourceEntry
+ // instance
+ ResourceEntry entry2 = (ResourceEntry) resourceEntries.get(name);
+ if (entry2 == null) {
+ resourceEntries.put(name, entry);
+ } else {
+ entry = entry2;
+ }
}
return entry;
Re: cvs commit: jakarta-tomcat-4.0/catalina/src/share/org/apache/catalina/loader WebappClassLoader.java
Posted by Remy Maucherat <re...@apache.org>.
> yes, the solution looks like a great deal in both, performance and
> stability,
> enven though i found out that synchronizing the ResourceEntry entry is
> enough. (at least it does not cause my good old error :-)
Most likely itwould be enough, since the sync block in findResourceInternal
made sure that all threads ended up with the same Entry instance. So syncing
on that object should be enough.
I'll commit the change. If there's a problem, let me know.
> i think the concurrent access to the ResourceEntry-object is the point
which
> leads to the error.
> (but i'm not sure if changing synchronized (this) to synchronized (entry)
> will have any signficant positive impact on the performance...)
Essentially, that should allow concurrent CL on different classes, which was
the goal (although it won't have any impact in the real world performance).
> keep on the good work!
I'll try to :)
Remy
RE: cvs commit: jakarta-tomcat-4.0/catalina/src/share/org/apache/catalina/loader WebappClassLoader.java
Posted by Peter Romianowski <an...@gmx.de>.
yes, the solution looks like a great deal in both, performance and
stability,
enven though i found out that synchronizing the ResourceEntry entry is
enough. (at least it does not cause my good old error :-)
i think the concurrent access to the ResourceEntry-object is the point which
leads to the error.
(but i'm not sure if changing synchronized (this) to synchronized (entry)
will have any signficant positive impact on the performance...)
keep on the good work!
pero
-----Original Message-----
From: Remy Maucherat [mailto:remm@apache.org]
Sent: Friday, September 07, 2001 7:19 PM
To: tomcat-dev@jakarta.apache.org
Subject: Re: cvs commit:
jakarta-tomcat-4.0/catalina/src/share/org/apache/catalina/loader
WebappClassLoader.java
> no, Peter tells you that it is WORKING! Yep.
Way COOL ! It's great, because it would have been bad to sync anywhere on
the critical path (since findResourceInternal is called quite often). The
updated code doesn't hurt performance in any significant way (it strenghtens
a bit one sync, and makes two more HashMaps lookups during the class
loading), so that's great news.
Remy
Re: cvs commit: jakarta-tomcat-4.0/catalina/src/share/org/apache/catalina/loader WebappClassLoader.java
Posted by Remy Maucherat <re...@apache.org>.
> no, Peter tells you that it is WORKING! Yep.
Way COOL ! It's great, because it would have been bad to sync anywhere on
the critical path (since findResourceInternal is called quite often). The
updated code doesn't hurt performance in any significant way (it strenghtens
a bit one sync, and makes two more HashMaps lookups during the class
loading), so that's great news.
Remy
RE: cvs commit: jakarta-tomcat-4.0/catalina/src/share/org/apache/catalina/loader WebappClassLoader.java
Posted by Peter Romianowski <an...@gmx.de>.
no, Peter tells you that it is WORKING! Yep.
thanks,
pero
-----Original Message-----
From: remm@apache.org [mailto:remm@apache.org]
Sent: Friday, September 07, 2001 3:23 AM
To: jakarta-tomcat-4.0-cvs@apache.org
Subject: cvs commit:
jakarta-tomcat-4.0/catalina/src/share/org/apache/catalina/loader
WebappClassLoader.java
remm 01/09/06 18:23:25
Modified: catalina/src/share/org/apache/catalina/loader
WebappClassLoader.java
Log:
- Should make the classloading fully thread safe (apparently synchronizing
on
the defineClass is not enough).
- The critical path of findResourceInternal or findClassInternal should
not be
synchronized for performance reasons, because it is called very often
(unlike
the call to defineClass which only happens once).
- I assume Peter will tell me it still doesn't work (in which case I would
appreciate
a test case) ;-)
Revision Changes Path
1.14 +23 -9
jakarta-tomcat-4.0/catalina/src/share/org/apache/catalina/loader/WebappClass
Loader.java
Index: WebappClassLoader.java
===================================================================
RCS file:
/home/cvs/jakarta-tomcat-4.0/catalina/src/share/org/apache/catalina/loader/W
ebappClassLoader.java,v
retrieving revision 1.13
retrieving revision 1.14
diff -u -r1.13 -r1.14
--- WebappClassLoader.java 2001/08/28 18:12:37 1.13
+++ WebappClassLoader.java 2001/09/07 01:23:25 1.14
@@ -1,7 +1,7 @@
/*
- * $Header:
/home/cvs/jakarta-tomcat-4.0/catalina/src/share/org/apache/catalina/loader/W
ebappClassLoader.java,v 1.13 2001/08/28 18:12:37 remm Exp $
- * $Revision: 1.13 $
- * $Date: 2001/08/28 18:12:37 $
+ * $Header:
/home/cvs/jakarta-tomcat-4.0/catalina/src/share/org/apache/catalina/loader/W
ebappClassLoader.java,v 1.14 2001/09/07 01:23:25 remm Exp $
+ * $Revision: 1.14 $
+ * $Date: 2001/09/07 01:23:25 $
*
* ====================================================================
*
@@ -123,7 +123,7 @@
*
* @author Remy Maucherat
* @author Craig R. McClanahan
- * @version $Revision: 1.13 $ $Date: 2001/08/28 18:12:37 $
+ * @version $Revision: 1.14 $ $Date: 2001/09/07 01:23:25 $
*/
public class WebappClassLoader
extends URLClassLoader
@@ -1482,11 +1482,17 @@
}
synchronized(this) {
- clazz = defineClass(name, entry.binaryContent, 0,
- entry.binaryContent.length, codeSource);
+ // Since all threads use the same ResourceEntry instance, it
is
+ // the one which will contain the class
+ if (entry.loadedClass == null) {
+ clazz = defineClass(name, entry.binaryContent, 0,
+ entry.binaryContent.length,
codeSource);
+ entry.loadedClass = clazz;
+ } else {
+ clazz = entry.loadedClass;
+ }
}
- entry.loadedClass = clazz;
return clazz;
@@ -1635,8 +1641,16 @@
entry.binaryContent = binaryContent;
// Add the entry in the local resource repository
- synchronized (resourceEntries) {
- resourceEntries.put(name, entry);
+ synchronized (this) {
+ // Ensures that all the threads which may be in a race to
load
+ // a particular class all end up with the same ResourceEntry
+ // instance
+ ResourceEntry entry2 = (ResourceEntry)
resourceEntries.get(name);
+ if (entry2 == null) {
+ resourceEntries.put(name, entry);
+ } else {
+ entry = entry2;
+ }
}
return entry;