Another attempt at discussing the major performance issue in Tomcat 8 JSP

[sebdotv <se...@gmail.com>]

There have been quite a few attempts by others at notifying the issue, so
far without an acceptable answer. Here's yet another attempt.

With Tomcat 8 (as of 8.0.27), the following simple tag:

<%@ attribute name="attr1" required="true" %>
<%@ attribute name="attr2" required="false" %>
${attr1} ${attr2}

will result when called in 12 calls to loadClass, for every request, with a
dramatic performance impact:
3x java.lang.attr2
3x javax.servlet.attr2
3x javax.servlet.jsp.attr2
3x javax.servlet.http.attr2

The workaround is to scope the optional attribute reference, e.g.
${pageContext.attr2}, but this might not be acceptable depending on the
number of JSP/tags to update.

I believe the following points need to be addressed:
- what is so special with optional attributes, should they really be
considered undefined?
- if everything is conform with the spec, a more aggressive cache should
probably be implemented and used by ImportHandler

Related discussions/bugs:
- most of
https://bz.apache.org/bugzilla/buglist.cgi?bug_status=__all__&content=ImportHandler&no_redirect=1&order=Importance&product=Tomcat%208&query_format=specific
- http://markmail.org/message/5uolmdqgy6oemru5

Re: Another attempt at discussing the major performance issue in Tomcat 8 JSP

[Mark Thomas <ma...@apache.org>]

On 08/10/2015 14:24, sebdotv wrote:
> Mark, thanks for your quick and detailed answer.
> 
>> Hmm. With 9.0.x trunk there are 4 calls, not 12 to loadClass. One (not
>> three) for each package. I can't immediately think of any reason why
>> 8.0.x would make additional calls. Having checked, it doesn't. 8.0.x
>> makes 4 calls just like 9.0.x. I don't see anything that would have
>> changed this since 8.0.27.
> 
> You're right, I was counting recursive calls in the classloader. 1
> call is indeed made per package, so here 4 calls per undefined
> attribute.
> 
>> Regardless of whether there are 4 or 12 calls, I agree the performance
>> impact is going to be significant. The time is mostly spent constructing
>> the ClassNotFoundException. Annoyingly, this is then thrown away. As set
>> out in [1], there is a work-around but that comes with its own
>> performance impact and it is currently the view that we don't want to
>> slow down valid lookups to make invalid lookups faster.
> 
> Not that it matters much at this point, but I am not seeing so much of
> the time constructing the exception. A good part of the time is spent
> blocking on WebappClassLoader, unless ParallelWebappClassLoader is
> used. Then the loading of a class means searching for it everywhere on
> the classpath, which takes time (IO). Of course classloaders should
> not be used in vain, especially not repeatedly.

If you have a lot of pages like this then under load the blocking in the
class loader could start to dominate the delay. My comment about
exception construction was with a single-threaded test if I remember
correctly. It sounds as if parallel class loader may help at least a
little in your case.

>>> - if everything is conform with the spec, a more aggressive cache should
>>> probably be implemented and used by ImportHandler
>>
>> That is a lot easier said than done.
>> - A new ImportHandler instance is required for every page request
>> - Each page can have a different set of imports
>> - ImportHandler is spec class used by multiple web applications each of
>>   which will have a different set of classes available
>> - The cache needs to be thread safe
>> - The cache needs to be self-limiting in size
> 
> Not knowing anything about the internals of Tomcat, but what about
> something like a concurrent hashmap with weak keys (classloader +
> name) and values (class), used in ImportHandler.findClass? Do you
> foresee issues with something along these lines?

That is the sort of thing I had in mind as a static in ImportHandler.
Doing this and limiting the size of the cache could be tricky and -
since the maximum size of the cache depends on the application not user
input - you could argue there is no need for limiting the size.

Mark


---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@tomcat.apache.org
For additional commands, e-mail: users-help@tomcat.apache.org

Re: Another attempt at discussing the major performance issue in Tomcat 8 JSP

[sebdotv <se...@gmail.com>]

Mark, thanks for your quick and detailed answer.

> Hmm. With 9.0.x trunk there are 4 calls, not 12 to loadClass. One (not
> three) for each package. I can't immediately think of any reason why
> 8.0.x would make additional calls. Having checked, it doesn't. 8.0.x
> makes 4 calls just like 9.0.x. I don't see anything that would have
> changed this since 8.0.27.

You're right, I was counting recursive calls in the classloader. 1
call is indeed made per package, so here 4 calls per undefined
attribute.

> Regardless of whether there are 4 or 12 calls, I agree the performance
> impact is going to be significant. The time is mostly spent constructing
> the ClassNotFoundException. Annoyingly, this is then thrown away. As set
> out in [1], there is a work-around but that comes with its own
> performance impact and it is currently the view that we don't want to
> slow down valid lookups to make invalid lookups faster.

Not that it matters much at this point, but I am not seeing so much of
the time constructing the exception. A good part of the time is spent
blocking on WebappClassLoader, unless ParallelWebappClassLoader is
used. Then the loading of a class means searching for it everywhere on
the classpath, which takes time (IO). Of course classloaders should
not be used in vain, especially not repeatedly.

> > - if everything is conform with the spec, a more aggressive cache should
> > probably be implemented and used by ImportHandler
>
> That is a lot easier said than done.
> - A new ImportHandler instance is required for every page request
> - Each page can have a different set of imports
> - ImportHandler is spec class used by multiple web applications each of
>   which will have a different set of classes available
> - The cache needs to be thread safe
> - The cache needs to be self-limiting in size

Not knowing anything about the internals of Tomcat, but what about
something like a concurrent hashmap with weak keys (classloader +
name) and values (class), used in ImportHandler.findClass? Do you
foresee issues with something along these lines?

Sebastien

---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@tomcat.apache.org
For additional commands, e-mail: users-help@tomcat.apache.org

Re: Another attempt at discussing the major performance issue in Tomcat 8 JSP

[Mark Thomas <ma...@apache.org>]

On 08/10/2015 10:28, sebdotv wrote:
> There have been quite a few attempts by others at notifying the issue, so
> far without an acceptable answer. Here's yet another attempt.

Repeating the question because you didn't find the previous answer [1]
acceptable is unlikely to get you a different answer.

> With Tomcat 8 (as of 8.0.27), the following simple tag:
> 
> <%@ attribute name="attr1" required="true" %>
> <%@ attribute name="attr2" required="false" %>
> ${attr1} ${attr2}
> 
> will result when called in 12 calls to loadClass, for every request,

Hmm. With 9.0.x trunk there are 4 calls, not 12 to loadClass. One (not
three) for each package. I can't immediately think of any reason why
8.0.x would make additional calls. Having checked, it doesn't. 8.0.x
makes 4 calls just like 9.0.x. I don't see anything that would have
changed this since 8.0.27.

> with a dramatic performance impact:
> 3x java.lang.attr2
> 3x javax.servlet.attr2
> 3x javax.servlet.jsp.attr2
> 3x javax.servlet.http.attr2

Regardless of whether there are 4 or 12 calls, I agree the performance
impact is going to be significant. The time is mostly spent constructing
the ClassNotFoundException. Annoyingly, this is then thrown away. As set
out in [1], there is a work-around but that comes with its own
performance impact and it is currently the view that we don't want to
slow down valid lookups to make invalid lookups faster.

> The workaround is to scope the optional attribute reference, e.g.
> ${pageContext.attr2}, but this might not be acceptable depending on the
> number of JSP/tags to update.

As things currently stand, adding the scope is the cleanest solution. On
the plus side it doesn't add overhead anywhere else and there is no risk
of anything breaking. The downside is the time taken to add the scope.

> I believe the following points need to be addressed:
> - what is so special with optional attributes, should they really be
> considered undefined?

That is a good question. If there were something in the specs that let
us resolve these earlier, then the performance issues associated with
doing the class lookup would go away.

The initial signs are not good. The JSP specification states (JSP.8.3)
that "For each attribute declared as optional and not specified, no
variable is created.".

The ScopedAttributeELResolver has no way to determine if the attribute
name is an option tag attribute or not. If it did, it could add special
handling for this case. One option may be to set undefined optional
attributes to some special value that ScopedAttributeELResolver is then
able to detect and handle. The thing is, that could easily break
applications that are unaware of the special value.

> - if everything is conform with the spec, a more aggressive cache should
> probably be implemented and used by ImportHandler

That is a lot easier said than done.
- A new ImportHandler instance is required for every page request
- Each page can have a different set of imports
- ImportHandler is spec class used by multiple web applications each of
  which will have a different set of classes available
- The cache needs to be thread safe
- The cache needs to be self-limiting in size

Which every way you approach it, a cache that meets all of the above is
going to be non-trivial and add its own overhead. It isn't immediately
obvious what the performance impacts (positive or negative, particularly
under load) of adding such a cache would be.

In summary, there is no quick and easy solution that will work well for
everyone. The solutions that require changes to Tomcat code all come
with potential downsides for some users. There is no solution that would
be clearly beneficial for most users.
Adding the scope where necessary remains the cleanest solution that is
known not to have negative performance impacts. The price of this
solution is the time taken to do the updates.

Mark

[1] https://bz.apache.org/bugzilla/show_bug.cgi?id=57773#c1


> Related discussions/bugs:
> - most of
> https://bz.apache.org/bugzilla/buglist.cgi?bug_status=__all__&content=ImportHandler&no_redirect=1&order=Importance&product=Tomcat%208&query_format=specific
> - http://markmail.org/message/5uolmdqgy6oemru5

---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@tomcat.apache.org
For additional commands, e-mail: users-help@tomcat.apache.org