On 9/28/07, Henri Yandell <[EMAIL PROTECTED]> wrote: > On 8/24/07, Kris Schneider <[EMAIL PROTECTED]> wrote: > > On 8/24/07, Henri Yandell <[EMAIL PROTECTED]> wrote: > > > I've been churning on with my patch to add a caching SPI to Standard. > > > Here's the state, and what I think we should do: > > > > > > 17700 - Ostensibly this is a complaint that there is no caching in the > > > i18n stuff. When you dig deeper, it doesn't make sense as the poster > > > is complaining that the Resources class that handles errors for > > > Standard is the one that needs caching, and he refers to > > > ResourceMessages which does not exist. Too confusing, so WONTFIX. > > > > > > 31789 - Memory leak in ELEvaluator. This is the big issue - EL bits > > > are never GC'd it seems and it grows and grows until things OOM. This > > > is because caching is done. I've not tested this, though all I'm > > > adding is the ability to plug your own cache in, or choose between > > > forever caching or no caching. > > > > > > It's open source. If someone feels this problem strongly enough, it's > > > not hard to go in and change it so it uses a LRUCache or something. > > > So WONTFIX. > > > > At one point, there was a fairly healthy discussion about this: > > > > http://marc.info/?t=109820705200001 > > > > Unfortunately, it seemed to just die rather abruptly. Since we're > > focusing on JSTL 1.1, I'd want to go back and review the code to see > > what's what. Based on my schedule, I won't be able to do that for > > another week. > > > > At the very least, I think we should expose ELEvaluator.mBypassCache > > via configuration and then actually honor it if it's set to false. I > > believe the current code skips the cache lookup when it's false but > > still caches the evaluation results - that seems like a bug (except > > that the code comments seem to imply that was the desired behavior). > > What do you think to the latest commit Kris? Is Justyna's unreleased > 1.0.x fix good enough for us to use for 1.1.3?
I think it was mentioned elsewhere, but it might be worthwhile to pull in the latest collections code instead of just using what was checked-in for 1.0.x (if that's what happened). Otherwise it seems fine. The only thing that nags at me is the potential for this to be a concurrency bottleneck. As with the "old" solution, the cache is wrapped with Collections.synchronizedMap which means that get/put share the same lock. I'm not aware of an LRU cache based on the JSR 166 (util-concurrent) backport, but something like that would probably be better. At worst, this isn't any different from the "old" solution... Now that I've got concurrency on the brain, I'm not sure whether the "new" solution is actually thread-safe: Should setBypassCache be synchronized? Hm, that's actually a new method, right? Previously, sCachedExpressionStrings was created upfront and mBypassCache was forced to be a constructor arg... > Hen -- Kris Schneider <mailto:[EMAIL PROTECTED]> directThought <http://www.directThought.com/> --------------------------------------------------------------------- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
