On Thursday, March 10, 2016 10:44 PM, [email protected] wrote:
>
> We'll have to agree to disagree on that one. If you are concerned
> about a performance issue then you need to know where to look to
> enable debug logging. A profiler will tell you where to look and
> at that point you don't need the debug logging.
Well, first of all, I found this without knowing where to look. I just checked
the thread dump, looked closer at the stack traces involving the
ClassNotFoundExceptions, and found the try catch clause. If there would have
been a log.debug(...) or log.trace(...) statement there, then I would know how
to enable debug logging for this. All without knowing beforehand where to look,
and all without anybody telling me where to look.
Secondly, not all problems are easily reproducible in a
development/test/staging environment, especially if one depends on the nature
of the production traffic to find all (or at least most) places in the code
that experience problems. Not all organizations have the time and/or the money
to replay/simulate live traffic in another environment. And I suspect that most
IT departments and/or service providers would react with some suspicion or
worry if one would try and get them to introduce a profiler in production, even
though these tools have become more and more production friendly over the years.
Thirdly, even if profiling would be an option, if no profiling is setup already
then it would take some time and money to do that. Compared to the trivial
change of logging level for a specific class. And I can't really see how
log.debug(...) or log.trace(...) (possibly surrounded by a
log.isDebugEnabled() or isTraceEnabled())would have any negative effect for
anyone.
Just my two cents...
> The code in question is in the JSP API JAR and there is no
> configuration API available. The only option would be
> a system property which would make it a global configuration option.
I'm not sure I understand why the absence of a "configuration API" would stop
you from simply checking for a specific init parameter in the servlet context
(thus making it possible to configure it in the web.xml using a context-param).
And when it comes to per jsp configuration, one could check for a page scoped
attribute with the same name. Ideally this could be added as a new page
directive attribute (like <%@ page disableELImports="true" %>), but I guess
that would constitute a "configuration API" change that would have to be in the
specification. But the init parameter or page scoped attribute could be fine
interim solutions, until the specification is updated to add these
configuration options the proper way.
> Um, no. See above. This would have to be a global option. It would work for
> some users/pages but not for others.
Um, yes, see above. ;)
And why would it not work for some users? I'm not saying it would magically
solve the problem for everyone. But it would solve the worst problems for most
people, and it wouldn't make it worse for anyone (ie, the fact that one would
have the *option* to turn it off, doesn't make the situation worse).
> Generally, the suggestions are reasonable. The control can't be as
> fine-grained as you suggest but that isn't necessarily a show-stopper.
Well, the solution with the page scoped attribute in the jsp page would make it
fine grained enough for most people, don't you think? :)
I would also be interested in hearing your thoughts about having an option to
disable this class lookup for all names that doesn't start with a capital
letter. Thus making ${Boolean.TRUE} work as normal, while skipping ${foo.bar},
even if they coexist in the same jsp page. I actually would think that would be
the best solution to this, since the naming convention dictates that you should
start class names with capital letter, and variables with lower case letters.
Basically, the code could look something like this, in
ScopedAttributeELResolver.java:
if (importHandler != null) {
if (resolveClass) {
//disableELImportsForNamesNotStartingWithUpperCase is false by
default,
//and can be configured using for example a web.xml init
parameter
if (disableELImportsForNamesNotStartingWithUpperCase &&
!Character.isUpperCase(key.charAt(0))) {
resolveClass = false;
}
}
if (resolveClass) {
...
}
...
> The main disadvantage that these options have is that a
> better solution is available. Follow the bug report from
> my previous message in this thread for details.
> [...]
> In this case, I believe the root cause has been fixed.
That sounds great! And I saw now in the bug report page that your simple test
case indicated a ~1ms performance gain per avoided ClassNotFoundException. That
sounds very close to my own tests, where the start page takes about 2500 ms,
and I counted about 2700 ClassNotFoundExceptions.
But, just a question. When looking at the code change in commit 1734419, it
seems to only handle the special case of ${foo} or ${empty foo} and similar.
But the problem would still remain for ${foo.bar}, right? So, technically, the
root cause is not fixed completely. We have lots of ${foo.bar} equivalents in
our code, where it is not uncommon for "foo" to be null. So I suspect that we
still would experience a big performance degradation on our sites, even with
this fix, compared to simply using Tomcat 7.
/Jimi
P.S. Having said all this. I hope you don't think I sound like an overcritical
fault-finding bastard. I just find it interesting do discuss this. :) And a
solution to this problem would mean that we didn't have to revert back to
Tomcat 7 for our sites. D.S.
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
