Bug 43790

Summary: concurrent access issue on TagHandlerPool
Product: Tomcat 6 Reporter: Michael Mao <michael.mao>
Component: JasperAssignee: Tomcat Developers Mailing List <dev>
Status: RESOLVED WONTFIX    
Severity: enhancement    
Priority: P2    
Version: unspecified   
Target Milestone: default   
Hardware: Other   
OS: other   
Attachments: Simple threadLocal based TagHandler
Patche based on LinkedBlockingQueue

Description Michael Mao 2007-11-03 07:09:17 UTC
when doing performance test, i found the concurrent acess lock issue about JSP 
tag. Many threads are waiting for TagHandlerPool object, such as 
TagHandlerPool.reuse, TagHandlerPool.get. 
In TagHandlerPool and PerThreadTagHandlerPool class, object pool was used, but 
its synchronization cause the thread waiting/lock issue when a lot of JSP tags 
are using. Later, I test a ThreadLocal based TagHandler object instead of 
TagHandlerPool, the response time was reduced to 5s from 30s against 50 
concurrent user. 
As a suggestion, please remove object pool to replace with simple ThreadLocal 
solution. The same issues exist both Tomcat 5.X and 6.0.
Comment 1 Mark Thomas 2008-01-01 09:44:21 UTC
I am marking this as an enhancement.

Could you clarify how your solution is different to the PerThreadTagHandlerPool?

If you attach our proposed solution to this bug then it will be reviewed for
inclusion.
Comment 2 Michael Mao 2008-01-10 03:25:01 UTC
(In reply to comment #1)
> I am marking this as an enhancement.
> Could you clarify how your solution is different to the 
PerThreadTagHandlerPool?
> If you attach our proposed solution to this bug then it will be reviewed for
> inclusion.

With threadLocal variable, we don't need to create object pool again.
There is the following code snippet for your reference.
    private ThreadLocal<Tag> perThread = new ThreadLocal<Tag>();
    protected void init(ServletConfig config) 
    {
    }
    public Tag get(Class handlerClass) throws JspException 
    {
    	Tag jspTag = perThread.get();
    	if (jspTag == null)
    	{
            try 
            {
            	jspTag = (Tag) handlerClass.newInstance();
            	perThread.set(jspTag);
            } 
            catch (Exception e) 
            {
                throw new JspException(e.getMessage(), e);
            }
		}
    	else
    	{
    		//System.out.println("Reset tag:" + handlerClass);
    		jspTag.release();
    	}
    	return jspTag;
    }
    public void reuse(Tag handler) 
    {
    	//Do nothing.
    }
    public void release() 
    {
    	//Do nothing.
    }
Comment 3 Michael Mao 2008-01-18 01:37:15 UTC
Created attachment 21404 [details]
Simple threadLocal based TagHandler
Comment 4 Mark Thomas 2011-06-20 23:01:48 UTC
ThreadLocal isn't the way to do this. That is likely to trigger memory leaks.

A better solution would be to replace the syncs in the TagHandlerPool with a solution based on a LinkedBlockingQueue.

The PerThreadTagHandlerPool needs to be deprecated.
Comment 5 Mark Thomas 2011-06-21 00:28:33 UTC
Created attachment 27184 [details]
Patche based on LinkedBlockingQueue

While switching to LinkedBlockingQueue allows concurrent get and reuse, that appears to be balanced by the additional complexity over the current implementation.

Testing with JMeter showed no measurable performance benefit of the current implementation over the patch. I have attached the patch for future reference.

I would like to see a test case that demonstrates a clear performance benefit before applying this or any other patch.
Comment 6 Mark Thomas 2011-06-21 00:28:59 UTC
Comment on attachment 21404 [details]
Simple threadLocal based TagHandler

ThreadLocal is not an option due to the memory leaks it triggers.
Comment 7 Mark Thomas 2011-06-21 00:33:04 UTC
ThreadLocals aren't an option, LinkedBlockingQueue appears to provide no performance benefit, the current code is fairly efficient given the constraints it has to work with and there is no obvious, safe way to speed this up so resolving as WONTFIX. It can always be re-opened if a potential test case and patch is identified.
Comment 8 Ian Hartney 2012-03-20 14:57:07 UTC
There does appear to be a concurrency issue with the synchronized block approach when using Java 5.  However in Java 6 the synchronized block approach appears faster than the LinkedBlockingQueue approach.

I have a little test harness that used 250 threads to concurrently access a queue in the same manner as the TagPoolHandler.  Each loop does a get of an object, then a reuse call on the object 10000 times.  It runs the same test using the synchronized block approach, then another using the LinkedBlockingQueue (LBQ) approach.

On Java 5 the test takes around 11000 ms to complete for the synchronized block approach, the LBQ approach takes 700 ms.  Conversely on Java 6 the synchronized block approach is much improved, taking around 450 ms whereas the LBQ approach takes 750 ms.

So when using Java 5 it does appear that the LBQ approach has some significant performance advantages to the synchronized block approach.

The tested versions of java were 1.5.0_22 and 1.6.0_23