views:

1053

answers:

8

First of all, a disclaimer: I have experience in other languages, but am still learning the subtleties of C#

On to the problem... I am looking at some code, which uses the try/catch blocks in a way that concerns me. When a parsing routine is called, rather than return an error code, the programmer used the following logic

catch (TclException e) {
  throw new TclRuntimeError("unexpected TclException: " + e.Message,e);
}

This is caught by the caller, which throws the same error ...
... which is caught by the caller, which throws the same error ...
..... which is caught by the caller, which throws the same error ...

back up about 6 levels.

Am I right in thinking all these catch/throw blocks are causing a performance problem, or is this a reasonable implementation under C#?

+13  A: 

It's a bad design under any language.

Exception are designed to be caught at the level which you can deal with them. Catching an exception, just to throw it again is just a pointless waste of time (it also causes you to lose valuable information about the location of the original error).

Apparently, the guy who wrote that code, used to use error codes, and then switched to exceptions without actually understanding how they work. Exceptions automatically "bubble up" the stack if there is no catch at one level.

Also, note that exceptions are for exceptional occurrences. Thing that should never happen. They should not be used in place to normal validity checking (i.e., don't catch a divide-by-zero exception; check to see if the divisor is zero beforehand).

James Curran
None of the original information is lost in the example provided in the question, since the original exception is passed as the innerException argument to the TclRuntimeError constructor.
Justice
That is true. However, the actual exception is buried six levels deep in side the exception thrown in the users face.
SealedSun
You should generally only wrap exceptions at abstraction points, and even then only if you will be providing additional information about the error. For example wrapping a 3rd party component and wrapping any exceptions that are odd (IndexOutOfBounds->KeyNotFound)
Guvante
Exceptions are not for things that NEVER happen, but they are for rare error conditions. They are slower than normal control flow, but a few exceptions per second doesn't cause a performance problem (except maybe if you're running in a debugger.)
Qwertie
+4  A: 

Throw (rather than catch) is expensive.

Don't put a catch block in unless you are going to do something useful (i.e. convert to a more useful exception, handle the error).

Just rethrowing the exception (throw statement without argument) or, even worse, throw the same object as just caught is definitely the wrong thing.

EDIT: To avoid ambiguity:

Rethrow:

catch (SomeException) {
  throw;
}

Create exception from previous exception object, where all the runtime provided state (notably stack trace) is overwritten:

catch (SomeException e) {
  throw e;
}

The latter case is a pointless way to throw away information about the exception. and without anything preceding the throw in the catch block is doubly pointless. It can be worse:

catch (SomeException e) {
  throw new SomeException(e.Message);
}

which loses almost all the useful state information e contained (including what was originally thrown.)

Richard
Does something useful include logging the error before rethrowing, or would this be better taken care of elsewhere (e.g on Application Error)?
Nick
Rethrowing is not expensive, as no Exception object has to be created (which involves unwinding the stack)
SealedSun
But at every level, aren't they unwinding the stack?
@devinb: In this case they are, since they are throwing new exceptions. If they simply used "throw;", they wouldn't unwind the stack at every level. When logging, that should be used.
configurator
not so, the catch handler has to unwind in order to get the data out - including reading the metadata for the call stack. That has to happen in every catch otherwise you wouldn't be able to print out the callstack.
gbjbaanb
@configurator: Clearly this all needs to be written. But as a first step, a minor change could be as simple as changing catch (TclException e){throw e;} to catch (TclException e){throw;}
Noah
Expanded to be clearer about bad approaches.
Richard
When "throw" is passed an Exception instance, it adds new information to it. Programmers incorrectly assume "throw" can distinguish between a new Exception instance and a used Exception instance. Since no distinction is made, the only way to rethrow is to not pass throw an Exception instance at all.
Triynko
+1  A: 

Try / Catch / Throw are slow - a better implementation would be to check the value before catching it, but if you absolutly can't continue, you are better off throwing and catching only when it counts. Checking and logging is more efficient otherwise.

Fry
The try-block itself has little to no overhead. It's the exceptional case (throw / catch) that slows the system down.
SealedSun
Right - The point was in his example, using a try/catch was slower because they resulted in a thrown exception, that might have been avoided using a check then performing another action instead of continuously throwing.
Fry
+6  A: 

According to msdn:Performance Tips and Tricks you can use try and catch without any performance problem until the real throw occure.

Avram
+2  A: 

In general, throwing an exception is costly in .NET. Simply having a try/catch/finally block is not. So, yes, the existing code is bad from a performance perspective because when it does throw, it throws 5-6 bloated exceptions without adding any value over simply letting the original exception naturally bubble up 5-6 stack frames.

Worse yet, the existing code is really bad from a design perspective. One of the main benefits of exception handling (as compared to returning error codes) is that you don't need to check for exceptions/return codes everywhere (in the call stack). You need only catch them at the small number of places you actually want to handle them. Ignoring an exception (unlike ignoring a return code) doesn't ignore or hide problem. It just means it'll be handled higher up the call stack.

C. Dragon 76
Clearly this all needs to be written. But as a first step, could a minor improvement be as simple as changing catch (TclException e){throw e;} to catch (TclException e){throw;} to allow the exception to bubble up the stack?
Noah
Yes, but it's even easier than that. Simply remove the entire catch(TclException e) block. If you don't catch the exception, it will bubble up the call stack all by itself.
C. Dragon 76
A: 

If every layer of the stack just rethrows the same type with the same information, adding nothing new, then it's completely absurd.

If it was happening at the boundary between independently developed libraries, it's understandable. Sometimes a library author wants to control what exceptions escape from their library, so that they can change their implementation later without having to figure out how to simulate the previous version's exception-throwing behaviour.

It's generally a bad idea to catch and re-throw under any circumstances without a very good reason. This is because as soon as a catch block is found, all the finally blocks between the throw and the catch are executed. This should only happen if the exception can be recovered from. It's okay in this case because a specific type is being caught, so the author of the code (hopefully) knows that they can safely undo any of their own internal state changes in response to that specific exception type.

So those try/catch blocks potentially have a cost at design time - they make the program messier. But at runtime they will only impose a significant cost when the exception is thrown, because the transmission of the exception up the stack has been made more complicated.

Daniel Earwicker
A: 

Exceptions are slow, try not to use them.

See the answer I gave here.

Basically, Chris Brumme (who was on the CLR team) said they are implemented as SEH exceptions, so you take a big hit when they are thrown, have to suffer the penalties as they bubble through the OS stack. Its a truly excellent article and goes in depth in what happens when you throw an exception. eg.:


Of course, the biggest cost of exceptions is when you actually throw one. I’ll return to this near the end of the blog.


Performance. Exceptions have a direct cost when you actually throw and catch an exception. They may also have an indirect cost associated with pushing handlers on method entry. And they can often have an insidious cost by restricting codegen opportunities.


However, there is a serious long term performance problem with exceptions and this must be factored into your decision.

Consider some of the things that happen when you throw an exception:

  • Grab a stack trace by interpreting metadata emitted by the compiler to guide our stack unwind.

  • Run through a chain of handlers up the stack, calling each handler twice.

  • Compensate for mismatches between SEH, C++ and managed exceptions.

  • Allocate a managed Exception instance and run its constructor. Most likely, this involves looking up resources for the various error messages.

  • Probably take a trip through the OS kernel. Often take a hardware exception.

  • Notify any attached debuggers, profilers, vectored exception handlers and other interested parties.

This is light years away from returning a -1 from your function call. Exceptions are inherently non-local, and if there’s an obvious and enduring trend for today’s architectures, it’s that you must remain local for good performance.

Some people will claim that exceptions are not a problem, have no performance issues and are generally a good thing. These people get lots of popular votes, but they are just plain wrong. I've seen Microsoft staff make the same claims (usually with the technical knowledge usually reserved for the marketing department), but the bottom line from the horse's mouth is to use them sparingly.

The old adage, exceptions should only be used for exceptional circumstances, is as true for C# as it is for any other language.

gbjbaanb
"Try not to use them"? That's a pretty gross misrepresentation of Brumme's advice.
Daniel Earwicker
no, that's my advice, and is a generalisation to make people think about what they're doing. Chris says to use them, but very sparingly: "...By ensuring that error cases are exceedingly rare"
gbjbaanb
So..... you are suggesting to check the values rather than just use exceptions all "willy-nilly", so instead of "Try not to use exceptions", your advise is more "Try not to create exceptions"?
Fry
exactly, try not to use exceptions. I didn't say "try not to use exception handling".
gbjbaanb
I would reformulate "Exceptions are slow, try not to use them." to "Code that abuses exceptions sucks, don't do it. Don't use exceptions for normal program flow. For truly exceptional situations, who cares about speed?"
Daniel Daranas
+1  A: 

That's not terrible in and of itself, but one should really watch the nesting.

The acceptable use case is as such:

I am a low-ish component that may encounter a number of different errors however my consumer is only interested in a specific type of exception. Therefore I may do this:

catch(IOException ex)
{
    throw new PlatformException("some additional context", ex);
}

Now this allows the consumer to do:

try
{
    component.TryThing();
}
catch(PlatformException ex)
{
   // handle error
}

Yes, I know some people will say but the consumer should catch the IOException but this depends on how abstract the consuming code actually is. What if the Impl is saving something to disk and the consumer has no valid reason to think their operation will touch the disk? In such a scenario it would make no sense to put this exception handling in the consuming code.

What we are generally trying to avoid by using this pattern is placing a "catch-all" exception handler in business logic code because we want to find out all the possible types of exceptions as they might lead to a more fundamental problem that needs to be investigated. If we don't catch, it bubbles up, hits the "top top" level handler and should halt the app from going any further. This means the customer will report that exception and you will get a chance to look into it. This is important when you're trying to build robust software. You need to find all these error cases and write specific code to handle them.

What isn't very pretty is nesting these an excessive amount and that is the problem you should solve with this code.

As another poster stated exceptions are for reasonably exceptional behaviour but don't take this too far. Basically the code should express the "normal" operation and exceptions should handle potential problems you may come across.

In terms of performance exceptions are fine, you will get horrible results if you perf test with a debugger to an embedded device but in release without a debugger they're actually pretty quick.

The main thing people forget when discussing the performance of exceptions is that in error cases everything slows down anyway because the user has encountered a problem. Do we really care about speed when the network is down and the user can't save their work? I very much doubt getting the error report back to the user a few ms quicker is going to make a difference.

The main guideline to remember when discussing exceptions is that Exceptions should not occur within the normal application flow (normal meaning no errors). Everything else stems from that statement.

In the exact example you give I'm unsure though. It seems to me that no benefit is really gained from wrapping what seems to be a generic tcl exception in another generic sounding tcl exception. If anything I would suggest tracking down the original creator of the code and learning whether there is any particular logic behind his thinking. Chances are though that you could just kill the catch.

Quibblesome
often you do care about performance - in a server, if you throw an exception only 1% of the time, but you're taking 100 calls per second, over 100 users - that's 100 exceptions per second! That can be a serious performance hit sometimes.
gbjbaanb
Dude if each of your 100 calls is resulting in an exception your code or your server is fucked and you have bigger problems to deal with. As I said exceptions should not occur in normal program operation.
Quibblesome