You are viewing a plain text version of this content. The canonical link for it is here.
Posted to taglibs-dev@jakarta.apache.org by Henri Yandell <fl...@gmail.com> on 2007/08/24 18:20:23 UTC
[standard] More bugs to resolve - thoughts?
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.
32311 - No Date caching. I can get 30% speed improvements here with
caching. I'm not convinced it's worth it. So WONTFIX.
All a bit of a shame, as I've a large patch with nothing hugely wrong
with it :) But I've no urge to do non-surgical things.
Any thoughts? Any +1s on closing these 3 issues?
Hen
---------------------------------------------------------------------
To unsubscribe, e-mail: taglibs-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: taglibs-dev-help@jakarta.apache.org
Re: [standard] More bugs to resolve - thoughts?
Posted by Kris Schneider <kr...@directthought.com>.
On 9/28/07, Henri Yandell <fl...@gmail.com> wrote:
> On 8/24/07, Kris Schneider <kr...@directthought.com> wrote:
> > On 8/24/07, Henri Yandell <fl...@gmail.com> 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 <ma...@directThought.com>
directThought <http://www.directThought.com/>
---------------------------------------------------------------------
To unsubscribe, e-mail: taglibs-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: taglibs-dev-help@jakarta.apache.org
Re: [standard] More bugs to resolve - thoughts?
Posted by Henri Yandell <fl...@gmail.com>.
On 8/24/07, Kris Schneider <kr...@directthought.com> wrote:
> On 8/24/07, Henri Yandell <fl...@gmail.com> 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?
Hen
---------------------------------------------------------------------
To unsubscribe, e-mail: taglibs-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: taglibs-dev-help@jakarta.apache.org
Re: [standard] More bugs to resolve - thoughts?
Posted by Kris Schneider <kr...@directthought.com>.
On 8/24/07, Henri Yandell <fl...@gmail.com> 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).
> 32311 - No Date caching. I can get 30% speed improvements here with
> caching. I'm not convinced it's worth it. So WONTFIX.
>
>
> All a bit of a shame, as I've a large patch with nothing hugely wrong
> with it :) But I've no urge to do non-surgical things.
>
>
> Any thoughts? Any +1s on closing these 3 issues?
>
>
> Hen
--
Kris Schneider <ma...@directThought.com>
directThought <http://www.directThought.com/>
---------------------------------------------------------------------
To unsubscribe, e-mail: taglibs-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: taglibs-dev-help@jakarta.apache.org