39

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?

Samuel Parkinson
  • 2,992
  • 1
  • 27
  • 38
artemka
  • 444
  • 1
  • 5
  • 8
  • 1
    Aren't there other side effects of synchronization that happen regardless of what is being sychronized? – TREE May 25 '11 at 11:46
  • @TREE, no. It need not have any effect as long as no other thread can see the object that's being synchronized on. – Ringding May 26 '11 at 07:46
  • The accepted answer to this question clarifies why I was wrong: http://stackoverflow.com/questions/1850270/memory-effects-of-synchronization-in-java – TREE May 26 '11 at 13:53

5 Answers5

74

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);  
    }
}
dogbane
  • 266,786
  • 75
  • 396
  • 414
  • 4
    Interesting, but I doubt that it is the main motivation in this example (because a better optimization would be to use a `StringBuilder` instead, removing the lock completely.) – finnw May 25 '11 at 13:22
  • 4
    @finnw well the code was written in 2007 so it's possible that the author was not using Java5 e.g. I can't see the use of generics. – dogbane May 25 '11 at 13:49
  • -1 I created this snippet https://gist.github.com/3828895 and the opposite is true. With synchronization takes almost twice – OscarRyz Oct 03 '12 at 18:37
  • 2
    @oscarryz your test is invalid. You need to use JDK1.4 because that is the version that the code in question (org.jasig.cas.client.util.CommonUtils) was written in. If you repeat the test with JDK1.4 you will see that the version with the synchronized block is faster. – dogbane Oct 04 '12 at 07:47
  • Lock coarsening is a bogus argument! If the StringBuffer really is shared(?), the real reason for the synchronized wrapper is to make all the method calls part of an atomic operation, and StringBuilder would have been less costly than a StringBuffer, especially now that lock eliding has been dropped in a recent Java version! StringBuffer, Hashtable and Vector were serious mistakes, thus far less use, because they can easily provide a false sense of security, and the same cautions apply to the Collections.synchronized* methods. – Infernoz Nov 15 '20 at 00:19
7

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

Andrzej Doyle
  • 102,507
  • 33
  • 189
  • 228
5

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.

finnw
  • 47,861
  • 24
  • 143
  • 221
4

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.

stevevls
  • 10,675
  • 1
  • 45
  • 50
1

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

king_nak
  • 11,313
  • 33
  • 58