Hey guys-
Don't know if you want to accept this patch right before a release, but it has been working in production for us for quite some time. This is the patch that I brought up long ago regarding excessive use of Class.forName() in the JavaBaseFactory. I have included a path here for that file, changes.xml, and added myself to project.xml as one of a long list of contributors.
-B
Index: project.xml
===================================================================
RCS file: /home/cvspublic/jakarta-turbine-2/project.xml,v
retrieving revision 1.136.2.11
diff -u -r1.136.2.11 project.xml
--- project.xml 22 Aug 2004 23:51:59 -0000 1.136.2.11
+++ project.xml 24 Aug 2004 17:49:00 -0000
@@ -247,6 +247,10 @@
<email>[EMAIL PROTECTED]</email>
</contributor>
<contributor>
+ <name>Brian Lawler</name>
+ <email>[EMAIL PROTECTED]</email>
+ </contributor>
+ <contributor>
<name>Josh Lucas</name>
<email>[EMAIL PROTECTED]</email>
</contributor>
Index: src/java/org/apache/turbine/services/assemblerbroker/util/java/ JavaBaseFactory.java
===================================================================
RCS file: /home/cvspublic/jakarta-turbine-2/src/java/org/apache/turbine/services/ assemblerbroker/util/java/JavaBaseFactory.java,v
retrieving revision 1.8.2.3
diff -u -r1.8.2.3 JavaBaseFactory.java
--- src/java/org/apache/turbine/services/assemblerbroker/util/java/ JavaBaseFactory.java 16 Aug 2004 22:57:48 -0000 1.8.2.3
+++ src/java/org/apache/turbine/services/assemblerbroker/util/java/ JavaBaseFactory.java 24 Aug 2004 17:49:01 -0000
@@ -16,7 +16,11 @@
* limitations under the License.
*/
+import java.util.Collections; +import java.util.HashMap; import java.util.Iterator; +import java.util.Map; +import java.util.Vector; import java.util.List;
import org.apache.commons.lang.StringUtils;
@@ -48,6 +52,12 @@
/** Logging */
protected Log log = LogFactory.getLog(this.getClass());+ /**
+ * A cache for previously obtained Class instances, which we keep in order
+ * to reduce the Class.forName() overhead (which can be sizable).
+ */
+ private Map classCache = Collections.synchronizedMap(new HashMap());
+
static
{
ObjectUtils.addOnce(packages, GenericLoader.getBasePackage());
@@ -68,7 +78,7 @@
if (StringUtils.isNotEmpty(name))
{
- for (Iterator it = packages.iterator(); it.hasNext();)
+ for (Iterator it = packages.iterator(); assembler == null && it.hasNext();)
{
StringBuffer className = new StringBuffer();
@@ -82,7 +92,12 @@
try
{
- Class servClass = Class.forName(className.toString());
+ Class servClass = (Class) classCache.get(className);
+ if(servClass == null)
+ {
+ servClass = Class.forName(className.toString());
+ classCache.put(className, servClass);
+ }
assembler = (Assembler) servClass.newInstance();
break; // for()
}
Index: xdocs/changes.xml
===================================================================
RCS file: /home/cvspublic/jakarta-turbine-2/xdocs/changes.xml,v
retrieving revision 1.60.2.19
diff -u -r1.60.2.19 changes.xml
--- xdocs/changes.xml 22 Aug 2004 23:52:00 -0000 1.60.2.19
+++ xdocs/changes.xml 24 Aug 2004 17:49:01 -0000
@@ -92,6 +92,18 @@
</ul>
</p>
</subsection>
+<subsection name="Other changes">
+<p>
+ <ul>
+ <li>
+ JavaBaseFactory executed a Class.forName() every time it was
+ searching for a named class, which showed up as a very costly
+ API call in our profiling. A synchronized cache has been added
+ to cache previously obtained class instances inside this class.
+ </li>
+ </ul>
+</p>
+</subsection>
</section>
<section name="Turbine 2.3-rc2">
+++++++++++++++++++++++
On Jul 9, 2004, at 8:06 AM, Henning P. Schmiedehausen wrote:
Brian Lawler <[EMAIL PROTECTED]> writes:
Ah yes. The assembler == null bit was leftover from my previous implementation (where assemblers were cached instead of Class instances). I will correct this and send in that patch along with a patch to changes.xml.
Cool. Don't forget to add you to the contributors in project.xml :-)
I'll apply the patch then (unless someone objects).
Regards Henning
-B On Jul 8, 2004, at 2:52 AM, Henning P. Schmiedehausen wrote:
Brian Lawler <[EMAIL PROTECTED]> writes:
So at long last, here is the patch for JavaBaseFactory, for you
perusal
(again). Just to refresh the context, the problem that we ran into on
our site is that excessive calls to Class.forName() in the
JavaBaseFactory was bubbling up to the top of our profiler output.
Turns out that the Base Factory was repeatedly calling Class.forName
for the same class name. This is easily remedied by just creating a
Map of Class instances in the JavaBaseFactory class.
Hi,
looks ok to me. I do consider applying it. Please add a patch to xdocs/changes.xml which describes this change and the reason why.
Why are you adding the assembler == null to the for() loop? When the assembler gets set, the following break breaks the for() loop anyway, so this test is always true.
Regards Henning
Here is the patch, applied against TURBINE_2_3_BRANCH:
Index: src/java/org/apache/turbine/services/assemblerbroker/util/java/ JavaBaseFactory.java =================================================================== RCS file: /home/cvspublic/jakarta-turbine-2/src/java/org/apache/turbine/ services/ assemblerbroker/util/java/JavaBaseFactory.java,v retrieving revision 1.8.2.2 diff -u -r1.8.2.2 JavaBaseFactory.java --- src/java/org/apache/turbine/services/assemblerbroker/util/java/ JavaBaseFactory.java 20 May 2004 03:05:16 -0000 1.8.2.2 +++ src/java/org/apache/turbine/services/assemblerbroker/util/java/ JavaBaseFactory.java 7 Jul 2004 18:13:17 -0000 @@ -16,7 +16,10 @@ * limitations under the License. */
+import java.util.Collections; +import java.util.HashMap; import java.util.Iterator; +import java.util.Map; import java.util.Vector;
import org.apache.commons.lang.StringUtils;
@@ -35,9 +38,7 @@
* A screen factory that attempts to load a java class from
* the module packages defined in the TurbineResource.properties.
*
- * @author <a href="mailto:[EMAIL PROTECTED]">Leon
Messerschmidt</a>
- * @author <a href="mailto:[EMAIL PROTECTED]">Henning P.
Schmiedehausen</a>
- * @version $Id: JavaBaseFactory.java,v 1.8.2.2 2004/05/20 03:05:16
seade Exp $
+ * @version $Id: JavaBaseFactory.java,v 1.2 2004/07/07 17:45:05 brian
Exp $
*/
public abstract class JavaBaseFactory
implements AssemblerFactory
@@ -49,6 +50,12 @@
/** Logging */
protected Log log = LogFactory.getLog(this.getClass());
+ /**
+ * A cache for previously obtained Class instances, which we keep
in order
+ * to reduce the Class.forName() overhead (which can be sizable).
+ */
+ private Map classCache = Collections.synchronizedMap(new
HashMap());
+
static
{
ObjectUtils.addOnce(packages,
GenericLoader.getBasePackage());
@@ -69,7 +76,7 @@
if (StringUtils.isNotEmpty(name))
{
- for (Iterator it = packages.iterator(); it.hasNext();)
+ for (Iterator it = packages.iterator(); assembler == null
&& it.hasNext();)
{
StringBuffer className = new StringBuffer();
@@ -83,7 +90,12 @@
try
{
- Class servClass =
Class.forName(className.toString());
+ Class servClass = (Class)
classCache.get(className);
+ if(servClass == null)
+ {
+ servClass =
Class.forName(className.toString());
+ classCache.put(className, servClass);
+ }
assembler = (Assembler) servClass.newInstance();
break; // for()
}
On Jun 12, 2004, at 1:37 AM, Brian Lawler wrote:
Henning-
On May 9, 2004, at 4:37 PM, Henning P. Schmiedehausen wrote:
You might either want to have some sort of real life cycle (which
would be overkill for a point release like 2.3.1) or, if just
Class.forName() is your bottleneck, simply store the Class objects
and
not the instance objects. This could even be a class cache, not just
an instance cache, because translating
"com.mycorp.modules.actions.FooAction" to a Class object describing
the FooAction class is the same for all the instances.
Great point. I have re-implemented this as a Class cache, since
Class.forName() is indeed our bottleneck, and I will be testing it
over the next few days. I will send my patched patch when satisfied
that it is doing the right thing...
-B
------------------------------------------------------------------- --
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
-------------------------------------------------------------------- -
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
-- Dipl.-Inf. (Univ.) Henning P. Schmiedehausen INTERMETA GmbH [EMAIL PROTECTED] +49 9131 50 654 0 http://www.intermeta.de/
RedHat Certified Engineer -- Jakarta Turbine Development -- hero for hire Linux, Java, perl, Solaris -- Consulting, Training, Development
"Fighting for one's political stand is an honourable action, but re- fusing to acknowledge that there might be weaknesses in one's position - in order to identify them so that they can be remedied - is a large enough problem with the Open Source movement that it deserves to be on this list of the top five problems." -- Michelle Levesque, "Fundamental Issues with Open Source Software Development"
--------------------------------------------------------------------- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
--------------------------------------------------------------------- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
-- Dipl.-Inf. (Univ.) Henning P. Schmiedehausen INTERMETA GmbH [EMAIL PROTECTED] +49 9131 50 654 0 http://www.intermeta.de/
RedHat Certified Engineer -- Jakarta Turbine Development -- hero for hire
Linux, Java, perl, Solaris -- Consulting, Training, Development
"Fighting for one's political stand is an honorable action, but re- fusing to acknowledge that there might be weaknesses in one's position - in order to identify them so that they can be remedied - is a large enough problem with the Open Source movement that it deserves to be on this list of the top five problems." -- Michelle Levesque, "Fundamental Issues with Open Source Software Development"
--------------------------------------------------------------------- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
--------------------------------------------------------------------- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
