Ok, got it. I hadn't looked at the file histories. My big question is this:
If I can fix the problem with the original WeakRef stuff this week, would we be interested in using it? I came up with the solution because it doesn't involve a change to the API, is that still a goal? Also, it preserves the guarantee of thread safety with respect to tag implementations. Or have we moved beyond the WeakRef thing, to where we're OK with making a possibly temporary change to the API? And few items about the JellyContext caching implementation: * This solution was originally proposed by someone else (can't remember who). That proposal had a method "setScriptData(Script s, Object data)" rather than "setTagForScript". I prefer these names. This way doesn't bind the JellyContext as tightly to the TagScript class, and the same cache can be used by other Script implementations. * Also, we rejected this idea originally because it would change the API. If we choose to adopt it, we should find the original posts (from October-November of last year) and let that person know. * This solution removes the guarantee of thread safety for tag implementations. I'm OK with this personally, but we should think about modifying the Thread TL to ensure that a new context is used for each thread. It will have to clear the tag cache as well. Or, we could store the Script data HashMap in a ThreadLocal. * There's no point to making the tagHolderMap a WeakHashMap. The references to Tags are hard and each Tag references its body Script. Then, the child Tags are given a reference to their parent Tag, causing a circular reference that prevents GC if you don't explicitly clear it. This is what was causing the original memory leak with ThreadLocal (which uses a WeakHashMap internally). * You have a method "getTagHolderMap" (could be "getScriptDataMap"), but it's not used. It should be used instead of the direct reference to the tagHolderMap variable to make inheritance easier. * I think that there should be a separate method clearScriptData (leaving clear() for its original use) Hans -----Original Message----- From: Paul Libbrecht [mailto:[EMAIL PROTECTED] Sent: Sunday, January 09, 2005 5:27 PM To: Jakarta Commons Developers List Subject: Re: [jelly] WeakReference stuff in CVS? Le 9 janv. 05, à 23:12, Hans Gilde a écrit : > Hey all, I've been out of the loop for a couple of weeks, sorry. I'm > back > and I'm going to look at the problem with the WeakReference stuff. > > It looks like HEAD on cvs.apache.org isn't using the WeakReference > code. Has > it not been applied to CVS or does this have something to do with the > switch > to SVN? Hans, I've removed it since it was unused and was creating problems with some tags (notably defined ones). But the modifications applied also: - removed method getTag in favor of getTag(JellyContext) - removed setCacheTags (but it could be coming back if wished) - removed any ThreadLocal around Indeed, this is all committed though I could have, instead, proposed a patch. paul --------------------------------------------------------------------- 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]
