Bug 49613 - Request.getAttributeNames() slows down some applications
Summary: Request.getAttributeNames() slows down some applications
Alias: None
Product: Tomcat 6
Classification: Unclassified
Component: Catalina (show other bugs)
Version: 6.0.28
Hardware: All All
: P2 major (vote)
Target Milestone: default
Assignee: Tomcat Developers Mailing List
Depends on:
Reported: 2010-07-19 06:23 UTC by Sampo Savolainen
Modified: 2010-09-07 11:56 UTC (History)
0 users

Profiler screenshot (58.36 KB, image/png)
2010-07-19 06:28 UTC, Sampo Savolainen
Proposed patch to specified issue (1.02 KB, patch)
2010-07-19 06:44 UTC, Sampo Savolainen
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sampo Savolainen 2010-07-19 06:23:09 UTC

Comment 1 Sampo Savolainen 2010-07-19 06:28:59 UTC
Created attachment 25782 [details]
Profiler screenshot

This is a screenshot from a profiler. It shows how a JSF application calls getAttributeNames() 1222 times for a single page render.
Comment 2 Sampo Savolainen 2010-07-19 06:41:28 UTC
Application frameworks like MyFaces use attributes while rendering a page. Tomcat reads all SSL connector attributes for every call to getAttributeNames(). This creates a large number of calls to getAttributeNames(). The current 6.0.x implementation is very slow as it does not recognize it has already read the SSL specific attributes. 

I will attach a patch which uses an object boolean field to determine whether the SSL attributes have already been read or not. The speedup to the profiled application is 10x.
Comment 3 Sampo Savolainen 2010-07-19 06:44:20 UTC
Created attachment 25783 [details]
Proposed patch to specified issue
Comment 4 Remy Maucherat 2010-07-19 07:51:06 UTC
Looking at your patch, you apparently did not understand the code. But I can see you might incur a performance problem if SSL is enabled and there's no certificate chain (at least if it is null). Maybe. However, using getAttributesNames repeatedly (if at all) is ... disturbing.
Comment 5 Sampo Savolainen 2010-07-19 08:18:52 UTC
Can you abbreviate on how I did not understand the code correctly? 

This is how I understood it:

'getAttributes(Globals.CERTIFICATES_ATTR)' is called when a secure connection is made. This call is made to make sure the 'if( isSSLAttribute(name) )' branch is activated. This branch then fills out the attributes Map with the SSL attributes.

The only change I did was to use an object variable (sslAttributesRead) to make sure the 'if( isSSLAttribute(name) )' branch is executed only once per request.

So this fixes the assumption the current code has, which is that once the SSL attributes are read, the 'getAttributes(Globals.CERTIFICATES_ATTR)' call terminates as it finds a value for the attribute.

I don't see a problem with my logic. Can you explain what I got wrong?

Re: disturbing...

You are very correct that this is a bit disturbing. But given the complexity and generic nature of web application frameworks, such use is inevitable. And, the performance hit the application takes is unacceptable (response time of 40 seconds instead of 4 in one case) especially since this is easily solvable.
Comment 6 Remy Maucherat 2010-07-20 10:20:44 UTC
Actually, the patch is appropriate for TC 6. I thought it should be in getAttribute, but this would interfere with the SSLAuthenticator.
Comment 7 Mark Thomas 2010-07-22 10:38:43 UTC
Thanks for the patch.

It has been applied to 7.0.x and will be in 7.0.1 onwards.

It has been proposed for 6.0.x.
Comment 8 Mark Thomas 2010-09-07 11:56:25 UTC
The patch has been applied to 6.0.x and will be included in 6.0.30 onwards.