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 <br...@tribenetwork.com> on 2004/08/24 19:53:41 UTC
Re: Class cache in JavaBaseFactory...
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:
>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