Do we get any benefit to change existing code from:
class MyClass {
volatile Object myVariable;
Object myMethod() {
if (myVariable == null) {
synchronized(this) {
if (myVariable == null) {
myVariable = this.getNewValue();
}
}
}
return myVariable;
}
}
to
class MyClass {
volatile Object myVariable;
Object myMethod() {
Object tmp = this.myVariable;
if (tmp == null) {
synchronized(this) {
tmp = this.myVariable;
if (tmp == null) {
this.myVariable = tmp = this.getNewValue();
}
}
}
return tmp;
}
}
I don't see the point of copying this.myVariable locally before using it, and I don't think it's a good practice to use "this." for every class variable.
Copying to a local variable is more efficient and more correct.
More Efficient: Assume that in the common case, myVariable is non-null. In the first version, you do two reads of myVariable, once to check for null and once to return the value. In the second version, you do one read of myVariable and two reads of tmp (local variable access, which is trivial). The whole point of using volatile is the strong memory guarantees, and those guarantees mean that two reads is a non-trivial performance hit over just one read.
More Correct: Assume that myVariable is some sort of "cache" which periodically needs to be refreshed. I.e. there is a background thread which periodically sets myVariable to null so that it will be reloaded on the next read. In the first version, you do two separate reads of myVariable. The first read could return non-null, then the "cache refresh" logic runs and sets myVariable to null. The second read (to return the value) then returns null! In the second version, tmp is always the value you tested for null (assuming that getNewValue() never returns null of course).
Note that using "this." is a stylistic choice in this code and has no bearing on the correctness or performance issues.
This is largely just expanding on what is stated in https://en.wikipedia.org/wiki/Double-checked_locking#Usage_in_Java .