views:

317

answers:

5

we're running into performance issues, and one potential culprit is a centralized use of a volatile singleton. the specific code is of the form

class foo {
  static volatile instance;
  static object l = new object();

  public static foo Instance {
    if (instance == null)
      lock(l) {
        if (instance == null)
          instance = new foo();
      }

    return foo();
  }
}

this is running on an 8-way box, and we're seeing context switching to the tune of 500,000 per second. typical system resources are fine - 25% cpu util, 25% memory util, low IO, no paging, etc.

does using a volatile field induce a memory barrier or any kind of cpu cache reload? or does it just go after main memory every time, for that field only?

+5  A: 

lock does induce a memory barrier, so if you are always accessing instance in a lock you don't need the volatile.

According to this site:

The C# volatile keyword implements acquire and release semantics, which implies a read memory barrier on read and a write memory barrier on write.

Skurmedel
no - take a look at the code. theoretically, lock will only occur once, as after that the volatile variable will be set, and the outer check will pass.
kolosy
that edit makes all the difference in the world :). thanks.
kolosy
Vance Morrison also covers DCL in this MSDN article - http://msdn.microsoft.com/en-us/magazine/cc163715.aspx
Keith Hill
Very useful Keith, thanks for the link!
Skurmedel
+1  A: 

Sadly, the singleton takes a bad rap for just about everything :)

This isn't my domain of expertise, but as far as I know there's nothing special about volatile other than the compiler/run-time NOT re-ordering read/writes (to the variable) for optimization purposes.

Edit: I stand corrected. Not only does volatile introduce memory barriers, but what goes on (and incidentally, performance) depends largely on the particular CPU involved. See http://dotnetframeworkplanet.blogspot.com/2008/11/volatile-field-and-memory-barrier-look.html

This is why you still need the lock.

Questions which may/may not have been answered already:

  1. What is your singleton instance actually doing? Maybe the instance code needs to be refactored...
  2. What's the thread count of the running process? An 8 way box won't help you if you have an abnormally high thread count.
  3. If it's higher than expected, why?
  4. What else is running on the system?
  5. Is the performance issue consistent?
hythlodayr
1. not much. it's a container for some data that needs to be globally available2. thread count is adequate. context switching/second is ridiculously high. 500,000/s3. good question.4. nothing.5. yes
kolosy
A: 

In your example here, volatile should not be the subject of any "slowdown". The lock() however, can involve huge roundtrips to the kernel, especially if there's a lot of contention for the lock.

There is really no need to lock your singleton in this case, you could just do

class Foo {
  static Foo instance = new Foo();
  public static Foo FooInstance() {
    return instance ;
  }
}

Ofcourse, if 'instance ' is used in a lot of different threads, you'd still have to lock() anything that alters that Foo, unless all methods/properties of Foo is readonly. e.g.

 class Foo {
      static Foo instance = new Foo();
      object l = new object();
      int doesntChange = 42;
      int canChange = 123;
      public static Foo FooInstance() {
        return instance ;
      }
      public void Update(int newVal) {
         lock(l) { // you'll get a lot of trouble without this lock if several threads accesses the same FOO. Atleast if they later on read that variable 
            canChange = newVal;
         }

      public int GetFixedVal() {
         return doesntChange; //no need for a lock. the doesntChange is effectivly read only
      }
    }
nos
+1  A: 

There's really no need for the use of volatile for a singleton, as you're setting it exactly once - and locking the code the sets it. See Jon Skeet's article on singletons for more info.

Jon B
+1  A: 

One thing volatile will not do is cause a context switch. If you're seeing 500,000 context switches per second, it means that your threads are blocking on something and volatile is not the culprit.

Peter Ruderman