Code review comment for lp:~mc-return/compiz/compiz.merge-grid-small-cleanup

Revision history for this message
MC Return (mc-return) wrote :

>
> Test program:
>
> int main ()
> {
> bool a = true;
> bool b = true;
> bool c = true;
>
> if (!a || !b || !c)
> return 1;
>
> return 0;
> }
>
> g++ -O0 assembly:
>
> 4004fc: 0f b6 45 fd movzbl -0x3(%rbp),%eax
> 400500: 83 f0 01 xor $0x1,%eax
> 400503: 84 c0 test %al,%al
> 400505: 75 16 jne 40051d <main+0x31>
> 400507: 0f b6 45 fe movzbl -0x2(%rbp),%eax
> 40050b: 83 f0 01 xor $0x1,%eax
> 40050e: 84 c0 test %al,%al
> 400510: 75 0b jne 40051d <main+0x31>
> 400512: 0f b6 45 ff movzbl -0x1(%rbp),%eax
> 400516: 83 f0 01 xor $0x1,%eax
> 400519: 84 c0 test %al,%al
> 40051b: 74 07 je 400524 <main+0x38>
>
> int main ()
> {
> bool a = false;
> bool b = false;
> bool c = false;
>
> if (a && b && c)
> return 1;
>
> return 0;
> }
>
> Assembly:
>
> 4004fc: 80 7d fd 00 cmpb $0x0,-0x3(%rbp)
> 400500: 74 13 je 400515 <main+0x29>
> 400502: 80 7d fe 00 cmpb $0x0,-0x2(%rbp)
> 400506: 74 0d je 400515 <main+0x29>
> 400508: 80 7d ff 00 cmpb $0x0,-0x1(%rbp)
> 40050c: 74 07 je 400515 <main+0x29>
>
> The latter version is technically faster, because you can directly compare the
> booleans without negating them.
>
Do not forget that we expect almost always true, in fact it will be true in 100% of cases if
the plugin is okay, which it normally should be.

> However, this is a micro-optimization really. It saves 6 instructions and for
> 50 plugins would save 300 instructions total, which would make for a
> theoretical 4 one-hundred-millionths of a second optimization on a Pentium 4
> system (at 6500 MIPS).
>
If I do one such optimization per week, in a year it is 15000 instructions less to
execute, times say 5 million computers running Compiz will save a lot of energy ;)

> It probably makes more sense to keep it consistent with the other code. Though
> that's not to say that I wouldn't accept a best-case 4 one-hundred-millionth
> of a second optimization.

Yes, I agree 100%. It should be consistent in all cases. We also should report
to the console in the case something goes wrong, which just a few of the plugins
currently do.
Also water will currently not correctly execute without FBOs being enabled, but
there is not even a log message about it...

> I just don't think its worth the one hour of effort
> spent to write it.
It took about 30 minutes to fix it for all of the plugins here locally.
The improved error reporting will take another while of course and also to check that all ABI dependencies,
that each plugin has, get checked...

« Back to merge proposal