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.
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.
(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. }
Created attachment 21404 [details] Simple threadLocal based TagHandler
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.
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 on attachment 21404 [details] Simple threadLocal based TagHandler ThreadLocal is not an option due to the memory leaks it triggers.
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.
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