System.Threading.Monitor.Enter (under the hood)

System.Threading.Monitor.Enter (under the hood)

• suomi7

  I appologize for "garbage", I was just trying to say that it's a well known and widely discussed optimization, called double check locking. The sleep and the flag really do more harm in there than good. The idea is generally to let the first thread get the data and block all the other threads untill the data is available. This is actually a subtle problem and more details can found on the web. This subtelties can be avoided in .NET using a static initializer as below.

  class DataHolder

  {

  static DataSet data = ConstructDataSet();

  private static DataSet ConstructDataSet()

  {

  // ...

  return null;

  }

  }

  
  

• WimB

  A classical double check locking looks like below. The rest is garbage :). If this lazy evaluation is not needed, it is easier to just getData() in the constructor.

  DataSet Data {

  get {

  if (ds == null) {

  lock (syncRoot) {

  if (ds == null) {

  ds = getData();

  }

  }

  }

  }

  return ds;

  }

  
  

• jam123jam123

  Hi,

  there might be an idea of merit in here somewhere. If the Monitor.Enter call is significantly more expensive than just checking a static flag (unless the threads are all calling the same instance), it might be "better" (here meaning "it might use less CPU" - it will *not* decrease latency!) to sleep a short time than to attempt to aquire the lock.

  However, the way it's implemented here it cannot possibly have any benefit, because it simply delays all threads by the same amount of time, and then tries to aquire the lock regardless!

  I doubt that the Monitor.Enter call is very expensive, but you could try to change your logic so it does not try to aquire the lock at all if the flag is set. To be fault-tolerant you need to make sure the flag is lowered again regardless of exceptions, so the code would become like this (sorry about the formatting - I can't get it right in this control):

  bool dsLocked = false;

  DataSet ds;

  DataSet Data {

  get {

    while (dsLocked) Thread.Sleep(50);

    if (ds == null) {

      lock (syncRoot) {

         if (ds == null) {

            dsLocked = true;

            try { ds = getData(); }

            finally { dsLocked = false; }

         }  // if

       }  // lock

    } // if

  return ds;

  }

  I doubt however that this will perform better than without the dsLocked flag and the sleep. But it *does* eliminate extra calls to Monitor.Enter while "getData()" executes, so I cannot really say I'm sure.

  Notice also that 50 milliseconds sleep time is probably far too high; I'd try a shorter sleep time if I were you.

  There is another notable difference between this implementation and only using the lock statement alone. Consider how an implementation using just the lock statement will behave:

  If a large number of threads are blocked by the one that is getting the data, these threads will have to form a nice queue, each one entering the locked region after the data has been fetched. Of course, they wouldn't stay long since the first thing they do is re-check if the dataset is null, but still the threads queued up would all enter the locked region and therefore be unable to execute concurrently.

  In contrast, with this implementation only a few threads could ever get as far as to the lock statement (more than one is possible since the reading of the flag is not thread safe, but the flag will still be set "at the beginning of things" compared to the 3 second execution time and thus might prevent a large number of threads from entering the locked region).

  If you try this out, let us know what your findings are! Like I said, I doubt it has any real merit, but it is an intriguing idea I think.

  
  

• Mario Aoun

  You're digressing and I'm beginning to think you didn't understand my point or indeed the issue. I'm a big fan of bluntness and wouldn't want you to pack your opinions in cotton, so don't worry about calling it "garbage". It would mean far more if you could substantiate the claim it doesn't achieve anything rather than change the words (but not the meaning - code that doesn't achieve anything deserves to be called garbage, clutter, etc.).

  To make the code execute only once, no double check is needed. Only the if (ds == null) that is INSIDE of the locked region is thread-safe.

  The outer if is there for one reason only: To allow threads that execute AFTER the first thread has finished executing the locked region to run concurrently (without entering the locked region).

  In this case, however, the person asking posted a case in which the locked region took a long time to execute. During that time, hundreds or thousands of threads could all be blocked on the Monitor.Enter statement. Once the first thread finished, the second thread would aquire the lock and all the other threads would still be blocked. All the threads that got beyond the first if condition have to execute one by one, each locking and unlocking before returning.

  My proposed solution doesn't behave that way, because once the first thread gets beyond the line setting the flag - which obviously takes much less than 3 seconds - new threads will never enter the monitor. Thus, when the first thread finishes, the hundreds or thousands of waiting threads will be able to execute concurrently. As for threads initiated after the first thread finished, they will all execute the same code in my example and your broilerplate code, so there is no cost at this point. Like I said originally, I think 50 ms is a bit long, and I'm doubtful if anything can be gained. So I'm still not saying this *will* achieve something worthy of achieving. However you seem certain that it achieves nothing, yet you don't refute that it does allow threads that would otherwise execute one at a time to run concurrently, nor do you provide any reasoning as to why this difference doesn't matter.

  
  

• mallio

  Monitor.Enter and Exit implement mutual exclusion. Only one thread can execute inside a region protected by a monitor. So Monitor.Enter checks if some thread already executes inside that region. If another thread is not there, then the monitor is marked as taken and the thread procedes. If the monitor is already taken then it waits for it to become free again. So it enters the WaitSleepJoin thread state. Pretty much it sleeps until the monitor is free again, when it wakes up and tries to enter again and so on. It does not consume any CPU while waiting.

  Sleep is not a way to synchronize threads :). Check MSDN for examples.

  
  

• celecstialclockie

  So we are all on the same page and not arguing over semantics here's where my logic is comming from.

  first this code originated here [ http://www.yoda.arachsys.com/csharp/SingletonBenchmark.cs ]

  It was designed to show to those who argued that the double lock was too slow to use that they were areguing over 3 seconds over 1 billion calls.

  I then added the SleeperDcl call just for fun. To my surprise it was faster leading me to wonder why. Hence this post.

  Now I will give Lucian that they could be right and that the sleeper is inefficent but the question is why is it faster.

  -----

  Here's what my computer reported

  SimpleLock took 00:01:30.9339216
  FixedDcl took 00:00:10.2462960
  SleeperDcl took 00:00:09.9647184
  BrokenDcl took 00:00:10.2462960
  SimpleNoLockLazy took 00:00:07.5087360
  SimpleNoLockNotAsLazy took 00:00:07.5556656
  NestedLazy took 00:00:10.9033104
  NestedNotAsLazy took 00:00:07.4774496

  -----

  Here's the code so you can all try it out.

  NOTE:

  I lowered the sleep
  I broke out the "Lock()" into it's "Try - Finally" blocks so i could add the "isLocked" to the "finally" section.

  -----

  using System;
  using System.Drawing;
  using System.Collections;
  using System.ComponentModel;
  using System.Windows.Forms;
  using System.Data;

  namespace singletonbenchmark
  {
  /// <summary>
  /// Summary description for Form1.
  /// </summary>
  public class Form1 : System.Windows.Forms.Form
  {
  private System.Windows.Forms.Button button1;
  /// <summary>
  /// Required designer variable.
  /// </summary>
  private System.ComponentModel.Container components = null;

  public Form1()
  {
  //
  // Required for Windows Form Designer support
  //
  InitializeComponent();

  //
  // TODO: Add any constructor code after InitializeComponent call
  //
  }

  /// <summary>
  /// Clean up any resources being used.
  /// </summary>
  protected override void Dispose( bool disposing )
  {
  if( disposing )
  {
  if (components != null)
  {
  components.Dispose();
  }
  }
  base.Dispose( disposing );
  }

  #region Windows Form Designer generated code
  /// <summary>
  /// Required method for Designer support - do not modify
  /// the contents of this method with the code editor.
  /// </summary>
  private void InitializeComponent()
  {
  this.button1 = new System.Windows.Forms.Button();
  this.SuspendLayout();
  //
  // button1
  //
  this.button1.Location = new System.Drawing.Point(0, 0);
  this.button1.Name = "button1";
  this.button1.TabIndex = 0;
  this.button1.Text = "button1";
  this.button1.Click += new System.EventHandler(this.button1_Click);
  //
  // Form1
  //
  this.AutoScaleBaseSize = new System.Drawing.Size(5, 13);
  this.ClientSize = new System.Drawing.Size(292, 273);
  this.Controls.Add(this.button1);
  this.Name = "Form1";
  this.Text = "Form1";
  this.ResumeLayout(false);

  }
  #endregion

  /// <summary>
  /// The main entry point for the application.
  /// </summary>
  [STAThread]
  static void Main()
  {
  Application.Run(new Form1());
  }

  private void button1_Click(object sender, System.EventArgs e)
  {
  Benchmark();
  }

  const int Iterations = 1000000000;
  
  static DateTime timer;
  
  static void Benchmark()
  {
  ResetTimer(null);
  
  for (int i=0; i < Iterations; i++)
  {
  SimpleLock sl = SimpleLock.Instance;
  }
  ResetTimer("SimpleLock");
  
  for (int i=0; i < Iterations; i++)
  {
  FixedDcl dcl = FixedDcl.Instance;
  }
  ResetTimer("FixedDcl");

  for (int i=0; i < Iterations; i++)
  {
  SleeperDcl dcl = SleeperDcl.Instance;
  }
  ResetTimer("SleeperDcl");

  for (int i=0; i < Iterations; i++)
  {
  BrokenDcl dcl = BrokenDcl.Instance;
  }
  ResetTimer("BrokenDcl");

  for (int i=0; i < Iterations; i++)
  {
  SimpleNoLockLazy inst = SimpleNoLockLazy.Instance;
  }
  ResetTimer("SimpleNoLockLazy");

  for (int i=0; i < Iterations; i++)
  {
  SimpleNoLockNotAsLazy inst = SimpleNoLockNotAsLazy.Instance;
  }
  ResetTimer("SimpleNoLockNotAsLazy");
  
  for (int i=0; i < Iterations; i++)
  {
  NestedLazy nl = NestedLazy.Instance;
  }
  ResetTimer("NestedLazy");

  for (int i=0; i < Iterations; i++)
  {
  NestedNotAsLazy nl = NestedNotAsLazy.Instance;
  }
  ResetTimer("NestedNotAsLazy");
  
  }
  
  static void ResetTimer (string description)
  {
  DateTime now = DateTime.UtcNow;
  if (description != null)
  {
  Console.WriteLine ("{0} took {1}", description, now-timer);
  }
  timer = DateTime.UtcNow;
  }
  }

  public sealed class SimpleLock
  {
  static SimpleLock instance=null;
  static readonly object padlock = new object();

  SimpleLock()
  {
  }

  public static SimpleLock Instance
  {
  get
  {
  lock (padlock)
  {
  if (instance==null)
  {
  instance = new SimpleLock();
  }
  return instance;
  }
  }
  }
  }

  public sealed class BrokenDcl
  {
  static BrokenDcl instance=null;
  static readonly object padlock = new object();

  BrokenDcl()
  {
  }

  public static BrokenDcl Instance
  {
  get
  {
  if (instance==null)
  {
  lock (padlock)
  {
  if (instance==null)
  {
  instance = new BrokenDcl();
  }
  }
  }
  return instance;
  }
  }
  }

  public sealed class FixedDcl
  {
  static volatile FixedDcl instance=null;
  static readonly object padlock = new object();

  FixedDcl()
  {
  }

  public static FixedDcl Instance
  {
  get
  {
  if (instance==null)
  {
  lock (padlock)
  {
  if (instance==null)
  {
  instance = new FixedDcl();
  }
  }
  }
  return instance;
  }
  }
  }

  public sealed class SleeperDcl
  {
  static volatile SleeperDcl instance=null;
  static readonly object padlock = new object();
  static volatile bool IsLocked = false;

  SleeperDcl()
  {
  }

  public static SleeperDcl Instance
  {
  get
  {
  if (instance==null)
  {
  while(IsLocked)
  System.Threading.Thread.Sleep(5);
  System.Threading.Monitor.Enter(padlock);
  try
  {
  if (instance==null)
  {
  IsLocked = true;
  instance = new SleeperDcl();
  }
  }
  finally
  {
  IsLocked = false;
  System.Threading.Monitor.Exit(padlock);
  }
  }
  return instance;
  }
  }
  }

  public sealed class SimpleNoLockLazy
  {
  static readonly SimpleNoLockLazy instance=new SimpleNoLockLazy();

  // Explicit static constructor to tell C# compiler
  // not to mark type as beforefieldinit
  static SimpleNoLockLazy()
  {
  }

  SimpleNoLockLazy()
  {
  }

  public static SimpleNoLockLazy Instance
  {
  get
  {
  return instance;
  }
  }
  }

  public sealed class SimpleNoLockNotAsLazy
  {
  static readonly SimpleNoLockNotAsLazy instance=new SimpleNoLockNotAsLazy();

  SimpleNoLockNotAsLazy()
  {
  }

  public static SimpleNoLockNotAsLazy Instance
  {
  get
  {
  return instance;
  }
  }
  }

  public sealed class NestedLazy
  {
  NestedLazy()
  {
  }

  public static NestedLazy Instance
  {
  get
  {
  return Nested.instance;
  }
  }
  
  class Nested
  {
  // Explicit static constructor to tell C# compiler
  // not to mark type as beforefieldinit
  static Nested()
  {
  }

  internal static readonly NestedLazy instance = new NestedLazy();
  }
  }

  public sealed class NestedNotAsLazy
  {
  NestedNotAsLazy()
  {
  }

  public static NestedNotAsLazy Instance
  {
  get
  {
  return Nested.instance;
  }
  }
  
  class Nested
  {
  internal static readonly NestedNotAsLazy instance = new NestedNotAsLazy();
  }
  }
  }

  
  

• NO_spam

  So the slow down, if i read this right, is the fact that it doubles the top object on the stack.  (seems pretty inefficient to me).

  NOTE: This is a debug build. (edt: just double checked the code is in release compile too)

  Double lock

    IL_0000:  volatile.
    IL_0002:  ldsfld     class singletonbenchmark.FixedDcl modreq([mscorlib]System.Runtime.CompilerServices.IsVolatile) singletonbenchmark.FixedDcl::'instance'
    IL_0007:  brtrue.s   IL_0033
    IL_0009:  ldsfld     object singletonbenchmark.FixedDcl::padlock
    IL_000e:  dup
    IL_000f:  stloc.1
    IL_0010:  call       void [mscorlib]System.Threading.Monitor::Enter(object)
    .try
    {
  

  Sleeper

    IL_0000:  volatile.
    IL_0002:  ldsfld     class singletonbenchmark.SleeperDcl modreq([mscorlib]System.Runtime.CompilerServices.IsVolatile) singletonbenchmark.SleeperDcl::'instance'
    IL_0007:  brtrue.s   IL_0056
    IL_0009:  br.s       IL_0011
    IL_000b:  ldc.i4.5
    IL_000c:  call       void [mscorlib]System.Threading.Thread::Sleep(int32)
    IL_0011:  volatile.
    IL_0013:  ldsfld     bool modreq([mscorlib]System.Runtime.CompilerServices.IsVolatile) singletonbenchmark.SleeperDcl::IsLocked
    IL_0018:  brtrue.s   IL_000b
    IL_001a:  ldsfld     object singletonbenchmark.SleeperDcl::padlock
    IL_001f:  call       void [mscorlib]System.Threading.Monitor::Enter(object)
    .try
    {
  

  
  

• Smacksnr

  These are difficult issues, so sticking to best practices seems wise to me. The solution is much too complex, and the flag should be made volatile for it to work as expected. Besides unneeded complexity, it is also less efficient than the classic solution. The problem is that the threads cannot know how much to go to sleep, so they either sleep while there is data available, either they wake up and sleep again, which is very expensive. Just taking the lock, seeing that ds is not null and exiting the lock is much less expensive in comparison. The main cost of the lock in this case is the waiting.

  The purpose of the first null check is to avoid taking a lock on reads (except those made during the loading of data).

  The idea of a loop before taking a lock is useful to avoid waiting, as is the case for spinlocks.

  
  

• Vikas Arora

  I don't see anywhere where you spawn any threads. Since there's only one thread there won't be any contention for the lock and Sleep will never get called.

  What sort of results do you get if you run the application several times. One run isn't going to be accurate.

  
  
  

• Jerry Frame

  I agree that these tests, given the fact that there is only one thread, hide the real problems of SleeperDcl. Having said that, it is true that SleeperDcl performs a little better than FixedDcl. This is not really relevant, but still, why does it happen J

  One reason is that the compiler generates the code for “lock” and copies the static field padlock into a local variable which it uses for Monitor.Enter and Exit. We can prevent this by writing Monitor.Enter and Exit by hand, as in the SleeperDcl example. But even after this, SleeperDcl is still a little faster. Commenting the while seems to make them perform the same. Weird. This is as deep as I can go with thisJ.

  
  

• Joshua Morgan

  Hi,

  that's the no-brainer solution for sure. But to call the rest "garbage" is pretty arrogant. The difference is that your implementation forces all threads that try to read the property during the initial execution to enter the locked region. It is not blindingly obvious, at least not to me, that nothing can be gained from allowing these threads to execute concurrently *after* the first thread has finished getting the data. That is what my suggested implementation does, and I think you should at least provide a little more substantiation before calling it "garbage".

  
  

• Bliperd

  The problem is, given that the only code is that shown in the example, testing a boolean before executing a method is not thread-safe.  Between when the boolean is checked and the method is called the state of the boolean could change rendering the call to the method extraneous.  Furthermore, using Sleep() forces the thread to be "busy" for a specific amount of time.  While the thread is sleeping another thread could come along and test the boolean which could now be false and continue on "budding in line", in front of your sleeping thread--possibly forcing your thread to sleep continously.

  The other problem is that IsdsUsersLock will also be true when UsersPadlock is locked, making it a double  (redundant) check.  Just get rid of the boolean and its check and use lock, you'll get a first-in-first-out processing.

  In my experience, use of Sleep means there is a logic error.  If you have to add Sleep to get your application to work correctly your locking is not implemented correctly.

  What do you define as "help" with respect to "The sleep call appears to help"   Lucian's example is much better, but you should lock before checking your field for null; because it could change between the comparison and the lock.  For example:

  DataSet Data {  get  {   lock (dataLock)   {    if (ds == null)    {     ds = getData();    }   }   return ds;  } }

  As a reference, I suggest reading Vance Morrison's article on threading (http://msdn.microsoft.com/msdnmag/issues/05/08/Concurrency/) and Andrew Birrell has a good article (http://research.microsoft.com/research/pubs/view.aspx type=Technical%20Report&id=921) as well (just ignore use of "lock(this)" and "lock(typeof(...))").

  
  
  
• System.Threading.Monitor.Enter (under the hood)