Performance, threading and double checked locks

time to read 3 min | 507 words

A very common pattern for lazy initialization of expensive things is this:

GetDescriptionsHelper.Delegate getDescription;
if (GetDescriptionsHelper.Cache.TryGetValue(entityType, out getDescription) == false)
{
	MethodInfo getDescriptionInternalGeneric = getDescriptionInternalMethodInfo.MakeGenericMethod(entityType);
	getDescription = (GetDescriptionsHelper.Delegate)Delegate.CreateDelegate(
		typeof(GetDescriptionsHelper.Delegate),
		getDescriptionInternalGeneric);

	GetDescriptionsHelper.Cache.Add(entityType, getDescription);
}

This tends to break down when we are talking about code that can run in multiply threads. So we start writing this:

lock(GetDescriptionsHelper.Cache)
{
	GetDescriptionsHelper.Delegate getDescription;
	if (GetDescriptionsHelper.Cache.TryGetValue(entityType, out getDescription) == false)
	{
		MethodInfo getDescriptionInternalGeneric = getDescriptionInternalMethodInfo.MakeGenericMethod(entityType);
		getDescription = (GetDescriptionsHelper.Delegate)Delegate.CreateDelegate(
			typeof(GetDescriptionsHelper.Delegate),
			getDescriptionInternalGeneric);
	
		GetDescriptionsHelper.Cache.Add(entityType, getDescription);
	}
}

Except, this is really bad for performance, because we always lock when we try to get the value out. So we start playing with it and get this:

GetDescriptionsHelper.Delegate getDescription;
if (GetDescriptionsHelper.Cache.TryGetValue(entityType, out getDescription) == false)
{
	lock(GetDescriptionsHelper.Cache)
	{	
		MethodInfo getDescriptionInternalGeneric = getDescriptionInternalMethodInfo.MakeGenericMethod(entityType);
		getDescription = (GetDescriptionsHelper.Delegate)Delegate.CreateDelegate(
			typeof(GetDescriptionsHelper.Delegate),
			getDescriptionInternalGeneric);
	
		GetDescriptionsHelper.Cache.Add(entityType, getDescription);
	}
}

This code has a serious error in it, in the right conditions, two threads will evaluate the if at the same time, and then try to enter the lock at the same time. The end result is an exception as the second of them will try to add the entity type again.

So we come out with the doubly checked lock, like this:

GetDescriptionsHelper.Delegate getDescription;
if (GetDescriptionsHelper.Cache.TryGetValue(entityType, out getDescription) == false)
{
	lock(GetDescriptionsHelper.Cache)
	{	
		if (GetDescriptionsHelper.Cache.TryGetValue(entityType, out getDescription) )
			return;

		MethodInfo getDescriptionInternalGeneric = getDescriptionInternalMethodInfo.MakeGenericMethod(entityType);
		getDescription = (GetDescriptionsHelper.Delegate)Delegate.CreateDelegate(
			typeof(GetDescriptionsHelper.Delegate),
			getDescriptionInternalGeneric);
	
		GetDescriptionsHelper.Cache.Add(entityType, getDescription);
	}
}

Now we are handling this condition as well. But my preference of late has been to use this code in multi threaded environments.

Update: I should be wrong more often, I got some very good replies to this post. The code below happened to work by chance and luck alone, apparently. The solution above is the more correct one.

Actually, it is more complex than that. It is still possible to have readers attempt to read the Cache variable (which is of type Dictionary<Type, Delegate>) while it is being written to inside the lock. There is a potential for seriousĀ  bit of mayhem in that case.

The safe thing to do in this case would be to lock it always before access, but for things that are going to be used frequently (hot spots) this can be a problem. Reader/Writer lock is much worse in terms or performance than the usual lock statement, and ReaderWriterLockSlim is for .NET 3.5 only.

An interesting dilemma, and I apologize for misleading you earlier.

GetDescriptionsHelper.Delegate getDescription;
if (GetDescriptionsHelper.Cache.TryGetValue(entityType, out getDescription) == false)
{
	MethodInfo getDescriptionInternalGeneric = getDescriptionInternalMethodInfo.MakeGenericMethod(entityType);
	getDescription = (GetDescriptionsHelper.Delegate)Delegate.CreateDelegate(
		typeof(GetDescriptionsHelper.Delegate),
		getDescriptionInternalGeneric);

	GetDescriptionsHelper.Cache[entityType] = getDescription;
}

can you spot the main difference? We are not using locks, and we are using the indexer instead of the Add() method.

Empirical evidence suggest that using the indexer in a multi threaded environment is safe, in that it doesn't corrupt the dictionary and one of the values will remain there.

This means that assuming that the cost of creating the value isn't very high, it is just fine to have it created twice on those rare cases, the end result is after the initial flurry, we have the value cached, even if it was calculated more than once. In the long run, it doesn't matter.