Java returning assignement vs assigning then returning. (E.g. in Singletons)

Question

Basically, you can have code like this:

From: Baeldung.com

public static synchronized ClassSingleton getInstance() {
    if(instance == null) {
        instance = new ClassSingleton();
    }
    
    return instance;
}

and you can have code like this:

My Version:

public static synchronized ClassSingleton getInstance() {
    return instance = (instance == null) ? new ClassSingleton() : instance;
}

The code below is cleaner in any way, but SonarLints rule java:S1121 sees this as non-compliant (major, code smell)

So is there more behind this except the readability SonarLint is talking about?

I have a strange feeling that my version is doing always an assignment before returning, can this be a performance disbenefit?

Happy to hear what you guys have to say.

Answer 1

Yes it may have [probably negligible] performance implications. This can be tested by just trying to compile it and see what comes out. Here is the bytecode from Javac 16.0.1:

Compiled from "ClassSingleton.java"
public class ClassSingleton {
  public ClassSingleton();
    Code:
       0: aload_0
       1: invokespecial #1                  // Method java/lang/Object."<init>":()V
       4: return

  public static synchronized ClassSingleton getInstance();
    Code:
       0: getstatic     #7                  // Field instance:LClassSingleton;
       3: ifnonnull     16
       6: new           #8                  // class ClassSingleton
       9: dup
      10: invokespecial #13                 // Method "<init>":()V
      13: putstatic     #7                  // Field instance:LClassSingleton;
      16: getstatic     #7                  // Field instance:LClassSingleton;
      19: areturn

  public static synchronized ClassSingleton getInstanceShort();
    Code:
       0: getstatic     #7                  // Field instance:LClassSingleton;
       3: ifnonnull     16
       6: new           #8                  // class ClassSingleton
       9: dup
      10: invokespecial #13                 // Method "<init>":()V
      13: goto          19
      16: getstatic     #7                  // Field instance:LClassSingleton;
      19: dup
      20: putstatic     #7                  // Field instance:LClassSingleton;
      23: areturn
}

Via the age old metric of long=bad, the second version is clearly worse. In all seriousness though, it just performs 2 extra instructions in all cases. First dup duplicates the last item on the stack so it can then use that extra item to putstatic which assigns instance to the top value on the stack. We can speculate that if another thread is not synchronized on ClassSingleton, and instance has the correct attributes, then in theory we might get some weird behavior where instance might not be set correctly. However, that seems highly unlikely considering that synchronized handles most of that for us.

In the end though, the JIT compiler will probably remove the need for dup by using registers and it has a decent chance of figuring out tat it can get rid of that extra putstatic as well. However, I am not experienced enough with the JIT compiler to do more than speculate on how it might act.

That being said, just use the first version. It is way easier to read and generates shorter bytecode.