You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@turbine.apache.org by Brian Lawler <bp...@pacbell.net> on 2004/04/21 03:10:20 UTC
Class cache in JavaBaseFactory...
Hey guys-
We were profiling our turbine application the other day and one thing
that bubbled up to the top was the JavaBaseFactory.getAssembler()
class. This class iterates over all of the packages where Assemblers
may be found and retrieves a Class instance when it encounters a hit.
But it does this for every call to the getAssembler method, and we
found the resulting time spent in Class.forName() to be very high. I
have placed a simple synchronized map in our version of the class that
is used to cache previously found Assemblers and is queried before
going through the package loop. This lowered the time we spend in
Class.forName() substantially.
First of all, is this not a good thing to do?
And second, if this *is* a good thing to do, here is the patch if you
are interested...
Index:
src/java/org/apache/turbine/services/assemblerbroker/util/java/
JavaBaseFactory.java
===================================================================
RCS file:
/home/cvsroot/turbine/src/java/org/apache/turbine/services/
assemblerbroker/util/java/JavaBaseFactory.java,v
retrieving revision 1.1.1.2
diff -u -r1.1.1.2 JavaBaseFactory.java
---
src/java/org/apache/turbine/services/assemblerbroker/util/java/
JavaBaseFactory.java 20 Apr 2004 23:59:23 -0000 1.1.1.2
+++
src/java/org/apache/turbine/services/assemblerbroker/util/java/
JavaBaseFactory.java 21 Apr 2004 00:06:03 -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;
@@ -47,6 +50,9 @@
/** Logging */
protected Log log = LogFactory.getLog(this.getClass());
+ /** A cache for previously obtained assemblers */
+ private Map assemblerCache = Collections.synchronizedMap(new
HashMap());
+
static
{
ObjectUtils.addOnce(packages, GenericLoader.getBasePackage());
@@ -67,7 +73,8 @@
if (StringUtils.isNotEmpty(name))
{
- for (Iterator it = packages.iterator(); it.hasNext();)
+ assembler = (Assembler) assemblerCache.get(name);
+ for (Iterator it = packages.iterator(); assembler == null
&& it.hasNext();)
{
StringBuffer className = new StringBuffer();
@@ -83,6 +90,7 @@
{
Class servClass =
Class.forName(className.toString());
assembler = (Assembler) servClass.newInstance();
+ assemblerCache.put(name, assembler);
break; // for()
}
catch (ClassNotFoundException cnfe)
---------------------------------------------------------------------
To unsubscribe, e-mail: turbine-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: turbine-dev-help@jakarta.apache.org
Re: Class cache in JavaBaseFactory...
Posted by "Henning P. Schmiedehausen" <hp...@intermeta.de>.
Brian Lawler <br...@tribenetwork.com> writes:
>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
Well, why not? ;-) That is, what RCs are for. We discussed it and the
patch looks not very intrusive to me. As we don't use different class
loaders and even parallel threads would generate the same type of
objects, I see no reason why not to apply it.
>- for (Iterator it = packages.iterator(); it.hasNext();)
>+ for (Iterator it = packages.iterator(); assembler == null
>&& it.hasNext();)
[...]
> assembler = (Assembler) servClass.newInstance();
> break; // for()
> }
You don't trust the break, do you? ;-)
Regards
Henning
--
Dipl.-Inf. (Univ.) Henning P. Schmiedehausen INTERMETA GmbH
hps@intermeta.de +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: turbine-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: turbine-dev-help@jakarta.apache.org
Re: Class cache in JavaBaseFactory...
Posted by Brian Lawler <br...@tribenetwork.com>.
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>kimptoc.mail@bigfoot.com</email>
</contributor>
<contributor>
+ <name>Brian Lawler</name>
+ <email>brian@tribenetwork.com</email>
+ </contributor>
+ <contributor>
<name>Josh Lucas</name>
<email>josh@stonecottage.com</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 <br...@tribenetwork.com> 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 <br...@tribenetwork.com> 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:leon@opticode.co.za">Leon
>>>> Messerschmidt</a>
>>>> - * @author <a href="mailto:hps@intermeta.de">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: turbine-dev-unsubscribe@jakarta.apache.org
>>>>> For additional commands, e-mail:
>>>>> turbine-dev-help@jakarta.apache.org
>>>>>
>>>
>>>
>>>> --------------------------------------------------------------------
>>>> -
>>>> To unsubscribe, e-mail: turbine-dev-unsubscribe@jakarta.apache.org
>>>> For additional commands, e-mail: turbine-dev-help@jakarta.apache.org
>>>
>>> --
>>> Dipl.-Inf. (Univ.) Henning P. Schmiedehausen INTERMETA GmbH
>>> hps@intermeta.de +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: turbine-dev-unsubscribe@jakarta.apache.org
>>> For additional commands, e-mail: turbine-dev-help@jakarta.apache.org
>>>
>
>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: turbine-dev-unsubscribe@jakarta.apache.org
>> For additional commands, e-mail: turbine-dev-help@jakarta.apache.org
>
> --
> Dipl.-Inf. (Univ.) Henning P. Schmiedehausen INTERMETA GmbH
> hps@intermeta.de +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: turbine-dev-unsubscribe@jakarta.apache.org
> For additional commands, e-mail: turbine-dev-help@jakarta.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: turbine-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: turbine-dev-help@jakarta.apache.org
Re: Class cache in JavaBaseFactory...
Posted by "Henning P. Schmiedehausen" <hp...@intermeta.de>.
Brian Lawler <br...@tribenetwork.com> 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 <br...@tribenetwork.com> 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:leon@opticode.co.za">Leon
>>> Messerschmidt</a>
>>> - * @author <a href="mailto:hps@intermeta.de">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: turbine-dev-unsubscribe@jakarta.apache.org
>>>> For additional commands, e-mail: turbine-dev-help@jakarta.apache.org
>>>>
>>
>>
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: turbine-dev-unsubscribe@jakarta.apache.org
>>> For additional commands, e-mail: turbine-dev-help@jakarta.apache.org
>>
>> --
>> Dipl.-Inf. (Univ.) Henning P. Schmiedehausen INTERMETA GmbH
>> hps@intermeta.de +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: turbine-dev-unsubscribe@jakarta.apache.org
>> For additional commands, e-mail: turbine-dev-help@jakarta.apache.org
>>
>---------------------------------------------------------------------
>To unsubscribe, e-mail: turbine-dev-unsubscribe@jakarta.apache.org
>For additional commands, e-mail: turbine-dev-help@jakarta.apache.org
--
Dipl.-Inf. (Univ.) Henning P. Schmiedehausen INTERMETA GmbH
hps@intermeta.de +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: turbine-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: turbine-dev-help@jakarta.apache.org
Re: Class cache in JavaBaseFactory...
Posted by Brian Lawler <br...@tribenetwork.com>.
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.
-B
On Jul 8, 2004, at 2:52 AM, Henning P. Schmiedehausen wrote:
> Brian Lawler <br...@tribenetwork.com> 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:leon@opticode.co.za">Leon
>> Messerschmidt</a>
>> - * @author <a href="mailto:hps@intermeta.de">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: turbine-dev-unsubscribe@jakarta.apache.org
>>> For additional commands, e-mail: turbine-dev-help@jakarta.apache.org
>>>
>
>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: turbine-dev-unsubscribe@jakarta.apache.org
>> For additional commands, e-mail: turbine-dev-help@jakarta.apache.org
>
> --
> Dipl.-Inf. (Univ.) Henning P. Schmiedehausen INTERMETA GmbH
> hps@intermeta.de +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: turbine-dev-unsubscribe@jakarta.apache.org
> For additional commands, e-mail: turbine-dev-help@jakarta.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: turbine-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: turbine-dev-help@jakarta.apache.org
Re: Class cache in JavaBaseFactory...
Posted by "Henning P. Schmiedehausen" <hp...@intermeta.de>.
Brian Lawler <br...@tribenetwork.com> 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:leon@opticode.co.za">Leon Messerschmidt</a>
>- * @author <a href="mailto:hps@intermeta.de">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: turbine-dev-unsubscribe@jakarta.apache.org
>> For additional commands, e-mail: turbine-dev-help@jakarta.apache.org
>>
>---------------------------------------------------------------------
>To unsubscribe, e-mail: turbine-dev-unsubscribe@jakarta.apache.org
>For additional commands, e-mail: turbine-dev-help@jakarta.apache.org
--
Dipl.-Inf. (Univ.) Henning P. Schmiedehausen INTERMETA GmbH
hps@intermeta.de +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: turbine-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: turbine-dev-help@jakarta.apache.org
Re: Class cache in JavaBaseFactory...
Posted by Brian Lawler <br...@tribenetwork.com>.
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.
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:leon@opticode.co.za">Leon Messerschmidt</a>
- * @author <a href="mailto:hps@intermeta.de">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: turbine-dev-unsubscribe@jakarta.apache.org
> For additional commands, e-mail: turbine-dev-help@jakarta.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: turbine-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: turbine-dev-help@jakarta.apache.org
Re: Class cache in JavaBaseFactory...
Posted by Brian Lawler <br...@tribenetwork.com>.
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: turbine-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: turbine-dev-help@jakarta.apache.org
Re: Class cache in JavaBaseFactory...
Posted by "Henning P. Schmiedehausen" <hp...@intermeta.de>.
Brian Lawler <bp...@pacbell.net> writes:
>Hey guys-
>We were profiling our turbine application the other day and one thing
>that bubbled up to the top was the JavaBaseFactory.getAssembler()
>class. This class iterates over all of the packages where Assemblers
>may be found and retrieves a Class instance when it encounters a hit.
>But it does this for every call to the getAssembler method, and we
>found the resulting time spent in Class.forName() to be very high. I
>have placed a simple synchronized map in our version of the class that
>is used to cache previously found Assemblers and is queried before
>going through the package loop. This lowered the time we spend in
>Class.forName() substantially.
>First of all, is this not a good thing to do?
Well, why not?
>And second, if this *is* a good thing to do, here is the patch if you
>are interested...
Did you try running a patched Turbine? Did it make any difference in
the loading process of the classes? I'm a little concerned about the
long-term life of these maps and their member objects. Currently, the
assemblers are created anew whenever one is required. With your patch,
there might be side effects with internal state that is now kept
because the objects are not created every time but kept around in a
table. I'm also concerned that these Factories, while being
instantiated are just one object (in the AssemblerBrokerService) for
the whole application with all the parallel running sessions. ATM,
each session instantiates its own Assembler whenever it needs one. Now
there is only one Assembler object per match for all the sessions
being pulled from a cache. This looks like a point where debugging
(having exactly one session which does one thing at a time) and real
life (having lots of sessions doing different things) don't match.
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.
Regards
Henning
>Index:
>src/java/org/apache/turbine/services/assemblerbroker/util/java/
>JavaBaseFactory.java
>===================================================================
>RCS file:
>/home/cvsroot/turbine/src/java/org/apache/turbine/services/
>assemblerbroker/util/java/JavaBaseFactory.java,v
>retrieving revision 1.1.1.2
>diff -u -r1.1.1.2 JavaBaseFactory.java
>---
>src/java/org/apache/turbine/services/assemblerbroker/util/java/
>JavaBaseFactory.java 20 Apr 2004 23:59:23 -0000 1.1.1.2
>+++
>src/java/org/apache/turbine/services/assemblerbroker/util/java/
>JavaBaseFactory.java 21 Apr 2004 00:06:03 -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;
>@@ -47,6 +50,9 @@
> /** Logging */
> protected Log log = LogFactory.getLog(this.getClass());
>+ /** A cache for previously obtained assemblers */
>+ private Map assemblerCache = Collections.synchronizedMap(new
>HashMap());
>+
> static
> {
> ObjectUtils.addOnce(packages, GenericLoader.getBasePackage());
>@@ -67,7 +73,8 @@
> if (StringUtils.isNotEmpty(name))
> {
>- for (Iterator it = packages.iterator(); it.hasNext();)
>+ assembler = (Assembler) assemblerCache.get(name);
>+ for (Iterator it = packages.iterator(); assembler == null
>&& it.hasNext();)
> {
> StringBuffer className = new StringBuffer();
>@@ -83,6 +90,7 @@
> {
> Class servClass =
>Class.forName(className.toString());
> assembler = (Assembler) servClass.newInstance();
>+ assemblerCache.put(name, assembler);
> break; // for()
> }
> catch (ClassNotFoundException cnfe)
>---------------------------------------------------------------------
>To unsubscribe, e-mail: turbine-dev-unsubscribe@jakarta.apache.org
>For additional commands, e-mail: turbine-dev-help@jakarta.apache.org
--
Dipl.-Inf. (Univ.) Henning P. Schmiedehausen INTERMETA GmbH
hps@intermeta.de +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
"Au�erdem k�nnen in Deutschland alle Englisch. [...] so entf�llt die
Notwendigkeit [...] Deutsch zu lernen."
-- Johan Micoud auf die Frage warum er kein Deutsch spricht.
(http://www.spiegel.de/spiegel/0,1518,273205,00.html)
---------------------------------------------------------------------
To unsubscribe, e-mail: turbine-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: turbine-dev-help@jakarta.apache.org