On Friday, March 11, 2016 6:07 PM, ma...@apache.org wrote:
>
> And a debug log message is unlikely to tell you any more than the thread dump
> did.
That depends on what is actually being logged. If the class name is printed,
then one could immediately figure out the name of the EL variable (like
"java.lang.myTempValue", then the EL parameter name is "myTempValue").
And the thread dump only gives me a snapshot, containing 0, 1 or more
stacktraces with the ClassNotFoundException. Unless I take a threaddump every
millisecond (or even more often), the thread dump method would miss some
stacktraces. With logging, it would not miss any.
> 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.
I'm wasn't talking about gathering information regarding performance. I was
talking about gathering information about what jsp/tag code and what EL
variable names are responsible for this, and how often it happens. In order to
get a grasp of if it is feasible to solve by fixing the code. For example, if
this log output showed me that 90% of the ClassNotFoundExceptions happen
because of a handful of jsp/tag files, then I would just fix these and see how
much that helped. But if the log output would indicate that this problem is
spread out over too many jsp and tag files, then I wouldn't even try and fix it
in the code, but would instead try some other approach (like disabling this
feature, or reverting back to Tomcat 7).
> 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.
Well, I guess you have a point. But the fact is that we don't have any profiler
tool installed in production right now (and I suspect that is a quite common
scenario for many users in general), and having one installed is not something
that is done quickly. Enabling trace or debug logging in a specific class takes
less than a minute, giving me much needed information more or less instantly.
But yes, if one would have followed common sense, and already had a profiler
installed (and have used it enough to know how to get it to output the needed
information), then you would be right ;)
> At the point where you'd need to access the ServletContext to read the
> configuration, the ServletContext isn't available.
> [...]
> 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.
So, just to check if I understood you correctly... You *can* get access to the
ServletContext, right? (Using page.getServletContext()...)
> It should be possible to hook up your proposed configuration option
> to pass data via that as long as the code is careful.
OK. Sounds promising. Fingers crossed. :)
> 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.
I'm sorry, but I don't follow you now. Are you saying that since there are
people who wouldn't benefit from this (but not get hurt either), there is no
reason to fix it for those that *would* benefit from it?
> 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.
Well, I never said that it would solve the problem for everyone. But for people
who don't use any EL imports at all (like everyone migrating from Tomcat 7),
they would benefit from a global *off* option, and then they can systematically
enable this feature slowly, page by page, as they add functionality that
depends on it.
As for people who have pages that need both imports and performance, I still
claim that this fix wouldn't make it any *worse* for them. In fact, I would
claim that it would make it somewhat better, because now they would have an
option to control what pages should have this feature, and if they have
important pages that doesn't use imports then they at least could reduce the
performance problem for those pages. And regarding the pages with both a need
for both imports and performance, well they still have the workaround that was
mentioned in the beginning of this thread, adding a specific scope to all the
EL variables (at least the ones that can be null or not set).
In my ears, it sounds almost like you think that having an extra option would
be *bad* for some users, and I can't quite understand that reasoning. :)
> 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.
Yes, of course. All my thoughts and ideas about configuration options to
disable the import feature completely or partly are all based on the assumption
that the main problem isn't solved. If the main problem is solved, then all is
good. :)
>> 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.
>
> 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.
Well, I have the same pragmatic view on this as above. It is an added option,
the developer doesn't have to use it. There can be cases where the developer
knows that such a problem can't happen, simply because no 3rd party jsp or tag
code is used in the project. Or the developer might not know it, but performs
enough testing to conclude that "all that important on the site is still
working, and now the performance is great!". It is for these people the fix is
for. If someone is uncertain about the implications, or actually notice big
problems after enabling the feature, then they can simply ignore/remove that
configuration option.
In terms of documentation, you could simply state that this configuration is
intended for "Experts", and that it can cause the problems you mention.
I mean... come on. There must be loads of advanced configuration options in
Tomcat that can cause problems if they are set without really knowing what one
is doing. Right? :)
> I've been doing some more digging. In a typical JSP page there are 4 failed
> lookups, each taking ~0.25ms (on my machine).
Well, the estimated number of failures for a typical jsp may be interesting to
know, but it is the combination all jsp pages (and tag pages) for a request
that is the more interesting thing to consider. I think that many sites use
quite a few different jsp and tag files, and they in turn include each other,
so that a single request can result in hundreds or even thousands of jsps/tags
being called. The code for our site certainly works this way. Like I said, a
single request to the start page triggered 2700 CNFEs in my local machine.
> Even in a working case (Long.MAX_VALUE) there are still 3 failed lookups.
Interesting. It got me thinking... I just realized that this feature (or the
implementation of it, actually) in a way breaks the general java coding
convention of not using exceptions for cases that doesn't really correspond to
an *exception* to the normal flow. I'm not saying that there is a way around
that, I just find it a bit interesting. Maybe the fault is with the Java API,
since they decided to have the ClassLoader throw a CNFE instead of returning
null. ;)
>we can't just take the first match because we need to check for conflicts
Should this really be happening in production, if the code has been tested in
development, test and stage environments? Maybe one could handle this like an
assertion, i.e. only doing the collision check unless in "production mode".
That would give the correct error when developing and testing, but would assume
that it "can't" happen in production (just like assertions) and thus increase
performance. Just a thought...
> 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}.
Interesting indeed! But, I tried to look at that commit but couldn't find it.
Is it not in trunk?
> 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.
I can definitely do that. But now it is Friday evening here, and I'm going
home. :)
Hopefully I have time to test this on Monday.
> 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.
I couldn't agree more. :)
Now I hope you have a nice weekend, Mark!
Regards
/Jimi
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscr...@tomcat.apache.org
For additional commands, e-mail: users-h...@tomcat.apache.org
