Sorry not to be around so much any more. I got laid off and had to find a
new job.
I'm so sorry to say that my memory leak patch has a big flaw in it. It
should not have caused the tag to be improperly reused.
I'm happy to say that I have a fix (replacement), which is attached. This
patch should work without the fix to Maven.
I didn't like the tag cache for two reasons: (1) it practically guarantees
leaks for applications that use the current jelly and aren't expecting it
and (2) I thought I had a good solution, which has now taken much longer to
implement than I expected.
Paul, you probably know, but you can't disable caching in the current code,
because some kinds of tags rely on it. This is a big issue with Jelly.
However, in the next version (should we actually get there :), I agree that
the whole life cycle should be separated.
-----Original Message-----
From: Paul Libbrecht [mailto:[EMAIL PROTECTED]
Sent: Sunday, May 01, 2005 2:32 PM
To: Jakarta Commons Developers List
Subject: Re: [jelly] SOLVED: Maven issue with Hans memory leak patch
Le 1 mai 05, � 19:36, peter royal a �crit :
> On May 1, 2005, at 9:09 AM, Brett Porter wrote:
>> I've solved this problem. One of the jelly tags was relying on a new
>> instance being created, but the instance is now being reused. Note
>> that
>> the tag is the same call from the same script, so I assume that this
>> is
>> valid, so I have made the tag not be stateful and it fixed the Maven
>> problem.
>> Does anyone know if this is correct behaviour?
> Sounds correct to me :)
Yes, this sounds also correct to me...
I think the assumption that a new instance of tag-objects be created
was correct if one would set not to "cache" tags (using the formerly
available setCacheTags method); this was probably the default.
SInce the thread-local fixes, all tags are cached and...
I am wondering wether there would be wish to bring back the method
setCacheTags and actually disable caching in some circumstances. I
would rather say it's better to keep it out as it makes life-cycle
easier to write for tags (and one can still clear the contexts).
Brett, I'm really looking forward to your commit (which will include
Hans' patch, then, correct?).
From there on we should hunt for 1.0!
paul
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
Index: src/java/org/apache/commons/jelly/JellyContext.java
===================================================================
RCS file:
/home/cvs/jakarta-commons/jelly/src/java/org/apache/commons/jelly/JellyContext.java,v
retrieving revision 1.66
diff -u -r1.66 JellyContext.java
--- src/java/org/apache/commons/jelly/JellyContext.java 27 Jan 2005 05:52:27
-0000 1.66
+++ src/java/org/apache/commons/jelly/JellyContext.java 21 May 2005 05:40:54
-0000
@@ -92,20 +92,6 @@
/** Should we export tag libraries to our parents context */
private boolean exportLibraries = true;
- /** Maps a Thread to its local Script data cache. It's
- * like a ThreadLocal, but it reclaims memory better
- * when the JellyCointext goes out of scope.
- * This isn't a ThreadLocal because of the typical usage scenario of
- * JellyContext. ThreadLocal is meant to be sued as a static variable,
- * we were using it as a local variable.
- * [EMAIL PROTECTED] #setThreadLocalScriptData(Script,Object)}
- */
- private Map threadLocalScriptData = Collections.synchronizedMap(new
WeakHashMap());
- // THINKME: Script objects are like Object (for equals and hashCode) I
think.
- // It should be asy to optimize hash-map distribution, e.g. by
- // shifting the hashcode return value (presuming Object.hashcode()
- // is something like an address)
-
/**
* Create a new context with the currentURL set to the rootURL
*/
@@ -390,93 +376,20 @@
return createChildContext();
}
-
- /** Gets the Script data item that may have previously been stored
- * by the script, in this context, for the current thread.
- *
- * @return the tag associated with the current context and thread
- */
- public Object getThreadScriptData(Script script) {
- if( script == null )
- return null;
- Tag tag = (Tag) getThreadScriptDataMap().get(script);
- if( tag == null && getParent() != null) {
- return getParent().getThreadScriptData(script);
- } else {
- return tag;
- }
- }
-
- /** Gets a per-thread (thread local) Map of data for use by
- * Scripts.
- * @return the thread local Map of Script data */
- public Map getThreadScriptDataMap() {
- Map rv;
- Thread t = Thread.currentThread();
- Map data = (Map) threadLocalScriptData.get(t);
- if (data == null) {
- rv = new HashMap();
- threadLocalScriptData.put(t, rv);
- } else {
- rv = data;
- }
- return rv;
- }
-
- /** Stores an object that lasts for the life of this context
- * and is local to the current thread. This method is
- * mainly intended to store Tag instances. However, any
- * Script that wants to cache data can use this
- * method.
- */
- public void setThreadScriptData(Script script, Object data) {
- getThreadScriptDataMap().put(script,data);
- }
-
- /** Clears variables set by Tags (basically, variables set in a Jelly
script)
- * and data stored by [EMAIL PROTECTED] Script} instances.
+ /** Clears variables set by Tags.
* @see #clearVariables()
- * @see #clearThreadScriptData()
- * @see #clearScriptData()
*/
public void clear() {
- clearScriptData();
clearVariables();
}
/** Clears variables set by Tags (variables set while running a Jelly
script)
* @see #clear()
- * @see #clearThreadScriptData()
- * @see #clearScriptData()
*/
- public void clearVariables() {
+ protected void clearVariables() {
variables.clear();
}
- /** Clears data cached by [EMAIL PROTECTED] Script} instances,
- * for this context, <strong>for the current thread</strong>.
- * The data cleared could be cached Tag instances or other data
- * saved by Script classes.
- * @see #clear()
- * @see #clearVariables()
- * @see #clearScriptData()
- */
- public void clearThreadScriptData() {
- getThreadScriptDataMap().clear();
- }
-
- /** Clears data cached by [EMAIL PROTECTED] Script} instances,
- * for this context, <strong>for all threads</strong>.
- * The data cleared could be cached Tag instances or other data
- * saved by Script classes.
- * @see #clear()
- * @see #clearThreadScriptData()
- * @see #clearVariables()
- */
- public void clearScriptData() {
- threadLocalScriptData.clear();
- }
-
/** Registers the given tag library against the given namespace URI.
* This should be called before the parser is used.
*/
Index: src/java/org/apache/commons/jelly/impl/TagScript.java
===================================================================
RCS file:
/home/cvs/jakarta-commons/jelly/src/java/org/apache/commons/jelly/impl/TagScript.java,v
retrieving revision 1.50
diff -u -r1.50 TagScript.java
--- src/java/org/apache/commons/jelly/impl/TagScript.java 20 Jan 2005
05:18:56 -0000 1.50
+++ src/java/org/apache/commons/jelly/impl/TagScript.java 21 May 2005
05:40:55 -0000
@@ -19,9 +19,12 @@
import java.lang.reflect.InvocationTargetException;
import java.net.MalformedURLException;
import java.net.URL;
+import java.util.Collections;
+import java.util.HashMap;
import java.util.Hashtable;
import java.util.Iterator;
import java.util.Map;
+import java.util.WeakHashMap;
import org.apache.commons.beanutils.ConvertingWrapDynaBean;
import org.apache.commons.beanutils.ConvertUtils;
@@ -104,6 +107,10 @@
/** the url of the script when parsed */
private URL scriptURL = null;
+
+ /** A synchronized WeakHashMap from the current Thread (key) to a Tag
object (value).
+ */
+ private Map threadLocalTagCache = Collections.synchronizedMap(new
WeakHashMap());
/**
* @return a new TagScript based on whether
@@ -286,11 +293,11 @@
* @return the tag to be evaluated, creating it lazily if required.
*/
public Tag getTag(JellyContext context) throws JellyException {
- Tag tag = (Tag) context.getThreadScriptData(this);
+ Tag tag = getTagForThread();
if ( tag == null ) {
tag = createTag();
if ( tag != null ) {
- context.setThreadScriptData(this,tag);
+ setTagForThread(tag);
configureTag(tag,context);
}
}
@@ -514,7 +521,7 @@
* when a StaticTag is switched with a DynamicTag
*/
protected void setTag(Tag tag, JellyContext context) {
- context.setThreadScriptData(this,tag);
+ this.setTagForThread(tag);
}
/**
@@ -681,4 +688,24 @@
throw new JellyTagException(e, fileName, elementName, columnNumber,
lineNumber);
}
+
+ private void setTagForThread(Tag t) {
+ Thread thread = Thread.currentThread();
+ Map tagCache = (Map) threadLocalTagCache.get(thread);
+ if (tagCache == null) {
+ tagCache = new HashMap();
+ threadLocalTagCache.put(thread,tagCache);
+ }
+
+ tagCache.put(this,t);
+ }
+
+ private Tag getTagForThread() {
+ Map tagCache = (Map) threadLocalTagCache.get(Thread.currentThread());
+ if (tagCache == null) {
+ return null;
+ }
+
+ return (Tag) tagCache.get(this);
+ }
}
Index: src/test/org/apache/commons/jelly/core/BaseMemoryLeakTest.java
===================================================================
RCS file:
/home/cvs/jakarta-commons/jelly/src/test/org/apache/commons/jelly/core/BaseMemoryLeakTest.java,v
retrieving revision 1.3
diff -u -r1.3 BaseMemoryLeakTest.java
--- src/test/org/apache/commons/jelly/core/BaseMemoryLeakTest.java 20 Jan
2005 05:19:04 -0000 1.3
+++ src/test/org/apache/commons/jelly/core/BaseMemoryLeakTest.java 21 May
2005 05:40:55 -0000
@@ -130,7 +130,6 @@
rt.runFinalization();
rt.gc();
long middle = rt.totalMemory() - rt.freeMemory();
- log.info("TagHolderMap has " +
jc.getThreadScriptDataMap().size() + " entries.");
log.info("Memory test after " + i + " runs: "
+ (middle - start));
}
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
