17

I often find myself writing a code that looks something like this:

if(a == nullptr) throw std::runtime_error("error at " __FILE__ ":" S__LINE__);

Should I prefer handling errors with if unlikely?

if unlikely(a == nullptr) throw std::runtime_error("error at " __FILE__ ":" S__LINE__);

Will the compiler automatically deduce which part of the code should be cached or is this an actually useful thing to do? Why do I not see many people handling errors like this?

Adam Hunyadi
  • 1,890
  • 16
  • 32
  • 5
    What actually do you think `unlikely()` is for, if not for exactly this? – user207421 Jun 26 '17 at 09:50
  • 29
    This question is lacking context. I asked my friend DuckDuckGo so that others don't have to: `likely`/`unlikely` are Linux kernel macros that wrap GCC's `__builtin_expect`. In other words, the question is specific to Linux kernel programming, or to those willing to `#define` their own non-portable macros of the same name. – Arne Vogel Jun 26 '17 at 10:05
  • 6
    Also, as such, the c++ and c++1z tags seem inappropriate because C++ is not supported in Linux [kernel] programming, which is written mostly in C and inline assembler. (Although there is at least one C++-enabled fork.) – Arne Vogel Jun 26 '17 at 11:45
  • FWIW, this is what Cython does in its code generation. – Davidmh Jun 26 '17 at 12:21
  • Without indications, gcc already guesses that `a` is probably not 0 (around 80%) and that a branch with throw is very unlikely (less than 1%). An explicit annotation does not add much to this. You can pass `-fdump-tree-optimized` to see the estimated probabilities in a dump file. – Marc Glisse Jul 08 '17 at 07:14

4 Answers4

22

Yes you can do that. But even better is to move the throw to a separate function, and mark it with __attribute__((cold, noreturn)). This will remove the need to say unlikely() at each call site, and may improve code generation by moving the exception throwing logic entirely outside the happy path, improving instruction cache efficiency and inlining possibilities.

If you prefer to use unlikely() for semantic notation (to make the code easier to read), that's fine too, but it isn't optimal by itself.

John Zwinck
  • 239,568
  • 38
  • 324
  • 436
  • 1
    +1, I'm curious however, how will moving the exception throwing logic outside the happy path improve cache efficiency? Is that in reference to how much code is stored in memory, and the fact that reducing the amount of code stored improves cache efficiency? – Curious Jun 26 '17 at 09:49
  • Interesting. Does `cold` prevent inlining? – Vittorio Romeo Jun 26 '17 at 09:49
  • 2
    @Curious: The exception throwing logic takes space in the generated code. Even in the best case, if that code is shifted to the "end" of a function so it is never encountered, it will still contribute to the total size of the function, which can inhibit inlining. Therefore, small functions that can throw might not be inlined, which means more instructions get executed. Moving it to `cold` keeps it totally out of the way unless it is needed. – John Zwinck Jun 26 '17 at 09:56
  • But don't you think the compiler will change the layout of code like this anyway to be more cache efficient? – Curious Jun 26 '17 at 09:57
  • 3
    @Curious: I too wondered if a sufficiently smart compiler could do this automatically, so I tested it (a long time ago). GCC does not, and it's not entirely clear that it could. Here's one example: https://godbolt.org/g/HEdwFF - not a perfect one, but even in this simple case you can see how `g1()` has more instructions in the happy path than `g2()`. – John Zwinck Jun 26 '17 at 10:07
  • @JohnZwinck that is surprising, do you think these flags have something to do with that? `-march=haswell --param inline-unit-growth=5` I am not sure what they do :( – Curious Jun 26 '17 at 10:08
  • @Curious: Those flags aren't very important - you can remove them. – John Zwinck Jun 26 '17 at 10:09
  • @JohnZwinck I had hoped gcc does optimize code like that :( – Curious Jun 26 '17 at 10:10
9

Should I use "if unlikely" for hard crashing errors?

For cases like that I'd prefer moving code that throws to a standalone extern function that's marked as noreturn. This way, your actual code isn't "polluted" with lots of exception-related code (or whatever your "hard crashing" code). Contrary to the accepted answer, you don't need to mark it as cold, but you really need noreturn to make compiler not to try generating code to preserve registers or whatever state and essentially assume that after going there there is no way back.

For example, if you write code this way:

#include <stdexcept>

#define _STR(x) #x
#define STR(x) _STR(x)

void test(const char* a)
{
    if(a == nullptr)
        throw std::runtime_error("error at " __FILE__ ":" STR(__LINE__));
}

compiler will generate lots of instructions that deal with constructing and throwing this exception. You also introduce dependency on std::runtime_error. Check out how generated code will look like if you have just three checks like that in your test function:

enter image description here

First improvement: to move it to a standalone function:

void my_runtime_error(const char* message);

#define _STR(x) #x
#define STR(x) _STR(x)

void test(const char* a)
{
    if (a == nullptr)
        my_runtime_error("error at " __FILE__ ":" STR(__LINE__));
}

this way you avoid generating all that exception related code inside your function. Right away generated instructions become simpler and cleaner and reduce effect on the instructions that are generated by your actual code where you perform checks: enter image description here

There is still room for improvement. Since you know that your my_runtime_error won't return you should let the compiler know about it, so that it wouldn't need to preserve registers before calling my_runtime_error:

#if defined(_MSC_VER)
#define NORETURN __declspec(noreturn)
#else
#define NORETURN __attribute__((__noreturn__))
#endif

void NORETURN my_runtime_error(const char* message);
...

When you use it multiple times in your code you can see that generated code is much smaller and reduces effect on instructions that are generated by your actual code:

enter image description here

As you can see, this way compiler doesn't need to preserve registers before calling your my_runtime_error.

I would also suggest against concatenating error strings with __FILE__ and __LINE__ into monolithic error message strings. Pass them as standalone parameters and simply make a macro that passes them along!

void NORETURN my_runtime_error(const char* message, const char* file, int line);
#define MY_ERROR(msg) my_runtime_error(msg, __FILE__, __LINE__)

void test(const char* a)
{
    if (a == nullptr)
        MY_ERROR("error");
    if (a[0] == 'a')
        MY_ERROR("first letter is 'a'");
    if (a[0] == 'b')
        MY_ERROR("first letter is 'b'");
}

It may seem like there is more code generated per each my_runtime_error call (2 more instructions in case of x64 build), but the total size is actually smaller, as the saved size on constant strings is way larger than the extra code size.

Also, note that these code examples are good for showing benefit of making your "hard crashing" function an extern. Need for noreturn becomes more obvious in real code, for example:

#include <math.h>

#if defined(_MSC_VER)
#define NORETURN __declspec(noreturn)
#else
#define NORETURN __attribute__((noreturn))
#endif

void NORETURN my_runtime_error(const char* message, const char* file, int line);
#define MY_ERROR(msg) my_runtime_error(msg, __FILE__, __LINE__)

double test(double x)
{
    int i = floor(x);
    if (i < 10)
        MY_ERROR("error!");
    return 1.0*sqrt(i);
}

Generated assembly: enter image description here

Try to remove NORETURN, or change __attribute__((noreturn)) to __attribute__((cold)) and you'll see completely different generated assembly! enter image description here

As a last point (which is obvious IMO and was omitted). You need to define your my_runtime_error function in some cpp file. Since it's going to be one copy only, you can put whatever code you want in this function.

void NORETURN my_runtime_error(const char* message, const char* file, int line)
{
    // you can log the message over network,
    // save it to a file and finally you can throw it an error:
    std::string msg = message;
    msg += " at ";
    msg += file;
    msg += ":";
    msg += std::to_string(line);
    throw std::runtime_error(msg);
}

One more point: clang actually recognizes that this type of function would benefit from noreturn and warns about it if -Wmissing-noreturn warning was enabled:

warning: function 'my_runtime_error' could be declared with attribute 'noreturn' [-Wmissing-noreturn] { ^

Pavel P
  • 15,789
  • 11
  • 79
  • 128
  • Thanks for mentioning `noreturn` - I remembered it after I wrote my answer and have updated it now. – John Zwinck Jun 27 '17 at 02:08
  • @JohnZwinck Yes, it really needs noreturn mostly if the function doesn't return. – Pavel P Jun 27 '17 at 02:14
  • This is quite a definitive guide, Pavel! Great explanation. I have a lot of questions about this approach though: **1. )** Why should I define my own exception for this case? That just decreases the construction cost of the exception(?) which is not something I would want to optimize away. **2. )** Why would my custom except be more efficient than `std::runtime_error`? **3. )** If my goal is absolute performance, does this mean that I should extract all my throws into noreturn/cold functions? Why aren't throws (that are not handled inside the function) cold by default? – Adam Hunyadi Jun 27 '17 at 07:46
  • **4. )** Should I generate separate to-crash-check-throw (for example, prefixed with c_) for my functions, or should I strive to have a general set of checkers. For example, I could make a template function that takes `__PRETTY_FUNCTION__` (?) and `__LINE__` as template arguments? **5. )** Should I prefer noreturn over cold? Should I use both? What I'm most confused about is why I need the wrapper around the `std::runtime error`. Shouldn't the definition of `MY_ERROR` be something like `{ throw std::runtime_error(msg); }`? – Adam Hunyadi Jun 27 '17 at 07:49
  • @AdamHunyadi you don't need to define your own exception class or anything like that. The two most important points are: 1) hide exception code inside some function (I used `my_runtime_error` for example), 2) mark that function as `noreturn` to help compiler produce better code. Internally, inside `my_runtime_error` you can do whatever you want, and throw any exceptions you like, but this function cannot return – Pavel P Jun 27 '17 at 09:27
5

It depends.

First of all, you can definitely do this, and this will likely (pun intended) not harm the performance of your application. But please note that likely/unlikely attributes are compiler-specific, and should be decorated accordingly.

Secondly, if you want a performance gain, the outcome will depend on the target platform (and corresponding compiler backend). If we're talking about the 'default' x86 architecture, you will not get much of a profit on modern chips - the only change these attributes will produce is a change in the code layout (unlike earlier times when x86 supported software branch prediction). For small branches (like your example), it will have very little effect on cache utilization and/or front-end latencies.

UPDATE:

Will the compiler automatically deduce which part of the code should be cached or is this an actually useful thing to do?

This is actually a very wide and complicated topic. What will compiler do depends on the particular compiler, its backend (target architecture) and compilation options. Again, for x86, here's the following rule (taken from Intel® 64 and IA-32 Architectures Optimization Reference Manual):

Assembly/Compiler Coding Rule 3. (M impact, H generality) Arrange code to be consistent with the static branch prediction algorithm: make the fall-through code following a conditional branch be the likely target for a branch with a forward target, and make the fall-through code following a conditional branch be the unlikely target for a branch with a backward target.

As far as I'm aware, that's the only thing that's left from static branch prediction in modern x86, and likely/unlikely attributes might only be used to "overwrite" this default behaviour.

iehrlich
  • 3,572
  • 4
  • 34
  • 43
  • Why would it need to override? It just needs to pick which branch is first. – Yakk - Adam Nevraumont Jun 26 '17 at 10:08
  • If likely/unlikely attribute overrides default branch layout, it is applied (at least in GCC). The reason for such behaviour is more laconic C/C++ code layout - simple branch inversion might cause minor performance drawback otherwise. – iehrlich Jun 26 '17 at 11:01
3

Since you're "crashing hard" anyways, I'd go with

#include <cassert>

...
assert(a != nullptr);

This is compiler-independent, should give you close to optimal performance, gives you a breakpoint when running in a debugger, generates a core dump when not in a debugger, and can be disabled by setting the NDEBUG preprocessor symbol, which many build systems do by default for release builds.

Simon Richter
  • 28,572
  • 1
  • 42
  • 64
  • 1
    Assert is by no means an equivalent to exceptions. Also, how are asserts compiler dependent? – iehrlich Jun 26 '17 at 17:47
  • Asserts are better than exceptions for this use case, because they do not destroy the stack -- and they are compiler *in*dependent, unlike `__builtin_unlikely()`. – Simon Richter Jun 26 '17 at 21:22
  • So does this mean that your advice is to *not* define DNDEBUG in release builds? – iehrlich Jun 27 '17 at 10:17
  • Release builds should be built with `-DNDEBUG`, because these "fail fast and hard" tests will always lead to data loss for the user. If you want your program to have at least somewhat defined behaviour for internal inconsistencies, you need explicit code for that. – Simon Richter Jun 27 '17 at 12:08