I think the problem with the weak ref code was that scripts would get garbage collected, and then attempts to run them would fail.
Do you think there is a way around this? Thread safety and the current code breakage are my worries with the current code. On Sun, 09 Jan 2005 22:57:12 -0500, Hans Gilde <[EMAIL PROTECTED]> wrote: > 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] > > -- http://www.multitask.com.au/people/dion/ --------------------------------------------------------------------- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
