Synchronization on the local variables

Question

Today I was faced with the method constructServiceUrl() of the org.jasig.cas.client.util.CommonUtils class. I thought he was very strange:

final StringBuffer buffer = new StringBuffer();  synchronized (buffer) {     if (!serverName.startsWith("https://") && !serverName.startsWith("http://"))     {         buffer.append(request.isSecure() ? "https://" : "http://");     }      buffer.append(serverName);     buffer.append(request.getRequestURI());      if (CommonUtils.isNotBlank(request.getQueryString()))     {         final int location = request.getQueryString().indexOf(                 artifactParameterName + "=");          if (location == 0)         {             final String returnValue = encode ? response.encodeURL(buffer.toString()) : buffer.toString();              if (LOG.isDebugEnabled())             {                 LOG.debug("serviceUrl generated: " + returnValue);             }              return returnValue;         }          buffer.append("?");          if (location == -1)         {             buffer.append(request.getQueryString());         }         else if (location > 0)         {             final int actualLocation = request.getQueryString()                     .indexOf("&" + artifactParameterName + "=");              if (actualLocation == -1)             {                 buffer.append(request.getQueryString());             }             else if (actualLocation > 0)             {                 buffer.append(request.getQueryString().substring(0, actualLocation));             }         }     } } 

Why did the author synchronizes a local variable?

Answer 1

This is an example of manual "lock coarsening" and may have been done to get a performance boost.

Consider these two snippets:

StringBuffer b = new StringBuffer(); for(int i = 0 ; i < 100; i++){     b.append(i); } 

versus:

StringBuffer b = new StringBuffer(); synchronized(b){   for(int i = 0 ; i < 100; i++){      b.append(i);   } } 

In the first case, the StringBuffer must acquire and release a lock 100 times (because append is a synchronized method), whereas in the second case, the lock is acquired and released only once. This can give you a performance boost and is probably why the author did it. In some cases, the compiler can perform this lock coarsening for you (but not around looping constructs because you could end up holding a lock for long periods of time).

By the way, the compiler can detect that an object is not "escaping" from a method and so remove acquiring and releasing locks on the object altogether (lock elision) since no other thread can access the object anyway. A lot of work has been done on this in JDK7.

---

Update:

I carried out two quick tests:

1) WITHOUT WARM-UP:

In this test, I did not run the methods a few times to "warm-up" the JVM. This means that the Java Hotspot Server Compiler did not get a chance to optimize code e.g. by eliminating locks for escaping objects.

JDK                1.4.2_19    1.5.0_21    1.6.0_21    1.7.0_06 WITH-SYNC (ms)         3172        1108        3822        2786 WITHOUT-SYNC (ms)      3660         801         509         763 STRINGBUILDER (ms)      N/A         450         434         475 

With JDK 1.4, the code with the external synchronized block is faster. However, with JDK 5 and above the code without external synchronization wins.

2) WITH WARM-UP:

In this test, the methods were run a few times before the timings were calculated. This was done so that the JVM could optimize code by performing escape analysis.

JDK                1.4.2_19    1.5.0_21    1.6.0_21    1.7.0_06 WITH-SYNC (ms)         3190         614         565         587 WITHOUT-SYNC (ms)      3593         779         563         610 STRINGBUILDER (ms)      N/A         450         434         475 

Once again, with JDK 1.4, the code with the external synchronized block is faster. However, with JDK 5 and above, both methods perform equally well.

Here is my test class (feel free to improve):

public class StringBufferTest {      public static void unsync() {         StringBuffer buffer = new StringBuffer();         for (int i = 0; i < 9999999; i++) {             buffer.append(i);             buffer.delete(0, buffer.length() - 1);         }      }      public static void sync() {         StringBuffer buffer = new StringBuffer();         synchronized (buffer) {             for (int i = 0; i < 9999999; i++) {                 buffer.append(i);                 buffer.delete(0, buffer.length() - 1);             }         }     }      public static void sb() {         StringBuilder buffer = new StringBuilder();         synchronized (buffer) {             for (int i = 0; i < 9999999; i++) {                 buffer.append(i);                 buffer.delete(0, buffer.length() - 1);             }         }     }          public static void main(String[] args) {          System.out.println(System.getProperty("java.version"));          // warm up         for(int i = 0 ; i < 10 ; i++){             unsync();             sync();             sb();         }          long start = System.currentTimeMillis();         unsync();         long end = System.currentTimeMillis();         long duration = end - start;         System.out.println("Unsync: " + duration);          start = System.currentTimeMillis();         sync();         end = System.currentTimeMillis();         duration = end - start;         System.out.println("sync: " + duration);          start = System.currentTimeMillis();         sb();         end = System.currentTimeMillis();         duration = end - start;         System.out.println("sb: " + duration);       } } 

Answer 2

Inexperience, incompetence, or more likely dead yet benign code that remains after refactoring.

You're right to question the worth of this - modern compilers will use escape analysis to determine that the object in question cannot be referenced by another thread, and so will elide (remove) the synchronization altogether.

(In a broader sense, it is sometimes useful to synchronize on a local variable - they are still objects after all, and another thread can still have a reference to them (so long as they have been somehow "published" after their creation). Still, this is seldom a good idea as it's often unclear and very difficult to get right - a more explicitly locking mechanism with other threads is likely to prove better overall in these cases.)