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
1=== modified file 'metadata/core.xml.in'
2--- metadata/core.xml.in 2012-03-09 16:13:05 +0000
3+++ metadata/core.xml.in 2012-04-04 08:03:19 +0000
4@@ -49,6 +49,13 @@
5 they will resume their past internal state when reloaded</_long>
6 <default>false</default>
7 </option>
8+ <option name="max_motion_rate" type="int">
9+ <_short>Maximum motion frame rate</_short>
10+ <_long>Maximum frame rate at which to handle motion events. Events arriving faster than this will be skipped. Zero = infinite (old behaviour).</_long>
11+ <default>60</default>
12+ <min>0</min>
13+ <max>1000</max>
14+ </option>
15 <group>
16 <_short>Display Settings</_short>
17 <option name="overlapping_outputs" type="int">
18
19=== modified file 'src/privatescreen.h'
20--- src/privatescreen.h 2012-03-29 16:54:46 +0000
21+++ src/privatescreen.h 2012-04-04 08:03:19 +0000
22@@ -995,6 +995,7 @@
23 CompTimer edgeDelayTimer;
24 CompDelayedEdgeSettings edgeDelaySettings;
25 Window xdndWindow;
26+ Time lastMotionTime;
27 };
28
29 class CompManager
30
31=== modified file 'src/screen.cpp'
32--- src/screen.cpp 2012-03-30 06:26:33 +0000
33+++ src/screen.cpp 2012-04-04 08:03:19 +0000
34@@ -747,24 +747,19 @@
35 {
36 std::list <XEvent> events;
37
38+ int maxMotionRate = optionGetMaxMotionRate ();
39+ int minMotionTime = (maxMotionRate <= 0) ? 0 : 1000 / maxMotionRate;
40+
41 while (XPending (dpy))
42 {
43- XEvent event, peekEvent;
44+ XEvent event;
45 XNextEvent (dpy, &event);
46
47- /* Skip to the last MotionNotify
48- * event in this sequence */
49 if (event.type == MotionNotify)
50 {
51- while (XPending (dpy))
52- {
53- XPeekEvent (dpy, &peekEvent);
54-
55- if (peekEvent.type != MotionNotify)
56- break;
57-
58- XNextEvent (dpy, &event);
59- }
60+ if ((event.xmotion.time - lastMotionTime) < minMotionTime)
61+ continue;
62+ lastMotionTime = event.xmotion.time;
63 }
64
65 events.push_back (event);
66@@ -5127,7 +5122,8 @@
67 initialized (false),
68 edgeWindow (None),
69 edgeDelayTimer (),
70- xdndWindow (None)
71+ xdndWindow (None),
72+ lastMotionTime (0)
73 {
74 pingTimer.setCallback (
75 boost::bind (&PrivateScreen::handlePingTimeout, this));

Subscribers

People subscribed via source and target branches