Merge lp:~mc-return/compiz/compiz.merge-performance-do-not-assign-values-that-are-never-used into lp:compiz/0.9.9
| Status: | Rejected |
|---|---|
| Rejected by: | Daniel van Vugt on 2012-11-27 |
| Proposed branch: | lp:~mc-return/compiz/compiz.merge-performance-do-not-assign-values-that-are-never-used |
| Merge into: | lp:compiz/0.9.9 |
| Diff against target: |
406 lines (+35/-38) 24 files modified
compizconfig/gconf/src/gconf.c (+1/-1) compizconfig/gsettings/gsettings_backend_shared/ccs_gsettings_backend.c (+1/-1) compizconfig/gsettings/gsettings_backend_shared/gsettings_util.c (+7/-7) compizconfig/integration/gnome/src/ccs_gnome_integration.c (+1/-1) compizconfig/libcompizconfig/backend/src/ini.c (+2/-2) compizconfig/libcompizconfig/src/compiz.cpp (+2/-2) compizconfig/libcompizconfig/src/config.c (+1/-1) compizconfig/libcompizconfig/src/main.c (+4/-4) gtk/window-decorator/events.c (+1/-1) gtk/window-decorator/gwd-settings-notified.c (+1/-1) gtk/window-decorator/gwd-settings-storage-gconf.c (+1/-1) gtk/window-decorator/gwd-settings-storage-gsettings.c (+1/-1) gtk/window-decorator/gwd-settings-xproperty-storage.c (+1/-1) kde/window-decorator-kde4/switcher.cpp (+1/-1) kde/window-decorator-kde4/window.cpp (+1/-2) plugins/bicubic/src/bicubic.cpp (+1/-1) plugins/cube/src/cube.cpp (+1/-1) plugins/dbus/src/dbus.cpp (+1/-1) plugins/decor/src/decor.cpp (+2/-2) plugins/grid/src/grid.cpp (+1/-1) plugins/opengl/src/texture.cpp (+1/-1) plugins/resize/src/logic/src/resize-logic.cpp (+0/-2) plugins/workarounds/src/workarounds.cpp (+1/-1) src/screen.cpp (+1/-1) |
| To merge this branch: | bzr merge lp:~mc-return/compiz/compiz.merge-performance-do-not-assign-values-that-are-never-used |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Daniel van Vugt | Disapprove on 2012-11-21 | ||
| Sam Spilsbury | 2012-11-20 | Needs Information on 2012-11-20 | |
|
Review via email:
|
|||
Commit Message
Minor Performance Optimization:
Do not assign values to variables, if those variables are assigned a new value, before the old one was ever used.
| MC Return (mc-return) wrote : | # |
> IMO its good defensive coding practise to initialize every variable before use
> because it insures against further changes using uninitialized variables.
>
> I'd like to hear the converse argument though. I think the compiler will just
> optimize out any redundancy really.
I do not trust every compiler to be that intelligent. If you assign a value to a local variable and then several instructions later reassign another value to it, without having used the first, the compiler will probably do what he is told to and make this first redundant assignment...
Also it is really ugly to find something in the code like this:
mState = saveState;
mState = saveState;
...even if the compiler would remove that one probably...
| Sam Spilsbury (smspillaz) wrote : | # |
On Tue, Nov 20, 2012 at 7:04 AM, MC Return <email address hidden> wrote:
>> IMO its good defensive coding practise to initialize every variable before use
>> because it insures against further changes using uninitialized variables.
>>
>> I'd like to hear the converse argument though. I think the compiler will just
>> optimize out any redundancy really.
>
> I do not trust every compiler to be that intelligent.
We are only using gcc . Clang is used to do some additional error
checking but that's about it.
> If you assign a value to a local variable and then several instructions later reassign another value to it, without having used the first, the compiler will probably do what he is told to and make this first redundant assignment...
The compiler can optimize out all zero-initializa
plain-old-data types allocated on the stack frame as long as they
aren't marked volatile:
http://
That's not the case for C++ classes though, which can change program
state when their constructors run, so it is good style to only declare
those variables when they are actually needed.
I guess the point I'm trying to make here is that zero initialization
makes sense when the code changes and is later recompiled, for
example:
Foo *foo = NULL;
/* blah blah blah */
foo = new Foo ();
changed to:
Foo *foo = NULL;
if (foo)
{
/* do something with foo */
}
It becomes immediately obvious to the person who changed it that foo
needs to be assigned to a value first, rather than:
Foo *foo;
if (foo)
{
}
Where foo could be anything and that if condition will pass (and the
program then becomes undefined)
>
> Also it is really ugly to find something in the code like this:
>
> mState = saveState;
> mState = saveState;
In which case the second assignment is probably the one you want to remove.
>
> ...even if the compiler would remove that one probably...
>
>
> --
> https:/
> You are reviewing the proposed merge of lp:~mc-return/compiz/compiz.merge-performance-do-not-assign-values-that-are-never-used into lp:compiz.
--
Sam Spilsbury
| Daniel van Vugt (vanvugt) wrote : | # |
I agree the nicest way to achieve this is usually to do it on a single line:
Foo *x = something
However that's only valid in C++ and not in C.
So in C, I think the existing redundant initialization is the preferred method of defence.
Also note, this is not a significant performance issue. Assigning a word-sized variable to zero or NULL is such a simple instruction that any modern CPU from the last 20 years can do several of them per clock cycle. You'd have to be doing millions or billions of redundant assignments for it to be a problem (for example in a very tight performance-
| MC Return (mc-return) wrote : | # |
> I agree the nicest way to achieve this is usually to do it on a single line:
> Foo *x = something
> However that's only valid in C++ and not in C.
>
> So in C, I think the existing redundant initialization is the preferred method
> of defence.
>
I can remove the changes to the NULL pointer initialization in the .c files of course,
but I do want to note here that all of those pointers get to point to something else
a few lines later and of course are not used without pointing to something and being
initialized first.
> Also note, this is not a significant performance issue. Assigning a word-sized
> variable to zero or NULL is such a simple instruction that any modern CPU from
> the last 20 years can do several of them per clock cycle. You'd have to be
> doing millions or billions of redundant assignments for it to be a problem
> (for example in a very tight performance-
I am sorry, but I cannot help myself - If I see something that *might* produce any
unneeded instructions I want to remove it, because it hurts my eyes ;)
| Sam Spilsbury (smspillaz) wrote : | # |
On Thu, Nov 22, 2012 at 7:49 AM, MC Return <email address hidden> wrote:
>> I agree the nicest way to achieve this is usually to do it on a single line:
>> Foo *x = something
>> However that's only valid in C++ and not in C.
>>
>> So in C, I think the existing redundant initialization is the preferred method
>> of defence.
>>
>
> I can remove the changes to the NULL pointer initialization in the .c files of course,
> but I do want to note here that all of those pointers get to point to something else
> a few lines later and of course are not used without pointing to something and being
> initialized first.
If one compiled without optimization this would be an issue, however
just for show, for a simple C program
that looks like this:
#include <stdio.h>
int main()
{
int a = 5;
int *i;
a = 1;
a = a + 1;
a = a + 2;
i = &a;
printf ("%d\n", *i);
return 0;
}
Will output assembly:
.file "main.c"
.section .rodata.
.LC0:
.string "%d\n"
.section .text.startup,
.p2align 4,,15
.globl main
.type main, @function
main:
.LFB24:
subq $8, %rsp
movl $4, %edx
movl $.LC0, %esi
movl $1, %edi
xorl %eax, %eax
call __printf_chk
xorl %eax, %eax
addq $8, %rsp
ret
.LFE24:
.size main, .-main
.ident "GCC: (Ubuntu/Linaro 4.7.2-11ubuntu1) 4.7.2"
.section .note.GNU-
And a similar program with i initialized to null:
.file "main.c"
.section .rodata.
.LC0:
.string "%d\n"
.section .text.startup,
.p2align 4,,15
.globl main
.type main, @function
main:
.LFB24:
subq $8, %rsp
movl $4, %edx
movl $.LC0, %esi
movl $1, %edi
xorl %eax, %eax
call __printf_chk
xorl %eax, %eax
addq $8, %rsp
ret
.LFE24:
.size main, .-main
.ident "GCC: (Ubuntu/Linaro 4.7.2-11ubuntu1) 4.7.2"
.section .note.GNU-
So the null initialization is effectively removed by the compiler if
it isn't used.
>
>> Also note, this is not a significant performance issue. Assigning a word-sized
>> variable to zero or NULL is such a simple instruction that any modern CPU from
>> the last 20 years can do several of them per clock cycle. You'd have to be
>> doing millions or billions of redundant assignments for it to be a problem
>> (for example in a very tight performance-
>
> I am sorry, but I cannot help myself - If I see something that *might* produce any
> unneeded instructions I want to remove it, because it hurts my eyes ;)
Well, there's a difference between un-needed instructions generally in
the most performant ve...
Unmerged revisions
- 3511. By MC Return on 2012-11-20
-
Do not assign 0 to int i, because this value is never used
- 3510. By MC Return on 2012-11-20
-
Do not assign 0 to int i, because this value is never used
- 3509. By MC Return on 2012-11-20
-
Do not assign output to the ints lco, tco, bco and rco, because this value is never used afterwards
- 3508. By MC Return on 2012-11-20
-
Do not assign NULL to EGLImageKHR eglImage, because this value is not used afterwards
- 3507. By MC Return on 2012-11-20
-
Do not assign 0 to CompWindow *cw, because this value is not used afterwards
- 3506. By MC Return on 2012-11-20
-
Do not assign 0 to XRectangle *shapeRects, because this value is not used afterwards
- 3505. By MC Return on 2012-11-20
-
Do not assign 0 to Xrectangle *shapeRects, because this value is not used afterwards
- 3504. By MC Return on 2012-11-20
-
Do not assign NULL to CompOption *option, because this value is not used afterwards
- 3503. By MC Return on 2012-11-20
-
Do not assign 0 to int output, because this value never gets used
- 3502. By MC Return on 2012-11-20
-
Do not assign 0 to int unit, because this value is never used
IMO its good defensive coding practise to initialize every variable before use because it insures against further changes using uninitialized variables.
I'd like to hear the converse argument though. I think the compiler will just optimize out any redundancy really.