Bug 25967 - getEvaluatorByName is synchronized
Summary: getEvaluatorByName is synchronized
Alias: None
Product: Taglibs
Classification: Unclassified
Component: Standard Taglib (show other bugs)
Version: 1.0.2
Hardware: Other other
: P3 normal with 2 votes (vote)
Target Milestone: ---
Assignee: Tomcat Developers Mailing List
URL: http://cvs.apache.org/viewcvs.cgi/jak...
: 28282 (view as bug list)
Depends on:
Blocks: 57433
  Show dependency tree
Reported: 2004-01-07 21:25 UTC by Tony Perkins
Modified: 2015-01-11 19:27 UTC (History)
2 users (show)

Remove synchronized from the getEvaluatorByName method (1.61 KB, patch)
2004-04-08 16:38 UTC, Richard Kraft
Details | Diff
Source for applied patch (1.78 KB, patch)
2004-05-11 23:14 UTC, Justyna Horwat
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tony Perkins 2004-01-07 21:25:49 UTC
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)
                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)
        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 {
Comment 1 Richard Kraft 2004-04-08 16:37:50 UTC
*** Bug 28282 has been marked as a duplicate of this bug. ***
Comment 2 Richard Kraft 2004-04-08 16:38:36 UTC
Created attachment 11186 [details]
Remove synchronized from the getEvaluatorByName method
Comment 3 Justyna Horwat 2004-05-11 23:14:49 UTC
Created attachment 11516 [details]
Source for applied patch
Comment 4 Justyna Horwat 2004-05-11 23:18:37 UTC
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.
Comment 5 Dmitry Andrianov 2004-07-29 00:44:07 UTC
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.
Comment 6 Richard Kraft 2004-07-30 03:14:17 UTC
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.