I have a soak test at constant load that is initially stable. Within the hour, an ever increasing number of blocked threads develops. The vast majority of threads are in JSP rendering, blocked on JspFactory.getDefaultFactory(). The server eventually crashes. We are running Java 6. Upon code inspection, there appears to be no real reason for synchronizing the getDefaultFactory() and setDefaultFactory() as the setter is called only once upon startup when the sub-class loads. Patching the jar, I tried three other experiments: 1) Removing the synchronized keyword entirely. 2) Locking on an inner static class instead of the JspFactory.class. 3) Using volatile for the static member variable. Both experiments #1 and #3 showed vastly better stability. I was able to double the throughput of the server without seeing increasing number of blocked threads. Experiment #2 yielded the same behavior as the original code. Thus, no other code is synchronizing on JspFactory.class. Rather, there seem to be some sort of contention in the java.lang.Class monitor. Using volatile would preserve the multi-threading semantics while avoiding contributing to the instability issue.
(In reply to comment #0) > I have a soak test at constant load that is initially stable. Within the hour, > an ever increasing number of blocked threads develops. The vast majority of > threads are in JSP rendering, blocked on JspFactory.getDefaultFactory(). > > The server eventually crashes. > > We are running Java 6. > > Upon code inspection, there appears to be no real reason for synchronizing the > getDefaultFactory() and setDefaultFactory() as the setter is called only once > upon startup when the sub-class loads. In which case, the setter should be package-protected? > Patching the jar, I tried three other experiments: > 1) Removing the synchronized keyword entirely. > 2) Locking on an inner static class instead of the JspFactory.class. > 3) Using volatile for the static member variable. > > Both experiments #1 and #3 showed vastly better stability. I was able to > double the throughput of the server without seeing increasing number of blocked > threads. > > Experiment #2 yielded the same behavior as the original code. Thus, no other > code > is synchronizing on JspFactory.class. Rather, there seem to be some sort of > contention in the java.lang.Class monitor. > > Using volatile would preserve the multi-threading semantics while avoiding > contributing to the instability issue. Might be worth trying synch. on a private static lock Object instead of an inner class? If the JspFactory class can be loaded after the JspRuntimeContext class, then JspRuntimeContext could store the factory as a static final field which could be accessed by JspFactory on startup. Or indeed, do away with setDefaultFactory() and have getDefaultFactory() return the static final value from JspRuntimeContext?
(In reply to comment #1) > (In reply to comment #0) > > I have a soak test at constant load that is initially stable. Within the hour, > > an ever increasing number of blocked threads develops. The vast majority of > > threads are in JSP rendering, blocked on JspFactory.getDefaultFactory(). > > > > The server eventually crashes. > > > > We are running Java 6. > > > > Upon code inspection, there appears to be no real reason for synchronizing the > > getDefaultFactory() and setDefaultFactory() as the setter is called only once > > upon startup when the sub-class loads. > > In which case, the setter should be package-protected? Sorry, that's nonsense - different packages. > > Patching the jar, I tried three other experiments: > > 1) Removing the synchronized keyword entirely. > > 2) Locking on an inner static class instead of the JspFactory.class. > > 3) Using volatile for the static member variable. > > > > Both experiments #1 and #3 showed vastly better stability. I was able to > > double the throughput of the server without seeing increasing number of blocked > > threads. > > > > Experiment #2 yielded the same behavior as the original code. Thus, no other > > code > > is synchronizing on JspFactory.class. Rather, there seem to be some sort of > > contention in the java.lang.Class monitor. > > > > Using volatile would preserve the multi-threading semantics while avoiding > > contributing to the instability issue. > > Might be worth trying synch. on a private static lock Object instead of an > inner class? That might perhaps help. > If the JspFactory class can be loaded after the JspRuntimeContext class, then > JspRuntimeContext could store the factory as a static final field which could > be accessed by JspFactory on startup. > > Or indeed, do away with setDefaultFactory() and have getDefaultFactory() return > the static final value from JspRuntimeContext? That's nonsense too.
The simplest approach is to change the static member variable declaration: private static volatile JspFactory deflt = null; and then remove the synchronized keyword on the getter/setter. This preserves the existing multi-threaded guarantees while not incurring the monitor overhead. If effect, exchanging a monitor for a memory latch.
Fixed in trunk and proposed for 6.0.x Note as a part of the JSP API, the changes permitted are limited.
This has been fixed in 6.0.x and will be included in 6.0.25 onwards.