More Micro-optimizations

Here's another micro-optimization that I'm not very fond of. I should note that we don't really obsess about these things too much internally - in general we have established coding practices and only occasionally debate a few debatable things, usually on the fringes. But they do make reasonable blog fodder and I'm trying to cover ones that haven't been debated ad nauseam.

 HRESULT DoSomething(HWND hwndParent, BOOL fFoo, BOOL fBar)
{
    HRESULT hr;

    if (hwndParent != NULL)
    {
        if (fFoo)
        {
            hr = DoOperationWithFoo(hwndParent);
        }
        else if (fBar)
        {
            hr = DoOperationWithBar(hwndParent);
        }
    }

    return hr;
}

Catch the bug? This one's so obvious I'm not going to wait a day: if "hwndParent" is NULL then "hr" is left uninitialized. The micro-optimization here is that not initializing "hr" up front potentially saves a few instructions. The argument is that code size is one of the most critical factors for application performance on a heavily multitasking modern desktop (which is true; nearly all of our code is compiled for minimum size rather than maximum performance) and that this helps because it all adds up. The fix for the people who do not prefer initializing up front is to add "else" clauses to set "hr" to the correct error values. You may guess that with that fix you end up with a similar total number of instructions (or even more in this example), but in many cases initializing up front is certainly redundant.

The counter argument, of course, is that the risk of using uninitialized variables far outweighs the minor performance increase. This is the school of thought that I subscribe to.

The counter-counter argument is that modern compilers are likely to pick this up, and even if they don't our more advanced static analysis tools will.

The counter-counter-counter argument is that I've personally fixed around a dozen bugs in other people's code over the years that were the direct result of doing this, and it's also annoying when stepping through the debugger and seeing garbage values. :-)

For the most part this debate can be skirted by doing what I consider a C++ best practice:

 HRESULT hr = DoSomething(hwndBlah, TRUE, TRUE);

That is, reducing the scope of variables as much as possible and avoid splitting initialization from assignment. However, in the above contrived example this isn't really possible.

Comments

• Anonymous
  January 25, 2006
  "Catch the bug? This one's so obvious I'm not going to wait a day: if "hwndParent" is NULL then "hr" is left uninitialized. "
  
  If "hwndParent" is not NULL and both the other params are false, same thing.
• Anonymous
  January 26, 2006
  "However, in the above contrived example this isn't really possible."
  
  Why not? What's wrong with:
  
  if (hwndParent != NULL)
  {
  if (fFoo)
  {
  return DoOperationWithFoo(hwndParent);
  }
  else if (fBar)
  {
  return DoOperationWithBar(hwndParent);
  }
  }
  return whatever_the_default_return_code_should_be;
  
  The advantage of this approach is that the compiler will tell you if you leave out the last "return" statement. At least I hope it will, it certainly will in C# or Java but I haven't worked in C++ in a long long time...
• Anonymous
  January 26, 2006
  Leppie, yep. ;-)
• Anonymous
  January 27, 2006
  Alternate approach... document your assumptions:
  
  assert(0 != hwndParent);
  assert(fFoo || fBar);
  
  (Arguably the latter should be fFoo != fBar to give an XOR condition, but from the presented code cannot tell if that would be right.)