I vote that if the WeakRef thing can be fixed in a week or so, it should go into an RC2 instead of the caching in JellyContext. I vote this way because it will not change the API at all. I think that we're all agreed that the next major release will break many things out of the JellyContext, so I think that it's not prudent to add more stuff now.
I think that the idea behind the caching in JellyContext is the right way to go in the long run but I'm not sure that JellyContext is actually the right place to put it. Any opinions? Hans -----Original Message----- From: Hans Gilde [mailto:[EMAIL PROTECTED] Sent: Sunday, January 09, 2005 6:26 PM To: 'Jakarta Commons Developers List' Subject: RE: [jelly] WeakReference stuff in CVS? 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
