views:

531

answers:

15

In the following two versions of switch case, I am wondering which version is efficient.

1:

string* convertToString(int i)
{
    switch(i)
    {
    case 1:
        return new string("one");
    case 2:
        return new string("two");
    case 3:
        return new string("three");
        .
        .
    default:
        return new string("error");
    }
}

2:

string* convertToString(int i)
{
    string *intAsString;
    switch(i)
    {
    case 1:
        intAsString = new string("one");
        break;
    case 2:
        intAsString = new string("two");
        break;
    case 3:
        intAsString = new string("three");
        break;
        .
        .
    default:
        intAsString = new string("error");
        break;
    }
return intAsString;
}

1: has multiple return statements will it cause compiler to generate extra code?

+1  A: 

The compiler most probably will optimize both versions to the same code.

sth
"Have faith my son" ;-)
jldupont
Have faith? Disassemble and you will _know_ ;)
int3
+2  A: 

If you turn optimizing on, both functions will very likely generate equivalent code.

drhirsch
+8  A: 

Both are.

What you should really be concerned about is your use of pointers here. Is it necessary? Who will delete these strings? Isn't there a simpler alternative?

Daniel Daranas
+25  A: 

This is a premature optimization worry.

The former form is clearer and has fewer source lines, that is a compelling reason to chose it (in my opinion), of course.

You should (as usual) profile your program to determine if this function is even on the "hot list" for optimization. This will tell you if there is a performance penalty for using break.

As was pointed out in the comments, it's very possible that the main performance culprit of this code is the dynamically allocated strings. Generally, when implementing this kind of "integer to string" mapping function, you should return string constants.

unwind
Wanting to understand your code is hardly premature optimization. Wanting to understand what happens in a switch statement is perfectly sensible. Of course, a meaningful answer should explain why the performance difference is nonexistent. But it shouldn't say "Pretend that performance doesn't matter, and ignore what the compiler does to your code"
jalf
+1, And if you are to "optimize it" already - change the return value from string* to const char* first and then worry about this minor switch issue ;)
RnR
@jalf - the compiler can do whatever it wants with both versions and generate exactly the same code, and if the function get's inlined while called with a constant parameter the switch statement may be completely removed. And he has a much bigger performance issue with the strings in this function - that's why it's premature optimization looking at the way of returning the pointer...
RnR
Neither voting up nor down. I really don't like those copy-and-paste premature-optimization answers. While the answer is of course correct, it isn't an answer to the original question. And instead of some blah blah about profiling, which wasn't exactly what the OP asked, some hints __why__ it is premature optimization would be more helpful.
drhirsch
+1  A: 

They will almost certainly both be compiled to an identical, highly-efficient branch table. Use whichever one you feel is clearer.

Nick Lewis
A: 

There won't be any difference in efficiency here. Certainly none that will matter. The only benefit of going with option #2 is if you'll need to do some post-processing of the string that applies to all cases.

jvenema
A: 

There should not be any measurable difference, return statements should not generate any machinery. They should put a pointer to the string object (allocated on the heap) on the stack of the callsite.

Hassan Syed
I don't think you mean reference here. :-)
Konrad
+6  A: 

There should be no difference in the compiled code. However:

  1. You'll probably find returning the strings by value to be more efficient.
  2. If there are a lot of strings consider prepopulating a vector with them (or declare a static array) and use i as the index in.
Phil Nash
@Vainstah - why is (1) inaccurate and evil?
Phil Nash
1 = inacurate and evil. 2=correct in some cases, perhaps as a global symbol table for numeric literals to strings.
Hassan Syed
pass by value = create copy. you want to pass the reference by value not the string itself. Please do some reading on mutable data structures, especially strings. A reference will be 4, or 8 bytes. a string .....
Hassan Syed
@Vainstah RVO will likely optimise the copy away and construct directly at the call site. Even if not, std::string has been crafted as a value type and typically manages its internal memory such that small strings are held in a static array and long strings on the heap. By creating the strings themselves on the heap you're throwing all that optimisation away, but still paying for its overhead.
Phil Nash
Btw, fragmentation is the enemy of garbage collectors as well :P they can remove fragmentation but the process is VERY expensive :D So hey you can avoid it by offloading some of the work it might have to do by writing sensible code.
Hassan Syed
I agree with that statement (+1_. since it is possible to detect in the compiler and thus optimize for, and since the library writers can create say a 32 char(both ASCII and unicode) static buffer. However this is bad rule of thumb to give someone. If they start using this approach everywhere they will be using in places where the strings exceed the 32-char string :/.
Hassan Syed
what does garbage collection have to do with this?
Phil Nash
The point is that there are compiler optimisations and library features at play here that it is very difficult, if not impossible, to generalise for. Rather than trying to second guess it for the sake of optimisation the best approach is to use the idiomatic approach (in this case treat strings as values, expect RVO to apply), and give the compiler and libraries the best chance to optimise for you. If, despite all that, you find you still need to tweak, given performance metrics, then go ahead and experiment with different approaches.
Phil Nash
your strategy (1) may use a copy constructor (if we discard the optimization assumption), and such a strategy creates objects needlessly. the more needless objects you create, the more de-fragmentation the GC will eventually have to perform.
Hassan Syed
This is C++ we're talking about. There is no Garbage Collector (unless you're using a 3rd party library-based GC - but that's pretty rare).
Phil Nash
@Vainstah: if you're worried about creating needless objects, then returning heap-allocated strings is wrong in the first place. The function should take a string by non-const reference (or pointer) and modify that.
Steve Jessop
@Steve, agreed - that would remove any uncertainty about whether RVO kicks in, but I'd still argue that it's more natural and idiomatic to return the strings by value, leaving the optimising for later if necessary. If RVO does apply it should boil down to much the same code.
Phil Nash
Ok I agree with your line of reasoning, I'm a low-level programmer that has written a lot of C# and Java as well. I guess I cannot fully take off my systems programming hat. I do believe that my line of reasoning is applicable to Java and C# as well.However, I strongly believe that once an applications get large enough you do need programming conventions that assist in taking load of the underlying machinery. I have seen such conventions in play in other contexts. Any eclipse/netbeans/Glassfish programmers here to chip in ? :
Hassan Syed
@steve yes I doubt you can get rid of a single string creation. But creating one and returning it by value and copy constructing another is such a simple step to side-step isn't it ? I thought this was in Java btw. But seeing as this is C++ a const string would be best.
Hassan Syed
@Phil: Yes indeed, and return-by-value with RVO avoids a default constructor, whereas with my suggested scheme the object has to be constructed before the reference is passed. I was just trying to make the point that even if we disbelieve RVO, allocating a heap object is *definitely* a bad way to "avoid creating objects needlessly". Assuming memory fragmentation really is a concern.
Steve Jessop
well without a garbage collector it is even more of a concern (in a long-running-application). But seeing that this is C++ you cannot depend on implementations of the STL. You have to make best fit decisions for portable code. I guess we are discussion non-issues here. It's been fruitfull (in the grand scheme of things) but unless we have a string related hot-spot we are arguing swings and roundabouts :D. Changing answer to +1.
Hassan Syed
@Vainstah - exactly, there are subtleties at play that suggest going with what's natural and not worrying until we have seen performance metrics. BTW, returning by value eliminates the chance of a memory leak too.@Steve - yep, sounds like we're both singing for the choir here :-)
Phil Nash
+3  A: 

You can never know how optimization will influence the code produced unless you compile with a specific compiler version, a specific set of settings and a specific code base.

C++ optimizing compilers may decide to turn your source code upside down to gain a specific optimization only available for compiler architecture so-and-so without you ever knowing it. A powerful optimizing compiler may e.g. find out that only 2 out of 10 cases are ever needed and will optimize away the whole switch-case-statement.

So my answer to your question is: Mu.

Thorsten79
yes, you should not worry about such things as a application developer. At least till you get enough feeling for it. Even if there was something to optimize here it would not make any measurable difference to your application :D unless you wrote a 3d engine which represented matrices and vectors as strings. if you ever find a real hotspot in your code then come back to us with the code.
Hassan Syed
+1  A: 

I would suggest something of the form:

void CScope::ToStr( int i, std::string& strOutput )
{
   switch( i )
   {
   case 1:
        strOutput = "Some text involving the number 1";

   ... etc etc
}

By returning a pointer to a string created on the heap, you risk memory leaks. Specifically regarding your question, I would suggest that the least number of return paths is more advisable than premature optimisation.

Konrad
+1  A: 

Consider keeping the strings as static constants:

static char const g_aaczNUMBER[][] = 
    {
        {"Zero"}, { "One" }, ...
    };

static char const g_aczERROR[] = { "Error" };

char* convertIntToString(int i) const { 
    return i<0 || 9<i ? g_aczERROR : g_aaczNUMBER[i]; 
}
Sebastian
Why trade off clarity for an obfuscated and actually rather error prone (should you ever extend it or get an argument >9 when not asserting) solution not even using C++ strings?
Fredrik
I consider my solution to be much clearer than a huge switch statement. I omitted the error case more out of lazyness.
Sebastian
Additionally, using global constants eliminates all string copying.
Sebastian
@Sebastian: I disagree about the clarity but what if you need to cover -1 to 5 or 100, 200, 300, 301, 302, 304, 401 and 501? Still clearer for you?
Fredrik
You only run into problems if the numbers are not continuous. If you have 500 continuous numbers i'd still write them in an array and be done with a single line of code.
Sebastian
A: 

Have you tried compiling them and comparing file sizes?

Justin
..or analyzing the machine code and/or assembly of code you wrote to analyze the efficiency?
Justin
+3  A: 

A switch statement is basically a series of if statements as generated machine instructions. One simple optimization strategy is to place the most frequent case first in the switch statement.

I also recommend the same solution as Sebastian but without the assert.

static const char *numberAsString[] = {
    "Zero",
    "One",
    "Two",
    "Three",
    "Four",
    "Five",
    "Six",
};

const char *ConvertToString(int num) {
  if (num < 1 || num >= (sizeof(numberAsString)/sizeof(char*))) 
    return "error";
  return numberAsString[num];
}
epatel
Do you have any thing backing this up: "A switch statement is basically a series of if statements as generated machine instruction". I'd call it downright incorrect.
Fredrik
as this is c++ this is the best answer imo.
Hassan Syed
@Vainstah: Out of curiosity, is that because it doesn't use any C++ at all (not saying it is a bad thing) or because of the incorrect statement about a switch/case resulting in a lot of if statements being generated in the order they appear?
Fredrik
Bad code. The error appears like a valid return. It should at least return NULL in case of an error (to separate the valid from the invalid cases) or throw an exception. Regarding the "series of if statements": Compilers can transform a switch-case into almost anything. They may even tumble the cases if they think it's useful for code generation. Relying on old-school source-to-binary rules is magical (and often wishful) thinking.
Thorsten79
@Fredrik Sure there can be generated jump tables too. Maybe for this example as it's a straight integer list. I'd say it depends on the compiler...but in my book a sort for the most frequent first is a *rule of thumb* if you have a heavy used switch statement
epatel
@Thorsten79 I tried to follow the questions range ("Zero" not used) and returning the string "error" as it does.
epatel
@Thorsten79 off-by-one? `(sizeof(numberAsString)/sizeof(char*))` should give the value 7 (7 entries) so when `num=6` should return "Six", `num=7` return "error"
epatel
@epatel:"most frequent first" rule is a good rule but it has nothing to do with code generation. The main reason for following it is that it is the code you are most likely to be looking for and having it first makes it easier to find it. Regarding the code generation, I don't think I have seen a compiler generating what you describe for any switch/case with more than 2 items ever, you are trying to outsmart the compiler and I can assure you it is not a good strategy here.
Fredrik
@epatel: Alright, I hadn't seen the unused zero in the original posting. So the design problem is with the original author, not you. Sorry!
Thorsten79
@Fredrik I just wanted to point out the most-frequeny-first rule, but I did not want to write an essay about code generation.
epatel
@Fredrik when I learnt about "most frequent first" it definitely was about generated code. Frequently used when running and frequently looked when developing is not the same thing. http://tinyurl.com/switch-case-stuff
epatel
BTW, series of if statements can be converted to a jumptable too by a smart compiler.
drhirsch
@drhirsch See comment 30 minutes before yours by me
epatel
@drhirsch: sur it could be done in some cases, but not easily for all cascading statement, you have to ensure the if expressions can be executed in any order without changing the result. Do you have any reference on a compiler actually doing it ? On the other hand I know of no compiler that doesn't use a jump table for switch statement (well a bit more complicated when target processor instruction set as limited jump range, or to avoid too big jump tables). But using switch is usually faster than using function pointers and about as fast as using C++ virtual functions and inheritance).
kriss
@epatel: The link you present is probably valid for some embedded environment without a real compiler but something that generates assembler from C (built by someone who's main business is the embedded environment and not compiler design). It is NOT the norm and if you deal with exceptions you will have to learn to deal with them. It doesn't make the exception the right way in all other cases (and yes, after a couple of decades I know that there is a difference between running and browsing code :-)
Fredrik
@frederick, firstly because it uses const hard-coded literals. Secondly because it only does two compares at worst.
Hassan Syed
@Fredrik The link was *one* example of someone else talking about the same *rule* and that it is about *code generation* and not *convenient to find* as you say the rule is about. Another example, http://tinyurl.com/switch-thing-01 under "Optimization Technique 2". I'm just trying to mention something I think is an interesting thing and not that this is some rule to live by. If you really need all this optimization you will need to test thoroughly anyway.
epatel
+1  A: 

You optimise[*] switch statements by doing as little work as possible in the switch (because it's uncertain whether the compiler will common up the duplication). If you insist on returning a string by pointer, and using a switch statement, I'd write this:

string *convertToString(int i) {
    const char *str;
    switch(i) {
        case 1 : str = "one"; break;
        // etc
        default : str = "error"; break;
    }
    return new string(str);
}

But of course for this example I'd probably just use a lookup table:

const char *values[] = {"error", "one", ... };

string convertToString(unsigned int i) {
    if (i >= sizeof(values)/sizeof(*values)) i = 0;
    return values[i];
}

That said, I just answered a question about the static initialization order fiasco, so you don't in general want rules of thumb which demand globals. What you do has to depend on the context of the function.

[*] Where I mean the kind of rule-of-thumb optimisation that you do when writing portable code, or in your first version, in the hope of creating code that is clear to read and won't need too much real optimisation. Real optimisation involves real measurements.

Steve Jessop
A: 

The funny part is you worry about efficieny of break then return but make a new string every time.

The answer is it's up to the compiler, but it should not matter either way. Avoiding the new string will if you call this all the time.

The switch can often be optimized so that it performs a jump instead of a bunch of if else, but if you look in the assembly source you'll generally be underwhelmed by how little the optimizer does.

Charles Eli Cheese