1

I wrote the following extension method to get an element from a dictionary, or null if the key isn't present:

public static TValue ItemOrNull<TKey, TValue>(this IDictionary<TKey, TValue> dict, TKey key)
{
    try
    {
        return dict[key];
    }
    catch (KeyNotFoundException ex)
    {
        return default(TValue);
    }
}

I noticed that my program was running very slowly so I tracked down the problem to this extension method using a high precision timer class. I get similar results ~100 times in a row:

DebugTimer.ResetTimer();
    dict1.ItemOrNull(key);
    dict2.ItemOrNull(key);
DebugTimer.StopTimer();

takes about 110,000,000 ticks (more than 0.03 seconds on my processor). While the more verbose version:

DebugTimer.ResetTimer();
    if (dict1.ContainsKey(key))
        y = dict1[key];
    if (dict2.ContainsKey(key))
        z = dict2[key];
DebugTimer.StopTimer();
MessageBox.Show(y.ToString(), z.ToString()) // so the compiler doesn't optimize away y and z

takes about 6,000 ticks (less than 0.000002 seconds).

Is it clear to anyone why my extension method version is taking more than 4 orders of magnitude longer than the verbose version?

Community
  • 1
  • 1
Rag
  • 6,405
  • 2
  • 31
  • 38
  • 6
    That is because you don't use exceptions in the verbose version. Make an extension method that uses `ContainsKey` and compare. – GSerg Aug 18 '11 at 16:35
  • 2
    Exceptions are very expensive. – drharris Aug 18 '11 at 16:36
  • you let the excpetion basically handle the check for existence and in case of failure you construct the default... nothing of this happens in the version without the extension method... for doing it differently and esp. "doing more" there is some cost/overhead... – Yahia Aug 18 '11 at 16:37
  • @GSerg that's it, wow. It still takes twice as long but that's better than 18 thousand times as long. Submit it as an answer an I'll accept – Rag Aug 18 '11 at 16:38
  • 3
    @Brian: Are you running these times *in a debugger*? And is the code path you are timing one in which an exception is produced? If so then I would totally expect that. Think about what has to happen every time an exception is thrown when the debugger is attached. The program has to halt while the debugger figures out whether this is a first-chance exception that needs to be debugged or not! If that's the case I am surprised it is only 18 thousand times slower. This is just one reason why people say **do not use exceptions for mainline control flow.** – Eric Lippert Aug 18 '11 at 16:44
  • 2
    @Brian: So, two lessons here. First, never use exceptions for mainline control flow if you can possibly avoid it. Second, never do performance timing while the debugger is running; the jitter deliberately de-optimizes your program when running in the debugger so that it is easier to debug, and the debugger can introduce other overheads into the CLR. – Eric Lippert Aug 18 '11 at 16:53
  • 2
    Additionally, were you really benchmarking a single iteration? Don't do that. Perform an operation hundreds, thousands or millions of times in a row, and benchmark that - it reduces the impact of timer discrepancies. – Jon Skeet Aug 18 '11 at 16:55

5 Answers5

15

Don't catch exceptions for flow control - it's not just that it can cause performance problems (although they're not nearly as bad as most people think - as Eric says, most people's fear of exception performance comes from using the debugger). It's more about the logical nature of exceptions. Personally I wouldn't use exceptions in this way even if they were essentially free.

Has anything bad happened here? Is it in any way invalid for the user to be asking for this key's value? Absolutely not - the whole purpose of the method is to provide a default. It's not exceptional for the key to be absent - so you should look for a way of working without exceptions.

Now Dictionary<,> already has a method to let you fetch a value if the key exists and let you know whether or not it was found: TryGetValue. 

public static TValue GetValueOrDefault<TKey, TValue>(
    this IDictionary<TKey, TValue> dict,
    TKey key)
{
    TValue ret;
    // We don't care about the return value - we want default(TValue)
    // if it returns false anyway!
    dict.TryGetValue(key, out ret);
    return ret;
}

Extension methods are just compiled into regular static method calls and thus have no performance difference to them.

You might also want to add an overload allowing the user to express the default value to return if the key wasn't found:

public static TValue GetValueOrDefault<TKey, TValue>(
    this IDictionary<TKey, TValue> dict,
    TKey key, TValue defaultValue)
{
    TValue ret;
    return dict.TryGetValue(key, out value) ? ret : defaultValue;
}

I've adjusted the name of your method to match TryGetValue, by the way. Obviously you don't have to follow that - it's just a suggestion.

dlev
  • 48,024
  • 5
  • 125
  • 132
Jon Skeet
  • 1,421,763
  • 867
  • 9,128
  • 9,194
  • Apologies for the "late" edit by the way folks - the answer got in *just* before my mobile connection went down :( – Jon Skeet Aug 18 '11 at 16:49
3

That is because you don't use exceptions in the verbose version. Make an extension method that uses ContainsKey and compare.

To clarify: exceptions can be very slow. It's one of the reasons to not rely on them to control normal workflow. They are for situations that are actually unexpected and unwanted.

For FW3.5+, use TryGetValue as others suggested.

GSerg
  • 76,472
  • 17
  • 159
  • 346
  • TryGetValue has been in `Dictionary<,>` since .NET 2. And exceptions aren't *that* slow normally - they're not nearly the boogeyman they're made out to be. They're *much* slower in debug, which is where devs tend to get scared. They shouldn't be used for flow control because it's simply an inappropriate use of them - it wouldn't be appropriate (IMO) even if they were free. – Jon Skeet Aug 18 '11 at 16:53
  • Sorry GSerg, it was an injustice to not accept jon skeet's considered answer – Rag Aug 18 '11 at 16:56
1

There are two issues here:

  1. Your extension method isn't using the same code as your test. If you switch it around to use the same code (or better yet, Dictionary.TryGetValue), the timings will get much closer to each other.
  2. I suspect you're doing your timings in debug mode. Do your timings in a full release build outside of Visual Studio. Timing in Debug mode (or even in the VS host process in Release) is very inaccurate, as the hosting process really slows down a lot of code. Extension methods actually have no extra overhead (they're compiled to normal static method calls) - but method calls in general have an exaggerated overhead when run inside of Visual Studio's debugger, so this will appear worse than reality.
Reed Copsey
  • 554,122
  • 78
  • 1,158
  • 1,373
0

Dictionary is a hash table so that the Dictionary.Contains runs fast (call here one step), it costs simple nothing, because it checks directly the memory for the value related to the key. [key-> hash and then check the memory]

But the first function the try part already make a check ! before returning the value key is hashed and the memory is checked for it. Shortly it already includes steps of Dictionary.Contains.

The reason your code runs slow the " try " block which includes steps of Dictionary.Contains with a missing bool value, that causes to fall in catch block. These wastes your time. The answer above are correct and more detailed.

icaptan
  • 1,495
  • 1
  • 16
  • 36
0

You just got the time values to add to my professors statement, "Exception handling is a costly process! Dont write all the logic in Catch block, try to code such that you have checks that can avoid an exception." Use exception handling for unexpected exception not for the obvious that when the item is not in the dictionary its gonna crash!

ioWint
  • 1,609
  • 3
  • 16
  • 34