Compiz plugin's ABI check logic could be improved and made faster
Affects | Status | Importance | Assigned to | Milestone | |
---|---|---|---|---|---|
Compiz |
Fix Released
|
Low
|
MC Return | ||
compiz (Ubuntu) |
Fix Released
|
Undecided
|
Unassigned |
Bug Description
Case A (used in current trunk):
bool
GridPluginVTabl
{
if (!CompPlugin:
!CompPlugin:
!CompPlugin:
return false;
return true;
}
Case B (suggested improvement):
bool
GridPluginVTabl
{
if (CompPlugin:
CompPlugin:
CompPlugin:
return true;
return false;
}
>
> Test program:
> Case A:
>
> 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>
>
>Case B:
> 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.
>
> Also 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.
Another issue is how detailed we want our CompLog error message to be if the ABI check failed.
If we want the message to report which part of the ABI check failed, we should simply repeat it again, but this is another bug for another day...
Related branches
- PS Jenkins bot (community): Approve (continuous-integration)
- Sam Spilsbury: Approve
- MC Return: Needs Resubmitting
-
Diff: 1998 lines (+499/-547)87 files modifiedplugins/addhelper/src/addhelper.cpp (+5/-5)
plugins/animation/src/animation.cpp (+11/-11)
plugins/animationaddon/src/animationaddon.cpp (+12/-11)
plugins/annotate/src/annotate.cpp (+5/-5)
plugins/bench/src/bench.cpp (+5/-6)
plugins/bicubic/src/bicubic.cpp (+5/-7)
plugins/blur/src/blur.cpp (+5/-6)
plugins/ccp/src/ccp.cpp (+3/-3)
plugins/clone/src/clone.cpp (+5/-5)
plugins/colorfilter/src/colorfilter.cpp (+6/-10)
plugins/commands/src/commands.cpp (+3/-4)
plugins/compiztoolbox/src/compiztoolbox.cpp (+12/-11)
plugins/composite/src/composite.cpp (+9/-8)
plugins/copytex/src/copytex.cpp (+4/-4)
plugins/crashhandler/src/crashhandler.cpp (+3/-3)
plugins/cube/src/cube.cpp (+11/-11)
plugins/cubeaddon/src/cubeaddon.cpp (+6/-6)
plugins/dbus/src/dbus.cpp (+4/-3)
plugins/decor/src/decor.cpp (+3/-4)
plugins/expo/src/expo.cpp (+5/-7)
plugins/extrawm/src/extrawm.cpp (+8/-8)
plugins/ezoom/src/ezoom.cpp (+6/-6)
plugins/fade/src/fade.cpp (+6/-7)
plugins/fadedesktop/src/fadedesktop.cpp (+5/-5)
plugins/firepaint/src/firepaint.cpp (+5/-5)
plugins/freewins/src/freewins.cpp (+6/-8)
plugins/gears/src/gears.cpp (+5/-7)
plugins/gnomecompat/src/gnomecompat.cpp (+3/-3)
plugins/grid/src/grid.cpp (+3/-3)
plugins/group/src/init.cpp (+11/-11)
plugins/imgjpeg/src/imgjpeg.cpp (+3/-4)
plugins/imgpng/src/imgpng.cpp (+3/-4)
plugins/imgsvg/src/imgsvg.cpp (+7/-7)
plugins/inotify/src/inotify.cpp (+3/-4)
plugins/kde/src/kde.cpp (+3/-3)
plugins/kdecompat/src/kdecompat.cpp (+5/-5)
plugins/loginout/src/loginout.cpp (+5/-5)
plugins/mag/src/mag.cpp (+6/-9)
plugins/maximumize/src/maximumize.cpp (+3/-3)
plugins/mblur/src/mblur.cpp (+5/-5)
plugins/mousepoll/src/mousepoll.cpp (+9/-8)
plugins/move/src/move.cpp (+3/-3)
plugins/neg/src/neg.cpp (+5/-5)
plugins/notification/src/notification.cpp (+3/-3)
plugins/obs/src/obs.cpp (+5/-5)
plugins/opacify/src/opacify.cpp (+6/-9)
plugins/opengl/src/opengl.cpp (+10/-9)
plugins/place/src/place.cpp (+3/-6)
plugins/put/src/put.cpp (+5/-7)
plugins/reflex/src/reflex.cpp (+5/-7)
plugins/regex/src/regex.cpp (+3/-3)
plugins/resize/src/resize.cpp (+3/-6)
plugins/resizeinfo/src/resizeinfo.cpp (+5/-5)
plugins/ring/src/ring.cpp (+10/-10)
plugins/rotate/src/rotate.cpp (+6/-7)
plugins/scale/src/scale.cpp (+12/-10)
plugins/scaleaddon/src/scaleaddon.cpp (+11/-11)
plugins/scalefilter/src/scalefilter.cpp (+4/-6)
plugins/screenshot/src/screenshot.cpp (+6/-6)
plugins/session/src/session.cpp (+3/-3)
plugins/shelf/src/shelf.cpp (+6/-8)
plugins/shift/src/shift.cpp (+10/-10)
plugins/showdesktop/src/showdesktop.cpp (+5/-7)
plugins/showmouse/src/showmouse.cpp (+6/-6)
plugins/showrepaint/src/showrepaint.cpp (+5/-6)
plugins/snap/src/snap.cpp (+3/-4)
plugins/splash/src/splash.cpp (+5/-5)
plugins/stackswitch/src/stackswitch.cpp (+11/-11)
plugins/staticswitcher/src/staticswitcher.cpp (+6/-7)
plugins/switcher/src/switcher.cpp (+6/-7)
plugins/td/src/3d.cpp (+6/-6)
plugins/text/src/text.cpp (+10/-8)
plugins/thumbnail/src/thumbnail.cpp (+7/-10)
plugins/titleinfo/src/titleinfo.cpp (+3/-3)
plugins/trailfocus/src/trailfocus.cpp (+5/-7)
plugins/trip/src/trip.cpp (+5/-5)
plugins/vpswitch/src/vpswitch.cpp (+3/-3)
plugins/wall/src/wall.cpp (+6/-10)
plugins/wallpaper/src/wallpaper.cpp (+5/-7)
plugins/water/src/water.cpp (+11/-8)
plugins/widget/src/widget.cpp (+5/-7)
plugins/winrules/src/winrules.cpp (+3/-4)
plugins/wizard/src/wizard.cpp (+6/-6)
plugins/wobbly/src/wobbly.cpp (+5/-6)
plugins/workarounds/src/workarounds.cpp (+6/-6)
plugins/workspacenames/src/workspacenames.cpp (+7/-6)
tests/xorg-gtest/plugins/testhelper/src/testhelper.cpp (+3/-3)
Changed in compiz: | |
assignee: | nobody → MC Return (mc-return) |
Changed in compiz: | |
status: | New → In Progress |
Changed in compiz: | |
importance: | Undecided → Low |
milestone: | none → 0.9.10.0 |
Changed in compiz: | |
status: | Fix Committed → Fix Released |
Fix committed into lp:compiz at revision None, scheduled for release in compiz, milestone 0.9.10.0