2

Recently I wrote something like this:

public void doSomething(boolean b1, boolean b2){
   while(true){
      if(b1){
         doThis();
      }
      if(b2){
         doThat();
      }
   }
}

But I really don't like this solution, because in every iteration you will have to check the 2 booleans. So as possible solution I could imagine to write 4 while loops with the ifs before each loop, but for obvious reasons this sucks in maintainability. Do you have any suggestions to make this piece of code nice and effective?

Robin
  • 3,512
  • 10
  • 39
  • 73
  • 2
    What exactly is the perceived issue with checking the two booleans? – NPE Dec 14 '12 at 17:54
  • Look after the molehills and the mountains will take care of themselves. – Perception Dec 14 '12 at 17:58
  • Its going to execute way more than 4x because your creating an infinite loop – Kevin Bowersox Dec 14 '12 at 17:58
  • @NPE Well this loop will iterate several million times in my code, so I was wondering if am able to improve the performance here. – Robin Dec 14 '12 at 17:58
  • 4
    @Robin: Have you profiled & established that this is an actual bottleneck? – NPE Dec 14 '12 at 17:59
  • I posted a jacked up solution the first time, this has been modified to be more in line with what the original question was asking. – Woot4Moo Dec 14 '12 at 18:01
  • @NPE No, but in my natural understanding of code execution, booth if statements will be executed way to often. Maybe this wont change anything in the performance. But I was wondering if anybody ever thought about it. – Robin Dec 14 '12 at 18:02
  • 3
    @Robin: Intuition is a very poor guide in these things. Make sure the code is correct, get the profiler out, and then optimize based on what the profiler tell you. – NPE Dec 14 '12 at 18:03
  • 1
    It isn't a bottleneck so to speak, but it does put extra strain on the system, which in small cases wont matter, but if this code is executed 12 trillion times a day, that time adds up. – Woot4Moo Dec 14 '12 at 18:03

6 Answers6

15

This looks like premature optimization.

This is a mind-trap.

Don't worry about details like this until your application is finished and correct. Then, if it's not fast enough, get a profiler out and see where the program's time is being spent.

Don't waste mental effort optimizing things that probably aren't notable.

assylias
  • 321,522
  • 82
  • 660
  • 783
Grant Birchmeier
  • 17,809
  • 11
  • 63
  • 98
  • 1
    Eh, I would contest it isn't so much premature as correct coding style. No point in doing extra steps when a "more correct" solution is available. It is valuable to remember we are writing proofs, not just code. – Woot4Moo Dec 14 '12 at 18:02
  • Thanks, but I was only wondering if anybody ever thought about it. And maybe there is a solution ready. – Robin Dec 14 '12 at 18:03
  • 2
    Wow. Can't believe someone edited my answer to change "crap" to "details". Sorry if I offended anyone's delicate sensibilities. I know us programmers are an aristocratic sort. – Grant Birchmeier Dec 14 '12 at 18:05
  • Someone edited because this is a professional site and there is no need to use the phrase "crap", when "details" is also acceptable. Now if this were a farming site, "crap" may be a better term to use, but I would opt for "manure" – Woot4Moo Dec 14 '12 at 18:06
9

From a performance perspective, because b1 and b2 are not modified within the method, it is extremely likely that branch prediction and/or JIT compilation will optimise the tests away and that the actual condition checking will hardly (if at all) penalise the performance of that method.

If instead of if(b1) you had if(getB1()) then there could be more room for improvement.

Community
  • 1
  • 1
assylias
  • 321,522
  • 82
  • 660
  • 783
  • Thank you, could you please explain me what you mean with branch prediction? – Robin Dec 14 '12 at 18:05
  • hmm where did you get b1 and b2 always have the same value? – Woot4Moo Dec 14 '12 at 18:05
  • @Robin http://stackoverflow.com/questions/11227809/why-is-processing-a-sorted-array-faster-than-an-unsorted-array – assylias Dec 14 '12 at 18:08
  • Branch prediction is a compiler thing. Really, you should Google it, as I doubt a comment could cover it completely. Anyway, that's another reason you should profile before optimizing -- the compiler may be solving the problem for you. – Grant Birchmeier Dec 14 '12 at 18:09
  • @assylias no? Can I not invoke it like this: `doSomething(false,true)` I think I am missing something. – Woot4Moo Dec 14 '12 at 18:09
  • 1
    @Woot4Moo What he means is throughout the execution of the function the boolean's values don't change. I don't think he is saying they always have an equal value to eachother :) – MrCode Dec 14 '12 at 18:12
  • @MrCode ah yes OK, that makes a bit more sense. – Woot4Moo Dec 14 '12 at 18:13
  • @GrantBirchmeier Thank you for mentioning it. I will read about this topic! Thank you. – Robin Dec 14 '12 at 18:15
4

The CPU designers have thought of this already. It's called branch prediction. What this does is effectively skip branches which are usually not called. This means the CPU will dynamically remove the branch(s) which are not used with next to no performance impact. The only impact of this is when the prediction gets it wrong as the "rollback" can be complex. In your case, the booleans don't change so you shouldn't see a problem.

Peter Lawrey
  • 525,659
  • 79
  • 751
  • 1,130
1

I agree that this is premature optimization, but here's another construct you might be able to use if your language has guarantees on short-circuit evaluation. Since java does not allow you to cast void return types to boolean, you would need to modify doThis() and doThat() to return boolean.

public void doSomething(boolean b1, boolean b2){
    while(true){
        b1 && doThis(),
        b2 && doThat();
    }
}

The (x && foo()) will only execute the function if the value of x is true, otherwise short-circuit evaluation will kick in.

You would have to be extremely careful that your compiler does not just optimize away this entire expression since no values are actually assigned.

A real, possible optimization that will avoid both comparisons in all cases is to use a switch statement in the inner loop.

public void doSomething(boolean b1, boolean b2){
    int state = (b1 ? 1 : 0) + (b2 ? 2 : 0);
    while(true){
        switch (state){
            case 1: doThis(); break;
            case 3: doThis();
            case 2: doThat();
            default:
        }
    }
}
Lucas
  • 8,035
  • 2
  • 32
  • 45
  • 4
    That assumes that `doThis` returns a boolean. – assylias Dec 14 '12 at 18:10
  • I think, this won't be practical in most cases, but this seems pretty clever to me. Really nice idea. – Robin Dec 14 '12 at 18:13
  • 1
    To be honest I don't see the difference, performance-wise, between `if(b1) doThis();` and `b1 && doThis();` - in both case the boolean will be evaluated. – assylias Dec 14 '12 at 18:18
  • @assylias: I agree. I proposed this more as a possibility for a concise expression of the OP code. – Lucas Dec 14 '12 at 18:20
  • @assylias Performance-wise, there will be no difference (unless a compiler writer made a joke). – Daniel Fischer Dec 14 '12 at 18:21
  • The compiler can only optimise the expression away if it can prove that `doThis()` and `doThat()` have no side effects. But in that case, optimising that out doesn't change anything. – Daniel Fischer Dec 14 '12 at 18:22
  • Your edit seems like a real solution to this. Thank you for the idea. – Robin Dec 14 '12 at 19:26
  • @Robin It is unlikely that any of the 2 proposed solutions are going to improve the performance (they might actually degrade it) - what the `switch` proposal successfully achieves however, is to make the code impossible to understand at first sight. This is counterproductive at best, if not dangerous. – assylias Dec 15 '12 at 09:36
  • @assylias Yes it's really not very readable I agree with this. But as far as I am concerned, the compiler will only execute the `switch-case` once, like in a `if-else` block? – Robin Dec 15 '12 at 12:12
  • @Robin It is (almost) impossible to know what machine code this will translate to. In particular, your if/else will almost certainly be optimized by the compiler and/or the processor. I'd keep your original code: it is simple and easy => easier for the compiler to optimise... – assylias Dec 15 '12 at 16:36
  • 1
    I agree with @assylias. I'm glad you accepted this answer, but please don't use it in real code. Your co-workers will thank you. – Lucas Dec 16 '12 at 01:24
  • @Lucas Yes I also think that your solution is actually not a very good idea, so I really struggled with my decision, what answer I will accept, but I chose yours because this was the only answer that offered a possible solution. Altough I liked the answers on `branch prediction` because I learned something here. – Robin Dec 16 '12 at 12:55
0
public void doSomething(boolean b1, boolean b2){
   while(true){
      if(b1 && b2)  
      {  
        doThis();  
        doThat();  
      }
      else if(b1){
         doThis();
      }
      else if(b2){
         doThat();
      }

   }
}

The compiler will optimize this and not hit each if / else if block in the loop. Similar to a switch statement. Going over the potential logic, If b1 and b2 are true you want to doThis and doThat. Else only one of each is true and the logic is similar to what it was before.

Woot4Moo
  • 23,987
  • 16
  • 94
  • 151
  • That doesn't really solve anything as `doThat` will only fire when b1 evaluates to false. Robin's solution has the potential for both to fire in the right conditions. – Grambot Dec 14 '12 at 17:56
  • I think this might not fulfil the business requirements of that code. Since the question does not suggest whether the value of those two booleans is exclulsive of each other. Maybe they are both for seperate purposes and can be true or false together – Hanky Panky Dec 14 '12 at 17:57
  • There changed it to be more in line with the original business requirements. – Woot4Moo Dec 14 '12 at 18:00
  • @Perception yes I see that now, as such I have modified mine. – Woot4Moo Dec 14 '12 at 18:01
  • @Woot4Moo Thank you, but this is writing code duplicate. In this small example these are only 2 lines. In a real life example this could be some more. And then it's not nice anymore. – Robin Dec 14 '12 at 18:08
  • @Robin I believe in the scenario where you have many, many if statements, it is time to reevaluate the function. Because it may be far too long. – Woot4Moo Dec 14 '12 at 18:10
-4

I guess you could move the if statements outside the while, like so

if (b1) {
    while (true) {
        doThis();
    }
}
if (b2) {
    while (true) {
        doThat();
    }
}

But you're really not gaining anything, since you still have to evaluate the condition that makes the while loop repeat, and you have to do THAT twice as often as the original code.

David
  • 2,602
  • 1
  • 18
  • 32