I understand that this need to be synchronized, but can it be handled differently? The reason I ask is that we have pages that are calling this method over 5,000 per user per request. I modified the class by creating a new method that puts the evalutator in the map. That method is sync. I removed sync from the getEvaluatorByName method. When testing this under heavy load it shows significant improvement in response time and load handling. ***** The existing method public static synchronized ExpressionEvaluator getEvaluatorByName(String name) throws JspException { try { Object oEvaluator = nameMap.get(name); if (oEvaluator == null) { ExpressionEvaluator e = (ExpressionEvaluator) Class.forName(name).newInstance(); nameMap.put(name, e); return (e); } else return ((ExpressionEvaluator) oEvaluator); ***** My changes private static synchronized void addEvaluatorByName (String name) throws ClassNotFoundException, IllegalAccessException, InstantiationException { System.out.println( "addEvaluatorCount: "+getEvalCounter++); ExpressionEvaluator e = (ExpressionEvaluator) Class.forName(name).newInstance(); nameMap.put(name, e); } /** * Gets an ExpressionEvaluator from the cache, or seeds the cache * if we haven't seen a particular ExpressionEvaluator before. * * tperkins: removed synchronized from method. Movde adding of evaluator * to separate sync method (addEvaluatorByName) */ public static ExpressionEvaluator getEvaluatorByName(String name) throws JspException { try {
*** Bug 28282 has been marked as a duplicate of this bug. ***
Created attachment 11186 [details] Remove synchronized from the getEvaluatorByName method
Created attachment 11516 [details] Source for applied patch
Thanks Tony & Richard for pointing this out. Removed synchronization on getEvaluatorByName() method. Now synchronizing on nameMap and allowing reads to occur without blocking. Should be a performance improvement. Attached applied patch for reference. Patch applied to standard HEAD and available in nightly and will be in next release: Standard 1.1.1.
Gentlemen, if I understood correctly, you moved get() out of synchronized block while left put() inside. I'm not a Java guru but to me this looks like bad solution. First of all, Sun's documentation: "Note that this implementation is not synchronized. If multiple threads access this map concurrently, and at least one of the threads modifies the map structurally, it must be synchronized externally. (A structural modification is any operation that adds or deletes one or more mappings; merely changing the value associated with a key that an instance already contains is not a structural modification.)" To me this text does not say syncronization is needed between modifying threads only. It clearly says if _at least one_ thread modifies hashmap, all threads must be syncronized. Although recent JDKs should not have problems with your implementations, old ones will. In JDK 1.2 rehash() changes 'table' attribute BEFORE it copies old values there. That means if get() from one thread arrives when another thread already entered rehash() result is unpredictable. If JDK 1.3 is the minimum required for JSTL everything should be Ok and I'm terribly sorry for disturbing you. But I din't found such a notices.
There are two get's. One inside the synchronized block, and another outside the block. This allows data to be retrieved for keys that are already inside the Map without blocking on the read. The additional get inside the synchronized block will catch the case where two threads request the same key.