Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > comp.lang.java.programmer > #30461 > unrolled thread

SingletonFactory and safe publication

Back to article view | Back to comp.lang.java.programmer

Contents

  SingletonFactory and safe publication John <xsli2@yahoo.com> - 2014-12-01 18:16 -0800
    Re: SingletonFactory and safe publication Arne Vajhøj <arne@vajhoej.dk> - 2014-12-01 21:34 -0500
    Re: SingletonFactory and safe publication Eric Sosman <esosman@comcast-dot-net.invalid> - 2014-12-02 18:20 -0500
      Re: SingletonFactory and safe publication Arne Vajhøj <arne@vajhoej.dk> - 2014-12-02 18:52 -0500
        Re: SingletonFactory and safe publication Eric Sosman <esosman@comcast-dot-net.invalid> - 2014-12-03 09:51 -0500
          Re: SingletonFactory and safe publication Arne Vajhøj <arne@vajhoej.dk> - 2014-12-03 21:29 -0500
          Re: SingletonFactory and safe publication Arne Vajhøj <arne@vajhoej.dk> - 2014-12-03 21:34 -0500
            Re: SingletonFactory and safe publication Robert Klemme <shortcutter@googlemail.com> - 2014-12-08 08:48 +0100
              Re: SingletonFactory and safe publication Lew <lewbloch@gmail.com> - 2015-01-04 16:15 -0800

#30461 — SingletonFactory and safe publication

Hi:

I am reading this article(http://shipilev.net/blog/2014/safe-public-construction/). It says the following code is GOOD:

public class SafeDCLFactory {
  private volatile Singleton instance;

  public Singleton get() {
    if (instance == null) {  // check 1
      synchronized(this) {
        if (instance == null) { // check 2
          instance = new Singleton();
        }
      }
    }
    return instance;
  }
}

I feel disagree, by learning from this article(http://en.wikipedia.org/wiki/Double-checked_locking).

If without 'volatile', the code above has two problems, not one problem: 1)other threads may not see the content of 'instance' which is initiated by constructing thread; 2)It is possible thread A is in the middle of constructing the object, so the reference 'instance' is not null, but what it references maybe an partially initiated object, so thread B may get this partially initiated object, causing crash later on.

Adding 'volatile' only solves the first problem, not the second. According to the second article above, the CORRECT way is:
class Foo {
    private volatile Helper helper;
    public Helper getHelper() {
        Helper result = helper;
        if (result == null) {  //1st check
            synchronized(this) {
                result = helper;
                if (result == null) { //2nd check
                    helper = result = new Helper();
                }
            }
        }
        return result;
    }
 
    // other functions and members...
}

The article does not explain well why the second problem is prevented. But I think when the constructing thread is doing:

helper = result = new Helper();

if in the middle of constructing(the worst case), 'result' is referencing it, but 'helper' is still null. Other threads will get TRUE for 1st check and go into the block[first if(result==null) block], after getting the lock(the constructing thread must have finished and released the lock), now 'result' becomes the reference pointing to a good, solid object. 2nd check will fail(and avoids constructing).

I would like to ask help to understand why the following code is GOOD. The code uses 'final', instead of 'volatile':
public class FinalWrapper<T> {
    public final T value;
    public FinalWrapper(T value) {
        this.value = value;
    }
}
 
public class Foo {
   private FinalWrapper<Helper> helperWrapper;
 
   public Helper getHelper() {
      FinalWrapper<Helper> wrapper = helperWrapper;
 
      if (wrapper == null) { //1st check
          synchronized(this) {
              if (helperWrapper == null) { //2nd check
                  helperWrapper = new FinalWrapper<Helper>(new Helper());
              }
              wrapper = helperWrapper;
          }
      }
      return wrapper.value;
   }
}
 
Thank you.

[toc] | [next] | [standalone]

#30462

On 12/1/2014 9:16 PM, John wrote:
> Hi:
>
> I am reading this article(http://shipilev.net/blog/2014/safe-public-construction/). It says the following code is GOOD:
>
> public class SafeDCLFactory {
>    private volatile Singleton instance;
>
>    public Singleton get() {
>      if (instance == null) {  // check 1
>        synchronized(this) {
>          if (instance == null) { // check 2
>            instance = new Singleton();
>          }
>        }
>      }
>      return instance;
>    }
> }
>
> I feel disagree, by learning from this article(http://en.wikipedia.org/wiki/Double-checked_locking).
>
> If without 'volatile', the code above has two problems, not one problem: 1)other threads may not see the content of 'instance' which is initiated by constructing thread; 2)It is possible thread A is in the middle of constructing the object, so the reference 'instance' is not null, but what it references maybe an partially initiated object, so thread B may get this partially initiated object, causing crash later on.
>
> Adding 'volatile' only solves the first problem, not the second.

How?

The constructor should complete before instance get assigned.

>                                                                   According to the second article above, the CORRECT way is:
> class Foo {
>      private volatile Helper helper;
>      public Helper getHelper() {
>          Helper result = helper;
>          if (result == null) {  //1st check
>              synchronized(this) {
>                  result = helper;
>                  if (result == null) { //2nd check
>                      helper = result = new Helper();
>                  }
>              }
>          }
>          return result;
>      }
>
>      // other functions and members...
> }

This is the same concept just with some irrelevant but confusing extras.

> I would like to ask help to understand why the following code is GOOD. The code uses 'final', instead of 'volatile':
> public class FinalWrapper<T> {
>      public final T value;
>      public FinalWrapper(T value) {
>          this.value = value;
>      }
> }
>
> public class Foo {
>     private FinalWrapper<Helper> helperWrapper;
>
>     public Helper getHelper() {
>        FinalWrapper<Helper> wrapper = helperWrapper;
>
>        if (wrapper == null) { //1st check
>            synchronized(this) {
>                if (helperWrapper == null) { //2nd check
>                    helperWrapper = new FinalWrapper<Helper>(new Helper());
>                }
>                wrapper = helperWrapper;
>            }
>        }
>        return wrapper.value;
>     }
> }

The Wikipedia link actually explains that:

"Semantics of final field in Java 5 can be employed to safely publish 
the helper object without using volatile"

Arne

[toc] | [prev] | [next] | [standalone]

#30470

On 12/1/2014 9:16 PM, John wrote:
> Hi:
>
> I am reading this article(http://shipilev.net/blog/2014/safe-public-construction/). It says the following code is GOOD:
>
> public class SafeDCLFactory {
>    private volatile Singleton instance;
>
>    public Singleton get() {
>      if (instance == null) {  // check 1
>        synchronized(this) {
>          if (instance == null) { // check 2
>            instance = new Singleton();
>          }
>        }
>      }
>      return instance;
>    }
> }
>
> I feel disagree, by learning from this article(http://en.wikipedia.org/wiki/Double-checked_locking).

     Brian Goetz agrees that this code is incorrect, calling it
"a commonly suggested nonfix."  He explains that although the
accesses to `instance' will be consistent because `volatile'
ensures it, any accesses to the member variables of the new
Singleton are *not* consistent (unless they are `volatile', too).
You could get a sequence like this:

	Thread T1 finds `instance' null, obtains the lock, finds
	that `instance' is still null, and calls the constructor.

	The constructor (running in T1) stores initial values in
	the member variables of the new Singleton.  We presume
	that at least some of these variables are not `volatile'.

	The constructor finishes, and now T1 stores the new
	reference to `instance'.  Because `instance' is `volatile',
	T1 ensures that the new value is actually flushed from
	store buffers and write caches and so on, and appears in
	stable memory.

	Thread T2 now finds `instance' non-null, and starts using
	it to refer to the Singleton's member variables (either
	directly or by calling the Singleton's methods).

	Unfortunately, the values stored by Singleton's constructor
	may still be sitting in caches and what-not, and may not yet
	have been flushed to stable memory.  Even if the constructor
	running in T1 stored 42 in some member variable, T2 may
	read the value as zero.

	... because there is no "happens-before" between T1's storing
	of the value and T2's reading of it.

     In short, making sure that `instance' is safe is not sufficient;
you also need to worry about everything `instance' refers to, directly
or indirectly.

http://www.javaworld.com/article/2074979/java-concurrency/double-checked-locking--clever--but-broken.html

-- 
esosman@comcast-dot-net.invalid
"Don't be afraid of work. Make work afraid of you." -- TLM

[toc] | [prev] | [next] | [standalone]

#30471

On 12/2/2014 6:20 PM, Eric Sosman wrote:
> On 12/1/2014 9:16 PM, John wrote:
>> Hi:
>>
>> I am reading this
>> article(http://shipilev.net/blog/2014/safe-public-construction/). It
>> says the following code is GOOD:
>>
>> public class SafeDCLFactory {
>>    private volatile Singleton instance;
>>
>>    public Singleton get() {
>>      if (instance == null) {  // check 1
>>        synchronized(this) {
>>          if (instance == null) { // check 2
>>            instance = new Singleton();
>>          }
>>        }
>>      }
>>      return instance;
>>    }
>> }
>>
>> I feel disagree, by learning from this
>> article(http://en.wikipedia.org/wiki/Double-checked_locking).
>
>      Brian Goetz agrees that this code is incorrect, calling it
> "a commonly suggested nonfix."  He explains that although the
> accesses to `instance' will be consistent because `volatile'
> ensures it, any accesses to the member variables of the new
> Singleton are *not* consistent (unless they are `volatile', too).
> You could get a sequence like this:
>
>      Thread T1 finds `instance' null, obtains the lock, finds
>      that `instance' is still null, and calls the constructor.
>
>      The constructor (running in T1) stores initial values in
>      the member variables of the new Singleton.  We presume
>      that at least some of these variables are not `volatile'.
>
>      The constructor finishes, and now T1 stores the new
>      reference to `instance'.  Because `instance' is `volatile',
>      T1 ensures that the new value is actually flushed from
>      store buffers and write caches and so on, and appears in
>      stable memory.
>
>      Thread T2 now finds `instance' non-null, and starts using
>      it to refer to the Singleton's member variables (either
>      directly or by calling the Singleton's methods).
>
>      Unfortunately, the values stored by Singleton's constructor
>      may still be sitting in caches and what-not, and may not yet
>      have been flushed to stable memory.  Even if the constructor
>      running in T1 stored 42 in some member variable, T2 may
>      read the value as zero.
>
>      ... because there is no "happens-before" between T1's storing
>      of the value and T2's reading of it.
>
>      In short, making sure that `instance' is safe is not sufficient;
> you also need to worry about everything `instance' refers to, directly
> or indirectly.
>
> http://www.javaworld.com/article/2074979/java-concurrency/double-checked-locking--clever--but-broken.html

But that is with the pre-5 memory model (the article is from 2001).

They changed the treatment of volatile in the memory model
in Java 5.

Or have I misunderstood something?

Arne

[toc] | [prev] | [next] | [standalone]

#30474

On 12/2/2014 6:52 PM, Arne Vajhøj wrote:
> On 12/2/2014 6:20 PM, Eric Sosman wrote:
>> On 12/1/2014 9:16 PM, John wrote:
>>> Hi:
>>>
>>> I am reading this
>>> article(http://shipilev.net/blog/2014/safe-public-construction/). It
>>> says the following code is GOOD:
>>>
>>> public class SafeDCLFactory {
>>>    private volatile Singleton instance;
>>>
>>>    public Singleton get() {
>>>      if (instance == null) {  // check 1
>>>        synchronized(this) {
>>>          if (instance == null) { // check 2
>>>            instance = new Singleton();
>>>          }
>>>        }
>>>      }
>>>      return instance;
>>>    }
>>> }
>>>
>>> I feel disagree, by learning from this
>>> article(http://en.wikipedia.org/wiki/Double-checked_locking).
>>
>>      Brian Goetz agrees that this code is incorrect, calling it
>> "a commonly suggested nonfix."  He explains that although the
>> accesses to `instance' will be consistent because `volatile'
>> ensures it, any accesses to the member variables of the new
>> Singleton are *not* consistent (unless they are `volatile', too).
>> You could get a sequence like this:
>>
>>      Thread T1 finds `instance' null, obtains the lock, finds
>>      that `instance' is still null, and calls the constructor.
>>
>>      The constructor (running in T1) stores initial values in
>>      the member variables of the new Singleton.  We presume
>>      that at least some of these variables are not `volatile'.
>>
>>      The constructor finishes, and now T1 stores the new
>>      reference to `instance'.  Because `instance' is `volatile',
>>      T1 ensures that the new value is actually flushed from
>>      store buffers and write caches and so on, and appears in
>>      stable memory.
>>
>>      Thread T2 now finds `instance' non-null, and starts using
>>      it to refer to the Singleton's member variables (either
>>      directly or by calling the Singleton's methods).
>>
>>      Unfortunately, the values stored by Singleton's constructor
>>      may still be sitting in caches and what-not, and may not yet
>>      have been flushed to stable memory.  Even if the constructor
>>      running in T1 stored 42 in some member variable, T2 may
>>      read the value as zero.
>>
>>      ... because there is no "happens-before" between T1's storing
>>      of the value and T2's reading of it.
>>
>>      In short, making sure that `instance' is safe is not sufficient;
>> you also need to worry about everything `instance' refers to, directly
>> or indirectly.
>>
>> http://www.javaworld.com/article/2074979/java-concurrency/double-checked-locking--clever--but-broken.html
>>
>
> But that is with the pre-5 memory model (the article is from 2001).
>
> They changed the treatment of volatile in the memory model
> in Java 5.
>
> Or have I misunderstood something?

     After a fair amount of studying JLS Chapter 17, my conclusion is:
"My head hurts."  :(

     17.4 describes the memory model, and the first part is not too
difficult for my comprehension: actions, program order, synchronization
order, happens-before order -- all these make sense to me.  Somewhere
near the end of 17.4.5, though, I start to bog down: The JLS starts
talking about whether a read is "allowed" to see the effect of a write,
whether a set of actions is "happens-before consistent," whether an
execution is "well-formed," and so on.  Just when it looked (to me)
like we'd reached the end of the story by defining happens-before,
the story continues in a way that makes my head ache ...

     So I turned to the less formal but more readable "Java Concurrency
in Practice" by Goetz et al., and found something understandable:

	"Subsequent changes in the JMM (Java 5.0 and later) have enabled
	DCL to work if `resource' is made `volatile', ..."

... indicating that you're right and I was wrong.  Thanks for the
correction.

     However, Goetz & Co. also opine that DCL is less useful now than
it once was (ironic: It was useful when it didn't work, and now that
it works it's useless).  Uncontended synchronization, they say, is a
lot faster than it used to be, so working hard to avoid it is effort
misdirected.  Instead, they recommend the shorter/simpler/safer
"holder class" method, here paraphrased in the O.P.'s terms:

	public class SingletonFactory {
	    private static class SingletonHolder {
	        static final Singleton instance = new Singleton();
	    }
	    public Singleton get() {
	        return SingletonHolder.instance;
	    }
	}

-- 
esosman@comcast-dot-net.invalid
"Don't be afraid of work. Make work afraid of you." -- TLM

[toc] | [prev] | [next] | [standalone]

#30478

On 12/3/2014 9:51 AM, Eric Sosman wrote:
> On 12/2/2014 6:52 PM, Arne Vajhøj wrote:
>> On 12/2/2014 6:20 PM, Eric Sosman wrote:
>>> On 12/1/2014 9:16 PM, John wrote:
>>>> Hi:
>>>>
>>>> I am reading this
>>>> article(http://shipilev.net/blog/2014/safe-public-construction/). It
>>>> says the following code is GOOD:
>>>>
>>>> public class SafeDCLFactory {
>>>>    private volatile Singleton instance;
>>>>
>>>>    public Singleton get() {
>>>>      if (instance == null) {  // check 1
>>>>        synchronized(this) {
>>>>          if (instance == null) { // check 2
>>>>            instance = new Singleton();
>>>>          }
>>>>        }
>>>>      }
>>>>      return instance;
>>>>    }
>>>> }
>>>>
>>>> I feel disagree, by learning from this
>>>> article(http://en.wikipedia.org/wiki/Double-checked_locking).
>>>
>>>      Brian Goetz agrees that this code is incorrect, calling it
>>> "a commonly suggested nonfix."  He explains that although the
>>> accesses to `instance' will be consistent because `volatile'
>>> ensures it, any accesses to the member variables of the new
>>> Singleton are *not* consistent (unless they are `volatile', too).
>>> You could get a sequence like this:
>>>
>>>      Thread T1 finds `instance' null, obtains the lock, finds
>>>      that `instance' is still null, and calls the constructor.
>>>
>>>      The constructor (running in T1) stores initial values in
>>>      the member variables of the new Singleton.  We presume
>>>      that at least some of these variables are not `volatile'.
>>>
>>>      The constructor finishes, and now T1 stores the new
>>>      reference to `instance'.  Because `instance' is `volatile',
>>>      T1 ensures that the new value is actually flushed from
>>>      store buffers and write caches and so on, and appears in
>>>      stable memory.
>>>
>>>      Thread T2 now finds `instance' non-null, and starts using
>>>      it to refer to the Singleton's member variables (either
>>>      directly or by calling the Singleton's methods).
>>>
>>>      Unfortunately, the values stored by Singleton's constructor
>>>      may still be sitting in caches and what-not, and may not yet
>>>      have been flushed to stable memory.  Even if the constructor
>>>      running in T1 stored 42 in some member variable, T2 may
>>>      read the value as zero.
>>>
>>>      ... because there is no "happens-before" between T1's storing
>>>      of the value and T2's reading of it.
>>>
>>>      In short, making sure that `instance' is safe is not sufficient;
>>> you also need to worry about everything `instance' refers to, directly
>>> or indirectly.
>>>
>>> http://www.javaworld.com/article/2074979/java-concurrency/double-checked-locking--clever--but-broken.html
>>>
>>>
>>
>> But that is with the pre-5 memory model (the article is from 2001).
>>
>> They changed the treatment of volatile in the memory model
>> in Java 5.
>>
>> Or have I misunderstood something?
>
>      After a fair amount of studying JLS Chapter 17, my conclusion is:
> "My head hurts."  :(
>
>      17.4 describes the memory model, and the first part is not too
> difficult for my comprehension: actions, program order, synchronization
> order, happens-before order -- all these make sense to me.  Somewhere
> near the end of 17.4.5, though, I start to bog down: The JLS starts
> talking about whether a read is "allowed" to see the effect of a write,
> whether a set of actions is "happens-before consistent," whether an
> execution is "well-formed," and so on.  Just when it looked (to me)
> like we'd reached the end of the story by defining happens-before,
> the story continues in a way that makes my head ache ...
>
>      So I turned to the less formal but more readable "Java Concurrency
> in Practice" by Goetz et al., and found something understandable:
>
>      "Subsequent changes in the JMM (Java 5.0 and later) have enabled
>      DCL to work if `resource' is made `volatile', ..."
>
> ... indicating that you're right and I was wrong.

I think the JSR 133 FAQ is easy to understand as well:

http://www.cs.umd.edu/~pugh/java/memoryModel/jsr-133-faq.html

<quote>
Volatile fields are special fields which are used for communicating 
state between threads. Each read of a volatile will see the last write 
to that volatile by any thread; in effect, they are designated by the 
programmer as fields for which it is never acceptable to see a "stale" 
value as a result of caching or reordering. The compiler and runtime are 
prohibited from allocating them in registers. They must also ensure that 
after they are written, they are flushed out of the cache to main 
memory, so they can immediately become visible to other threads. 
Similarly, before a volatile field is read, the cache must be 
invalidated so that the value in main memory, not the local processor 
cache, is the one seen. There are also additional restrictions on 
reordering accesses to volatile variables.

Under the old memory model, accesses to volatile variables could not be 
reordered with each other, but they could be reordered with nonvolatile 
variable accesses. This undermined the usefulness of volatile fields as 
a means of signaling conditions from one thread to another.

Under the new memory model, it is still true that volatile variables 
cannot be reordered with each other. The difference is that it is now no 
longer so easy to reorder normal field accesses around them. Writing to 
a volatile field has the same memory effect as a monitor release, and 
reading from a volatile field has the same memory effect as a monitor 
acquire. In effect, because the new memory model places stricter 
constraints on reordering of volatile field accesses with other field 
accesses, volatile or not, anything that was visible to thread A when it 
writes to volatile field f becomes visible to thread B when it reads f.
</quote>

Arne

[toc] | [prev] | [next] | [standalone]

#30479

On 12/3/2014 9:51 AM, Eric Sosman wrote:
>      However, Goetz & Co. also opine that DCL is less useful now than
> it once was (ironic: It was useful when it didn't work, and now that
> it works it's useless).  Uncontended synchronization, they say, is a
> lot faster than it used to be, so working hard to avoid it is effort
> misdirected.  Instead, they recommend the shorter/simpler/safer
> "holder class" method, here paraphrased in the O.P.'s terms:
>
>      public class SingletonFactory {
>          private static class SingletonHolder {
>              static final Singleton instance = new Singleton();
>          }
>          public Singleton get() {
>              return SingletonHolder.instance;
>          }
>      }

I have a strong feeling that all the effort "optimizing" singleton
pattern is ridiculous compared to the significance of the problem.

This has been discussed over and over again by some of the
brightest Java people for more than a decade.

And I have never heard of a case where the trivial solution
of making the getInstance method synchronized was shown
to actually have a significant negative impact on application
performance.

Arne

[toc] | [prev] | [next] | [standalone]

#30508

On 04.12.2014 03:34, Arne Vajhøj wrote:

> I have a strong feeling that all the effort "optimizing" singleton
> pattern is ridiculous compared to the significance of the problem.
>
> This has been discussed over and over again by some of the
> brightest Java people for more than a decade.
>
> And I have never heard of a case where the trivial solution
> of making the getInstance method synchronized was shown
> to actually have a significant negative impact on application
> performance.

I second that.  Plus, if it _does_ have an impact then initializing the 
instance at the time the factory instance is created and removing the 
synchronization usually is the best solution.  Granted, the instance may 
be created prematurly but if the synchronization has an impact then it's 
because there are so many accesses to the singleton throughout a long 
running program - in that case a bit of lag during application start is 
often negligible if the application runs faster then.

Kind regards

	robert

[toc] | [prev] | [next] | [standalone]

#30626

Lazy instantiation is the villain here. Don't use lazy instantiation. 

[toc] | [prev] | [standalone]

Back to top | Article view | comp.lang.java.programmer

csiph-web