Merge lp:~vanvugt/compiz-core/lastMotionTime into lp:compiz-core

Proposed by Daniel van Vugt on 2012-04-04
Status: Rejected
Rejected by: Daniel van Vugt on 2012-04-06
Proposed branch: lp:~vanvugt/compiz-core/lastMotionTime
Merge into: lp:compiz-core
Diff against target: 75 lines (+17/-13)
3 files modified
metadata/core.xml.in (+7/-0)
src/privatescreen.h (+1/-0)
src/screen.cpp (+9/-13)
To merge this branch: bzr merge lp:~vanvugt/compiz-core/lastMotionTime
Reviewer Review Type Date Requested Status
Sam Spilsbury Disapprove on 2012-04-05
Alan Griffiths 2012-04-04 Approve on 2012-04-05
Review via email: mp+100742@code.launchpad.net

Description of the change

Faster window movement, by faster MotionNotify handling.

PrivateScreen::queueEvents has always been designed to try and deduplicate
MotionNotify events so as to reduce CPU load. However the old algorithm
fails to deduplicate events most of the time as the event loop is fast
enough that only one such event happened per iteration. Hence there was
nothing to deduplicate against.

This new algorithm deduplicates motion events across event loop iterations,
at least halving, usually quartering, the number of motion events that
need to be processed. This translates directly to lower CPU use.

To post a comment you must log in.
Alan Griffiths (alan-griffiths) wrote :

AFAICS There would be no cost to reducing the scope of peekEvent to its use.

review: Approve
Daniel van Vugt (vanvugt) wrote :

Sam had reasonable concerns about this branch. So consider it on hold for now.

Sam Spilsbury (smspillaz) wrote :

I don't really like the idea the idea of what this branch is doing for two reasons

1) It shouldn't be a user option to control the internals of core, sampling of motion events is definitely one of those
2) When we are processing events we need to have a correct idea of server side state at the time the event was emitted. For example, it is theoretically possible that we need to know the cursor position at the time a window geometry changes or at the time a window is mapped (this is relevant for the focus code). Its also relevant to things like, eg, edge detection.

Throttling processing of motion events is a good idea, however it shouldn't be done globally and really should be done on a plugin-by-plugin basis. Releasable queues are one way to do it. Throttling events like this takes away that information where it might otherwise be necessary and could result in very difficult to debug issues.

review: Disapprove
Daniel van Vugt (vanvugt) wrote :

All good points.

This branch really was just a proof of concept. And it's now redundant as Sam's shadow geometry optimizations have fixed window movement performance:
https://code.launchpad.net/~smspillaz/compiz-core/compiz-core.fix_969101/+merge/100270

Unmerged revisions

3085. By Daniel van Vugt on 2012-04-04

Faster window movement, by faster MotionNotify handling.

PrivateScreen::queueEvents has always been designed to try and deduplicate
MotionNotify events so as to reduce CPU load. However the old algorithm
fails to deduplicate events most of the time as the event loop is fast
enough that only one such event happened per iteration. Hence there was
nothing to deduplicate against.

This new algorithm deduplicates motion events across event loop iterations,
at least halving, usually quartering, the number of motion events that
need to be processed. This translates directly to lower CPU use.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'metadata/core.xml.in'
--- metadata/core.xml.in 2012-03-09 16:13:05 +0000
+++ metadata/core.xml.in 2012-04-04 08:03:19 +0000
@@ -49,6 +49,13 @@
49 they will resume their past internal state when reloaded</_long>49 they will resume their past internal state when reloaded</_long>
50 <default>false</default>50 <default>false</default>
51 </option>51 </option>
52 <option name="max_motion_rate" type="int">
53 <_short>Maximum motion frame rate</_short>
54 <_long>Maximum frame rate at which to handle motion events. Events arriving faster than this will be skipped. Zero = infinite (old behaviour).</_long>
55 <default>60</default>
56 <min>0</min>
57 <max>1000</max>
58 </option>
52 <group>59 <group>
53 <_short>Display Settings</_short>60 <_short>Display Settings</_short>
54 <option name="overlapping_outputs" type="int">61 <option name="overlapping_outputs" type="int">
5562
=== modified file 'src/privatescreen.h'
--- src/privatescreen.h 2012-03-29 16:54:46 +0000
+++ src/privatescreen.h 2012-04-04 08:03:19 +0000
@@ -995,6 +995,7 @@
995 CompTimer edgeDelayTimer;995 CompTimer edgeDelayTimer;
996 CompDelayedEdgeSettings edgeDelaySettings;996 CompDelayedEdgeSettings edgeDelaySettings;
997 Window xdndWindow;997 Window xdndWindow;
998 Time lastMotionTime;
998};999};
9991000
1000class CompManager1001class CompManager
10011002
=== modified file 'src/screen.cpp'
--- src/screen.cpp 2012-03-30 06:26:33 +0000
+++ src/screen.cpp 2012-04-04 08:03:19 +0000
@@ -747,24 +747,19 @@
747{747{
748 std::list <XEvent> events;748 std::list <XEvent> events;
749749
750 int maxMotionRate = optionGetMaxMotionRate ();
751 int minMotionTime = (maxMotionRate <= 0) ? 0 : 1000 / maxMotionRate;
752
750 while (XPending (dpy))753 while (XPending (dpy))
751 {754 {
752 XEvent event, peekEvent;755 XEvent event;
753 XNextEvent (dpy, &event);756 XNextEvent (dpy, &event);
754757
755 /* Skip to the last MotionNotify
756 * event in this sequence */
757 if (event.type == MotionNotify)758 if (event.type == MotionNotify)
758 {759 {
759 while (XPending (dpy))760 if ((event.xmotion.time - lastMotionTime) < minMotionTime)
760 {761 continue;
761 XPeekEvent (dpy, &peekEvent);762 lastMotionTime = event.xmotion.time;
762
763 if (peekEvent.type != MotionNotify)
764 break;
765
766 XNextEvent (dpy, &event);
767 }
768 }763 }
769764
770 events.push_back (event);765 events.push_back (event);
@@ -5127,7 +5122,8 @@
5127 initialized (false),5122 initialized (false),
5128 edgeWindow (None),5123 edgeWindow (None),
5129 edgeDelayTimer (),5124 edgeDelayTimer (),
5130 xdndWindow (None)5125 xdndWindow (None),
5126 lastMotionTime (0)
5131{5127{
5132 pingTimer.setCallback (5128 pingTimer.setCallback (
5133 boost::bind (&PrivateScreen::handlePingTimeout, this));5129 boost::bind (&PrivateScreen::handlePingTimeout, this));

Subscribers

People subscribed via source and target branches