Caching compiled lambda expressions

Question

I need to get some info passed as a lambda expression to some methods. Basically, it's a way to add information to a database query. A simple example would be:

companyData.GetAll(
   where => "SomeField = @SOMEFIELD",
   order => "Id",
   SOMEFIELD => new Parameter {DbType = DbType.String, Value = "some value"}
)

It's working pretty well, except for the fact that I need to call LambdaExpression.Compile to get the parameter object, which have a big perfomance impact.

To get faster results, I've come with this naive caching test:

class ExpressionCache<T,U>
{

    private static ExpressionCache<T, U> instance;
    public static ExpressionCache<T, U> Instance
    {
     get
     {
      if (instance == null) {
       instance = new ExpressionCache<T, U>();
      }
      return instance;
     }
    }

    private ExpressionCache() { }

    private Dictionary<string, Func<T, U>> cache = new Dictionary<string, Func<T, U>>();

    public Func<T, U> Get(Expression<Func<T, U>> expression)
    {
     string key = expression.Body.ToString();
     Func<T,U> func;
     if (cache.TryGetValue(key, out func)) {
      return func;
     }
     func = expression.Compile();
     cache.Add(key, func);
     return func;
    }
}

This class made a huge difference: from about 35000 milliseconds on 10000 iterations to about 700.

Now comes the question: what kind of problem I will run into using the expression body as the key for the dictionary?

Answer 1

Lambda expressions can create closures. When that happens, the expression body doesn't encompass everything that goes into the resulting code. You could be missing potentially important local variables that are included.

Answer 2

It's not clear to me why you need expression trees at all if you're compiling them down to delegates. Why not just use delegates to start with and get the compiler to convert the lambda expression into a delegate instead of into an expression tree?

As for using the body of the string - you may get into odd cases where you use different types but the same property names. Given that you're already using the types as generic type parameters, however, that's probably not an issue... I should think it'll work, although I would like to swear to it.

Oh, and your singleton cache isn't thread-safe by the way. I would suggest that you initialize the instance variable in a static initializer. This leads to simpler code as well as it being safer...

private static readonly ExpressionCache<T, U> instance
     = new ExpressionCache<T, U>();
public static ExpressionCache<T, U> Instance { get { return instance; } }