Bring lock back until 12, it has a busy day tomorrow

One of the downsides of using lock is obviously performance. While locking an object, any other thread trying to acquire the lock on that object will wait in line. This can open up a deep hole to performance hit. Rule of thumb while working with locks is to acquire it as late as possible and release it as soon as possible. To demonstrate the order of magnitude bad usage of locks can affect your performance, I decided to write a little demo. 
So let’s assume we have a component that is responsible for executing tasks while getting new ones in the process (on different threads). I tried to make this example as simple as possible. Let’s start with our “task” class:

public class Task
{
   private int _id;
   private string _name;

   public Task(int id, string name) {
      _id = id;
      _name = name;
   }

   public int Id { // getter, setter }
   public string Name { // getter, setter }
}

We have a TasksRunner that’s responsible for getting new tasks and saving it to internal list and executing the current tasks every X milliseconds (via timer). In order to simulate a real-life process, I’ve made sure that executing a single task is expensive. Let’s start with the non-optimized solution:

public class TasksRunner
{
   private List<Task> _tasks;
   private System.Timers.Timer _handleTasksTimer;

   public TasksRunner()
   {
      _tasks = new List<Task>();

      _handleTasksTimer = new Timer(200); 
      _handleTasksTimer.Elapsed += new System.Timers.ElapsedEventHandler(_handleTasksTimer_Elapsed);
      _handleTasksTimer.Start();
   }

   public void AddTask(Task t)
   {
      lock (_tasks)
      {
         _tasks.Add(t);
         Console.WriteLine(“Task added, id: “ + t.Id + “, name: “ + t.Name);
      }
   }

   //Execute the (delta) tasks in a thread from the ThreadPool
   private void _handleTasksTimer_Elapsed(object sender, System.Timers.ElapsedEventArgs e)
   {
      ExecuteCurrentTasks();
   }

   public void ExecuteCurrentTasks()
   {
      lock (_tasks)
      {
         foreach (Task t in _tasks)
            ExecuteSingleTask(t);
         
         _tasks.Clear();
      }
   }

   private void ExecuteSingleTask(Task t)
   {
      Console.WriteLine(“Handling task, id: “ + t.Id + “, name: “ + t.Name);
      Thread.Sleep(1000); //simulate long run
   }
}

AddTask will acquire the lock on _tasks and add the new task to the list while ExecuteCurrentTasks will acquire the lock (on _tasks) and simulate real execution on the task. Notice that during the execution, calling AddTask will wait until the current execution will be finished. Using Roy‘s ThreadTester, we can run the following in order to notice the behavior so far:

static void Main(string[] args)
{
   TasksRunner runner = new TasksRunner();
   ThreadTester threadTester = new ThreadTester();
   threadTester.RunBehavior = ThreadRunBehavior.RunUntilAllThreadsFinish;

   Stopwatch watch = new Stopwatch();
   watch.Start();

   int numberOfTasksToCreate = 100;
   threadTester.AddThreadAction(delegate
      {
         for (int j = 0; j < numberOfTasksToCreate; j++) 
         {
            runner.AddTask(new Task(j, “job “ + j));
            Thread.Sleep(100);
         }
      });

   threadTester.StartAllThreads(int.MaxValue); //wait, no matter how long

   Console.WriteLine(“Total time so far (milliseconds): “ + watch.ElapsedMilliseconds);
   Console.WriteLine(“Tasks added so far: “ + runner.TasksAdded);
   Console.WriteLine(“Tasks executed so far: “ + runner.TasksExecuted);
   Console.WriteLine(“Waiting for tasks to end…”);

   while (runner.TasksExecuted < numberOfTasksToCreate)
      Thread.Sleep(1000);

   runner.Shutdown();

   Console.WriteLine(“done!”);
   Console.WriteLine(“Total time so far (milliseconds): “ + watch.ElapsedMilliseconds);
   Console.WriteLine(“Tasks added so far: “ + runner.TasksAdded);
   Console.WriteLine(“Tasks executed so far: “ + runner.TasksExecuted);
}

Running this test will give us a very poor result for adding & executing 100 tasks takes around ~99 seconds.

No doubt, the lock on _tasks while executing each and every task in the list is too expensive as we’re depend on ExecuteSingleTask (which is expensive by itself). This way, each new task we’re trying to add must wait until the current execution is finished. An elegant solution to this problem, suggested by my teammate Tomer Gabel, is to use a temporal object to point to the current tasks thus freeing the lock much quicker. So here is an optimized version of ExecuteCurrentTasks:

public void ExecuteCurrentTasks()
{
   List<Task> copyOfTasks = null;
   lock (_tasks)
   {
      copyOfTasks = _tasks;
      _tasks = new List<Task>();
   }

   foreach (Task t in copyOfTasks)
      ExecuteSingleTask(t);
}

This little refactoring give us around ~11 seconds for adding & executing 100 tasks.

Smoking!