Synchronization on the local variables

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://");
    }

    buff开发者_如何学Cer.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?

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);  
    }
}

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.)

I don't think the synchronization can have any effect, since buffer is never passed to another method or stored in a field before it goes out of scope, so no other thread can possibly have access to it.

The reason it is there could be political - I've been in a similar situation: A "pointy-haired boss' insisted that I clone a string in a setter method instead of just storing the reference, for fear of having the contents changed. He didn't deny that strings are immutable but insisted on cloning it "just in case." Since it was harmless (just like this synchronization) I did not argue.

That's a bit insane...it doesn't do anything except add overhead. Not to mention that calls to StringBuffer are already synchronized, which is why StringBuilder is preferred for cases where you won't have multiple threads accessing the same instance.

IMO, there is no need for synchronization that local variable. Only if it was exposed to others, e.g. by passing it to a function that will store it and potentially use it in another thread, the synchronization would make sense.
But as this is not the case, I see no use of it