Which piece of code is more performant?

Question

I have some code i'm revewing, which is used to convert some text into an MD5 Hash. Works great. It's used to create an MD5Hhash for a gravatar avatar. Here it is :-

static MD5CryptoServiceProvider md5CryptoServiceProvider = null;

public static string ToMD5Hash(this string value)
{
    //creating only when needed
    if (md5CryptoServiceProvider == null)
    {
        md5CryptoServiceProvider = new MD5CryptoServiceProvider();
    }

    byte[] newdata = Encoding.Default.GetBytes(value);
    byte[] encrypted = md5CryptoServiceProvider.ComputeHash(newdata);
    return BitConverter.ToString(encrypted).Replace("-", "").ToLower();
}

Notice how we create a MD5CryptoServiceProvider the first time this method is called? (lets not worry about race-conditions here, for simplicity).

I was wondering, is it more computationally expensive if i change the lines that are used to create the provider, to this...

using(var md5CryptoServiceProvider = new MD5CryptoServiceProvider())
{
    ... snip snip snip ....
}

Now, how is this method used/consumed? Well, imagine it's the home page of StackOverflow -> for each post, generate an md5 hash of the user, so we can generate their gravatar url. So the view could call this method a few dozen times.

Without trying to waste too much time stressing over premature optimzation, etc ... which would be better?

Kellene

Answer 1

I'd be more interested in the thread-safefy... MSDN doesn't (unless I missed it) say that MD5CryptoServiceProvider is thread-safe, so IMO the best option is to have one per call...

It doesn't matter how quickly you can get the wrong answer ;-p

What you probably don't want to do (to fix the thread-safety issue) is have a static instance and lock around it... that'll serialize all your crypto code, when it could run in parallel on different requests.

Peaceable

So you're suggesting for him to apply the "Using" statement, right? – Frey

I'm suggesting he use the second example that already includes the "using" statement, if that is what you mean. – Peaceable

If I static + lock, all that will do will be to block other parallel calls .. when i don't need to lock, right? It will work, but be waaay slower that it could be. (I'm not intending to do that, but just trying to understand your answer 100%). – Kellene

Re "when I don't need to lock"; with separate providers per call you don't need to lock; with one shared provider it looks like you might, but it unclear. Re "way slower" - that depends how long it takes to run ;-p It'll certainly introduce a potential pinch-point - but how critical that is would only show up through profiling. The other alternative would be to mark the field [ThreadStatic] - not sure if that is a great idea, but it would work, I suppose. – Peaceable

Answer 2

Test it and time it. The first is more performant, but it is probably not significant.

using System;
using System.Diagnostics;
using System.Security.Cryptography;
using System.Text;

namespace ConsoleApplication11
{
    class Program
    {
        static void Main(string[] args)
        {
            Stopwatch timer=new Stopwatch();
            int iterations = 100000;
            timer.Start();
            for (int i = 0; i < iterations; i++)
            {
                string s = "test" + i;
                string t=s.ToMd5Hash0();
            }
            timer.Stop();
            Console.WriteLine(timer.ElapsedTicks);

            timer.Reset();
            timer.Start();
            for (int i = 0; i < iterations; i++)
            {
                string s = "test" + i;
                string t = s.ToMd5Hash1();
            }
            timer.Stop();
            Console.WriteLine(timer.ElapsedTicks);

            Console.ReadKey();
        }
    }
    public static class Md5Factory
    {
        private static MD5CryptoServiceProvider md5CryptoServiceProvider;
        public static string ToMd5Hash0(this string value)
        {
            if (md5CryptoServiceProvider == null)
            {
                md5CryptoServiceProvider = new MD5CryptoServiceProvider();
            }
            byte[] newData = Encoding.Default.GetBytes(value);
            byte[] encrypted = md5CryptoServiceProvider.ComputeHash(newData);
            return BitConverter.ToString(encrypted).Replace("-", "").ToLower();
        }
        public static string ToMd5Hash1(this string value)
        {
            using (var provider = new MD5CryptoServiceProvider())
            {
                byte[] newData = Encoding.Default.GetBytes(value);
                byte[] encrypted = provider.ComputeHash(newData);
                return BitConverter.ToString(encrypted).Replace("-", "").ToLower();
            }
        }
    }
}

Conflagration

Answer 3

The existing code I would expect to be very slightly faster, as it saves reconstructing the MD5CryptoServiceProvider on every call, but I'd also expect the time to be dominated by the call to ComputeHash().

Langford

Answer 4

I personnally can't see why it would matter either way, think of the lashing of other code that has to run just to generate a page. Its six of one and half a dozen of the other, the savings either way will be tiny.

Pearliepearline