3
\$\begingroup\$

In a web application I'm working on I have a class that Map objects to other (different) objects. It is implemented with something like:

public interface IMapper
{
    object Map<T>(T item);
}

public class Mapper
{
    private static ConcurrentDictionary<Type, Delegate> cache = new ConcurrentDictionary<Type, Delegate>();

    public Mapper()
    {
        // dependencies are injected in the constructor
        // not shown here for simplicity
    }

    public object Map<T>(T item)
    {
        if (cache.ContainsKey(typeof(T)))
            return (cache[typeof(T)] as Func<T, object>)(item);

        var method = typeof(Mapper).GetMethod("Map", BindingFlags.Public | BindingFlags.Instance, null, new[] { typeof(T) }, null);

        var @delegate = (Func<T, object>)Delegate.CreateDelegate(typeof(Func<T, object>), this, method);
        cache.TryAdd(typeof(T), @delegate);

        return @delegate(item);

        // I'm trying not to use this 
        //return method.Invoke(this, new object[] { item });
    }

    public object Map(Guid item) { return new object(); }
}

In order to assess what I believe should be a performance gain (compared to the .Invoke() solution) I tried to generate ten million objects (tested with strings) and then I ran the mapper for all of them and printed out the time elapsed. The results are.

  • Direct call: 1.67 s
  • Reflection: 14.09 s
  • Optimized Reflection: 2.62 s

which is not bad actually but I wonder if I'm testing this correctly.

Test code

 var guidList = new List<Guid>();
        for(int i = 0; i < 10000000; i++)
        {
            guidList.Add(Guid.NewGuid());
        }

        var mapper = new Mapper();

        var sw = new Stopwatch();
        sw.Start();
        var result = guidList.Select(_ => mapper.Map<Guid>(_)).ToList();
        sw.Stop();

(Obviously, when I tested the direct method call I changed the select lambda expression to _ => mapper.Map(_))

Am I doing it right?

On a side note: I declared the ConcurrentDictionary as ConcurrentDictionary<Type, Delegate> and so I have to "cast" the delegate part like this (cache[typeof(T)] as Func<T, object>)(item) when I'm using it. Is there a better way to do this?

\$\endgroup\$
  • 1
    \$\begingroup\$ This blog post by Jon Skeet may interest you. \$\endgroup\$ – Bruno Costa Feb 12 '16 at 14:02
4
\$\begingroup\$

Instead of using a ConcurrentDictionary as a cache, and having to cast, you can use an embedded generic static class. The static construction is lazy and threadsafe by design. To make it work you have to create and open delegate (an instance method with no "this") for the mapper function:

public class Mapper
{
    public Mapper()
    {
        // dependencies are injected in the constructor
        // not shown here for simplicity
    }

    public object Map<T>(T item)
    {
        var mapper = MapperCache<T>.Map;
        return mapper(this, item);
    }

    // example methods
    public object Map(SomeObject item) { return new object(); }

    public object Map(SomeTotallyDifferentObject item) { return new object(); }

    public object Map(Guid item) { return new object(); }

    private static class MapperCache<T>
    {
        public static readonly Func<Mapper, T, object> Map;

        static MapperCache()
        {
            var mapMethod = typeof(Mapper).GetMethod("Map", BindingFlags.Public | BindingFlags.Instance, null, new[] { typeof(T) }, null);

                Map = mapMethod != null ? (Func<Mapper, T, object>)Delegate.CreateDelegate(typeof(Func<Mapper, T, object>), mapMethod)
                                        : (mapper, item) => { throw new InvalidOperationException($"{nameof(Mapper)} cannot map from {typeof(T).Name} to object"); };
        }
    }
}

The first time you reference the static property, Map, the MapperCache<T> for the given type T will be initialized. By setting up the delegate creation in the static constructor you guarantee that it will only run once for each T. If any exceptions are thrown in the static constructor the type will fail to load and cannot be used at runtime.

The same trick is used in System.Data.DataRowExtensions (source) to cache converters.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ This approach makes you create all delegates ever used at the same time though. \$\endgroup\$ – Bruno Costa Feb 12 '16 at 14:19
  • \$\begingroup\$ @BrunoCosta they're created at first use for each type, not all at once. \$\endgroup\$ – Johnbot Feb 12 '16 at 14:20
  • \$\begingroup\$ Ah it uses a generic MapperCache class! Very clever with this use of generics :D! Even so could make the static constructor a bit tricky because you could have to check for the type of T but I don't think I would mind that \$\endgroup\$ – Bruno Costa Feb 12 '16 at 14:24
3
\$\begingroup\$

First you are defining an interface IMapper but are not using it.

Your method will be null, if no matching method is available. Delegate.CreateDelegate will therefore throw an ArgumentNullException.

Converting to a List via .ToList() takes quite some time. List has a .ForEach() method. If you only want to test performance use:

sw.Start();
guidList.ForEach(_ => mapper.Map<Guid>(_));
sw.Stop();

Also you can use dynamic, which is faster than reflection (not what you asked, but still interesting):

public object Map<T>(T item)
{
    return Map((dynamic)item);
}

(The above code will result in an infinite loop and therefore a StackOverflowException, if the corresponding method is not implemented.)

Edit: As you are interested in this: Before calling Map((dynamic)item), you should make sure, that the right method is available. Catching a StackOverflowException is impossible (in this case). You could do this by providing a default implementation

public object Map(object item)
{ ... }

Or you could use a HashSet<Type> with all implemented Types and throw a NotImplementedException if item.GetType() is not in the HashSet<Type>.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ The interface is used in other part of the programs, not very relevant here and removed all the validation code to make it simple. Your last part seems interesting, gonna check it out as soon as I can, thank you! \$\endgroup\$ – Lennox Feb 12 '16 at 15:31
  • \$\begingroup\$ @Lennox - it's a handy way to get dynamic dispatch. I.e. the best Map method will be chosen at runtime rather than compile time. \$\endgroup\$ – RobH Feb 12 '16 at 15:32
  • \$\begingroup\$ Thanks Fabian, the dynamic solution is slightly faster than the cache solution and much much simpler. \$\endgroup\$ – Lennox Feb 12 '16 at 20:26
  • \$\begingroup\$ You could also make Map<T> an explicit interface implementation, object IMapper.Map<T>(T item)..., then it won't be a candidate for the dynamic overload resolution. \$\endgroup\$ – Johnbot Feb 12 '16 at 21:50
2
\$\begingroup\$

Looks OK. Did you avoid the usual benchmark error of running this under the debugger or in Debug mode?

You avoided another common error of having too short of a benchmark. Since your measurements are above 1sec all one-time costs such as JIT disappear in the noise.

You're also measuring the performance of Select and ToList. You can avoid that with a for loop.

You can avoid the delegate casting if you push the casting into the map method. All map methods must take object and cast their argument. That should be faster because casting generic types is, I think, rather slow.

You can go a little faster if you avoid ConcurrentDictionary. Ideally, you'd replace it with a custom hash table. Dictionary has some features in it that you don't need but that cost performance.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Is this stuff you picked up along the way? Where did you learn about these performance stats? \$\endgroup\$ – Mathemats Feb 12 '16 at 0:41
  • \$\begingroup\$ I measured timings while compiled in Debug but I believe it doesn't matter much since I'm interested in relative difference and not the absolute values. Same goes with select I believe. I need the concurrent dictionary because I'm using this in a multi thread application. I didn't quite get the part about casting, would you care to expand a little more on that? \$\endgroup\$ – Lennox Feb 12 '16 at 8:48

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.