On 11/03/2016 14:17, jimi.hulleg...@svensktnaringsliv.se wrote:
> On Thursday, March 10, 2016 10:44 PM, ma...@apache.org 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.
And a debug log message is unlikely to tell you any more than the thread
dump did. If the same code keeps appearing in a thread dump it is a fair
bet that time is being spent in that code but you still need performance
data - which you can't get from debug logging or thread dumps - to be
certain.
> 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.
You still need to know what debug logging to turn on and if you are
guessing based on thread dumps then you already have everything debug
logging is likely to give you.
> 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.
Compared to the cost of the time to debug issues without a profiler, the
cost of a profiler is tiny. I use YourKit (because they give me a free
copy to use for my ASF work) and, while the price of it has gone up
quite a bit since I last looked a few years ago, it still (in my view)
offers a very quick return on investment.
As for using it in production, you aren't going to be using a profiler
unless you have a problem. I'd look at the risk (never seen an issue) of
using a profiler against the production system and the quality of data /
time to useful results vs debug logging and the longer to get results /
lower quality of data and choose the profiler every time. YMMV.
>> 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.
At the point where you'd need to access the ServletContext to read the
configuration, the ServletContext isn't available. The code in question
is in a spec JAR and we can't change the public API in any way, nor can
we add a dependency to some other component. That tends to leave system
properties as the only option.
Since I wrote the "configuration API" above, I did some more research
and found the route via the ELContext. That is what the implemented
solution uses. It should be possible to hook up your proposed
configuration option to pass data via that as long as the code is careful.
> 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).
Which is my point. The solution doesn't help those users that need
imports and good performance. Greater granularity in configuration helps
to a point but there will be users that need imports and good
performance on the same page and per expression configuration is taking
things a bit too far for my taste.
>> 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? :)
Per page is certainly better than global but I still have concerns. In
addition to the large pages needing imports and performance above, I'm
concerned that there is no good default for an option that disables
import support. Disabling imports by default will break pages that use
imports unless the developer adds the extra code. Not disabling them by
default means having to add code to every page with a performance problem.
My preference remains addressing the root cause of the performance issue
rather than trying to add configuration option to optionally disable a
spec feature because there is a performance issue.
> 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.
Such an approach would be fairly low down my list of preferred options.
Mainly because it depends on something outside of the developer's
control. If some third-party library opts not to follow the convention
for whatever reason (and I have seen this happen), the fix breaks. The
developer could work around it but it would get messy.
>> 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.
I've been doing some more digging. In a typical JSP page there are 4
failed lookups, each taking ~0.25ms (on my machine). Even in a working
case (Long.MAX_VALUE) there are still 3 failed lookups. Note not all of
that 0.25ms is caused by the CNFE but a large chunk of it is.
> 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.
Correct. The EL processing has no way (ignoring naming conventions) to
differentiate between ${ foo.bar } and ${ Long.MAX_VALUE }.
/me pauses to think
The bulk of the delay is caused by the CNFEs which in turn are caused by
having to look up the identifier in all of the imported packages (we
can't just take the first match because we need to check for conflicts).
There is a trick I used in the WebappClassloader to avoid CNFEs that
could work here as well.
I've committed that fix as well. My very rough performance testing
indicates ${foo.bar} is still slower than ${foo}. ${foo.bar} looks to
add somewhere between 0.1ms and 0.2ms. A lot better than the 1ms it was
but also still slower than ${foo}.
Since it is still faster to avoid the ImportHandler completely if
possible so I'll leave the previous fix in place.
I have some more ideas for further improvements but I don't want to add
unnecessary complexity. It would be good to get some idea of how well
the current code works with your app before going further.
It means building from source but with 8.0.x that should be fairly painless.
> /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.
Not at all. Civilised debate is a good thing. We don't have to agree
with each other in order to learn something from each other's point of
view or to get ideas on how to move things forward. Having this
discussion has sparked a number of ideas for improving the code which
I've already implemented and that can only be a good thing.
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscr...@tomcat.apache.org
For additional commands, e-mail: users-h...@tomcat.apache.org
