4

I'm working on some existing c++ code that appears to be written poorly, and is very frequently called. I'm wondering if I should spend time changing it, or if the compiler is already optimizing the problem away.

I'm using Visual Studio 2008.

Here is an example:

void someDrawingFunction(....)
{
  GetContext().DrawSomething(...);
  GetContext().DrawSomething(...);
  GetContext().DrawSomething(...);
  .
  .
  .
}

Here is how I would do it:

void someDrawingFunction(....)
{
  MyContext &c = GetContext();
  c.DrawSomething(...);
  c.DrawSomething(...);
  c.DrawSomething(...);
  .
  .
  .
}
Tim Rupe
  • 4,323
  • 1
  • 22
  • 23
  • Would that be Visual Studio 2008? – epotter May 01 '09 at 16:46
  • I had to make exactly this kind of optimization recently. And i *did* profile the code before doing it. – Constantin May 04 '09 at 17:00
  • Thanks for the catch epotter. Leave it up to me to get VS 9 (2008) combined into 2009. :-) – Tim Rupe May 04 '09 at 22:44
  • Sampling would tell if GetContext() takes more than epsilon time, if time is what you're worried about. Some folks might wonder if the paint finish on their Humm-V would make it get better gas mileage. I assume you're not like that. – Mike Dunlavey May 27 '09 at 17:39

6 Answers6

25

Don't guess at where your program is spending time. Profile first to find your bottlenecks, then optimize those.

As for GetContext(), that depends on how complex it is. If it's just returning a class member variable, then chances are that the compiler will inline it. If GetContext() has to perform a more complicated operation (such as looking up the context in a table), the compiler probably isn't inlining it, and you may wish to only call it once, as in your second snippet.

If you're using GCC, you can also tag the GetContext() function with the pure attribute. This will allow it to perform more optimizations, such as common subexpression elimination.

Adam Rosenfield
  • 390,455
  • 97
  • 512
  • 589
  • 1
    +1: Profile. And have a set of unittests before you make any changes. No unit tests means you could easily break something and never know. – S.Lott May 01 '09 at 15:14
  • +1. In particular, the compiler will need to see the source code for GetContext() in the same translation unit (e.g. via a header) in order to be able to either inline it or reason that it is pure and can be safely optimised out. – j_random_hacker May 01 '09 at 15:22
  • Even if GetContext() is not inlined, it seems unlikely that this "optimization" will make a measureable difference. You've gotta profile! – Mark Ransom May 01 '09 at 18:22
  • which version of GCC lets you use the pure attribute? Is this a C++0x feature or am I mistaken? – Carson Myers May 02 '09 at 19:45
  • 1
    @Carons Myers: It's a GCC extension; it's not a feature of C++98 or C++0x. It's available in GCC 2.96 and later, according to the documentation. – Adam Rosenfield May 02 '09 at 21:23
11

If you're sure it's a performance problem, change it. If GetContext is a function call (as opposed to a macro or an inline function), then the compiler is going to HAVE to call it every time, because the compiler can't necessarily see what it's doing, and thus, the compiler probably won't know that it can eliminate the call.

Of course, you'll need to make sure that GetContext ALWAYS returns the same thing, and that this 'optimization' is safe.

Michael Kohne
  • 11,888
  • 3
  • 47
  • 79
  • Absolutely. Mind you, in a "DrawSomthing" context it is probably true that all the GetContext()s are the same. – dmckee --- ex-moderator kitten May 01 '09 at 15:12
  • Inside the drawing function, the GetContext() will always return the same object reference (a member variable in the code I'm working with). As per Adam's suggestion, I'll try some profiling later and see if making some test changes affect performance. When drawing hundreds of thousands of objects to the screen, I'd be happy to make even the smallest of improvements. – Tim Rupe May 01 '09 at 15:21
  • Also, if GetContext() is something like a getter for a variable, it's probably as fast as a local variable. If it's fairly complicated, then you may be losing performance here. Testing and profiling is the only way to go. – David Thornley May 01 '09 at 16:50
  • David Thornley: Unless it's an inline function, then no, it's not going to be as fast as a local variable fetch because the compiler is going to have to push a return address, jump to another part of the code (with possible cache misses), etc, etc. GetContext can only be fast if it's an inline getter or if it's a macro that does something simple. – Michael Kohne May 01 '09 at 20:27
  • @Michael Kohne: Right, but it might be an inline getter. Many getters get defined in the class definition, which makes them inline by default, and the compiler is free to inline functions anyway if it likes. – David Thornley May 04 '09 at 17:01
8

If it is logically correct to do it the second way, i.e. calling GetContext() once on multiple times does not affect your program logic, i'd do it the second way even if you profile it and prove that there are no performance difference either way, so the next developer looking at this code will not ask the same question again.

Shing Yip
  • 1,596
  • 1
  • 12
  • 11
  • 1
    I agree with this. Optimizations aside, the multiple calls to GetContext() force the programmer to ask what GetContext() is *doing*, such that it needs to be called multiple times. – Dan Breslau May 01 '09 at 15:51
2

Obviously, if GetContext() has side effects (I/O, updating globals, etc.) than the suggested optimization will produce different results.

So unless the compiler can somehow detect that GetContext() is pure, you should optimize it yourself.

nimrodm
  • 23,081
  • 7
  • 58
  • 59
1

If you're wondering what the compiler does, look at the assembly code.

Paul Nathan
  • 39,638
  • 28
  • 112
  • 212
  • The problem is that this shows what this compiler is doing now, not what the next version will do. You can spot some slow spots this way (although assembly code is a lot more opaque in performance than it used to be), but you can't find if a construct is unsafe under other conditions (like the next service pack). – David Thornley May 04 '09 at 17:03
  • Well, yes. But if he was wondering what the compiler was doing, ASM is a good place to look at. – Paul Nathan May 06 '09 at 16:03
0

That is such a simple change, I would do it.
It is quicker to fix it than to debate it.

But do you actually have a problem?
Just because it's called often doesn't mean it's called TOO often.
If it seems qualitatively piggy, sample it to see what it's spending time at.
Chances are excellent that it is not what you would have guessed.

Community
  • 1
  • 1
Mike Dunlavey
  • 40,059
  • 14
  • 91
  • 135