Merge lp:~vanvugt/compiz-core/fix-startup-plugins into lp:compiz-core

Proposed by Daniel van Vugt on 2012-03-29
Status: Rejected
Rejected by: Daniel van Vugt on 2012-03-30
Proposed branch: lp:~vanvugt/compiz-core/fix-startup-plugins
Merge into: lp:compiz-core
Diff against target: 181 lines (+27/-71)
2 files modified
src/privatescreen.h (+5/-0)
src/screen.cpp (+22/-71)
To merge this branch: bzr merge lp:~vanvugt/compiz-core/fix-startup-plugins
Reviewer Review Type Date Requested Status
Daniel van Vugt Disapprove on 2012-03-30
Alan Griffiths Needs Fixing on 2012-03-29
Sam Spilsbury 2012-03-29 Approve on 2012-03-29
Didier Roche 2012-03-29 Pending
Review via email: mp+99886@code.launchpad.net

Description of the change

Avoid graphics corruption and hangs on compiz start-up by ensuring that
plugins don't get initialized, de-initialized and re-initialzed during
start-up. (LP: #963093) (LP: #963633)

The multiple init/fini/init calls occured when compiz was asked to load an
invalid plugin name. This occurred most recently as plugins "bailer" and
"detection" were left in some peoples' configs while the plugins themselves
no longer exist in the current compiz release.

The source of the graphics corruption and hangs has been found to be a
problem in the composite and/or opengl plugins. One or both of them are unsafe
to init/fini multiple times without a full compiz restart. So the root cause
is not exactly known yet. However composite and opengl are not alone; many
plugins have bugs with init/fini/init sequences, so it is valuable to fix
the start-up plugin ordering as this does.

Essentially this fix works by remembering which plugins don't exist at all
and putting them on a black list. Then subsquent updates check the blacklist
and know they should never include those failed plugins in testing whether
the active plugin lists have changed.

The second part of the fix is to ensure updatePlugins no longer calls
screen->setOptionForPlugin to change the active_plugins setting based on what
can and is loaded. That's not only redundant and wasteful but might cause
further callbacks to updatePlugins unnecessarily.

The final part of the fix is to remove a rendundant call to updatePlugins from
EventManager::init. It is not required as main tells PluginManager when to
load plugins on startup.

IMPORTANT NOTE FOR UBUNTU PACKAGING
In downstream ubuntu branches the DEFAULT_PLUGINS list in "debian/rules" also causes multiple plugin loads on start-up and prevents this fix from working! The solution is to change DEFAULT_PLUGINS to just "ccp".

To post a comment you must log in.
Tim Penhey (thumper) wrote :

std::set has a count method.

Ideally I was looking for something like "contains", but a non-zero
count means that it exists in the set.

Perhaps
  blacklist.count(name)
is nicer than:
  blacklist.find(name) != blacklist.end()

??

Sam Spilsbury (smspillaz) wrote :

Looks good to me.

I would also say there's another case this branch doesn't handle, which is the case where a plugin fails to load because it either a) can't be loaded or b) can't be loaded because of a missing plugin dep. In that case, I think we should put such plugins at the back of the list at the end of every new push. Dependencies will in that case be automatically resolved if the user tries to load the dependend plugin A as A will load and dependent plugin B will load thereafter. It also avoids the mass push/pop if a plugin was failing to init and in the middle of the list.

review: Approve
Daniel van Vugt (vanvugt) wrote :

Tim: count() would have to visit all elements in the set. Whereas find() only needs to visit 50% on average.

Daniel van Vugt (vanvugt) wrote :

Ignore my previous comment. The performance is the same. A set can only contain 0 or 1 of each unique value.

Alan Griffiths (alan-griffiths) wrote :

An improvement. But...

Important:

You also need to update src/privatescreen/tests/test-privatescreen.cpp

Less important:

It doesn't handle cases where a blacklisted plugin would load on a subsequent attempt - e.g. because it got installed while compiz was running.

I also have a preference for giving better names to a many of the variables here.

PS I do agree about using find() not count() - but both are O(log(n)) in an RB-tree, not liner.

review: Needs Fixing
Didier Roche (didrocks) wrote :

Nice work Daniel!
Once the branch will land to trunk, I'll make the necessary change in the ubuntu package, we can push it to a ppa and ask people to try from it.

Note to myself: override the gconf "default" profile with the plugin set once this change is done.

Unmerged revisions

3072. By Daniel van Vugt on 2012-03-29

Avoid graphics corruption and hangs on compiz start-up by ensuring that
plugins don't get initialized, de-initialized and re-initialzed during
start-up. (LP: #963093) (LP: #963633)

The multiple init/fini/init calls occured when compiz was asked to load an
invalid plugin name. This occurred most recently as plugins "bailer" and
"detection" were left in some peoples' configs while the plugins themselves
no longer exist in the current compiz release.

The source of the graphics corruption and hangs has been found to be a
problem in the composite and/or opengl plugins. One or both of them are unsafe
to init/fini multiple times without a full compiz restart. So the root cause
is not exactly known yet. However composite and opengl are not alone; many
plugins have bugs with init/fini/init sequences, so it is valuable to fix
the start-up plugin ordering as this does.

Essentially this fix works by remembering which plugins don't exist at all
and putting them on a black list. Then subsquent updates check the blacklist
and know they should never include those failed plugins in testing whether
the active plugin lists have changed.

The second part of the fix is to ensure updatePlugins no longer calls
screen->setOptionForPlugin to change the active_plugins setting based on what
can and is loaded. That's not only redundant and wasteful but might cause
further callbacks to updatePlugins unnecessarily.

The final part of the fix is to remove a rendundant call to updatePlugins from
EventManager::init. It is not required as main tells PluginManager when to
load plugins on startup.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/privatescreen.h'
2--- src/privatescreen.h 2012-03-24 09:49:30 +0000
3+++ src/privatescreen.h 2012-03-29 07:37:48 +0000
4@@ -38,6 +38,8 @@
5
6 #include <glibmm/main.h>
7
8+#include <set>
9+
10 #include "privatetimeoutsource.h"
11 #include "privateiosource.h"
12 #include "privateeventsource.h"
13@@ -574,6 +576,9 @@
14 private:
15 CompOption::Value plugin;
16 bool dirtyPluginList;
17+
18+ typedef std::set<CompString> CompStringSet;
19+ CompStringSet blackList;
20 };
21
22 class GrabList
23
24=== modified file 'src/screen.cpp'
25--- src/screen.cpp 2012-03-24 09:49:30 +0000
26+++ src/screen.cpp 2012-03-29 07:37:48 +0000
27@@ -896,68 +896,18 @@
28 void
29 cps::PluginManager::updatePlugins ()
30 {
31- unsigned int pListCount = 1;
32-
33 possibleTap = NULL;
34 dirtyPluginList = false;
35
36 CompOption::Value::Vector &list = optionGetActivePlugins ();
37-
38- /* Determine the number of plugins, which is core +
39- * initial plugins + plugins in option list in addition
40- * to initial plugins */
41- foreach (CompString &pn, initialPlugins)
42- {
43- if (pn != "core")
44- pListCount++;
45- }
46-
47- CompString lpName;
48-
49- foreach (CompOption::Value &lp, list)
50- {
51- bool skip = false;
52-
53- lpName = lp.s();
54-
55- if (lpName == "core")
56- continue;
57-
58- foreach (CompString &p, initialPlugins)
59- {
60- if (p == lpName)
61- {
62- skip = true;
63- break;
64- }
65- }
66-
67- /* plugin not in initial list */
68- if (!skip)
69- pListCount++;
70- }
71-
72- /* pListCount is now the number of plugisn contained in both the
73- * initial and new plugins list */
74- CompOption::Value::Vector pList(pListCount);
75-
76- if (pList.empty ())
77- {
78- screen->setOptionForPlugin ("core", "active_plugins", plugin);
79- return;
80- }
81+ CompOption::Value::Vector pList;
82
83 /* Must have core as first plugin */
84- pList.at (0) = "core";
85- unsigned int j = 1;
86-
87- /* Add initial plugins */
88+ pList.push_back ("core");
89 foreach (CompString &p, initialPlugins)
90 {
91- if (p == "core")
92- continue;
93- pList.at (j).set (p);
94- j++;
95+ if (p != "core" && blackList.find (p) == blackList.end ())
96+ pList.push_back (p);
97 }
98
99 /* Add plugins not in the initial list */
100@@ -965,13 +915,13 @@
101 {
102 std::list <CompString>::iterator it = initialPlugins.begin ();
103 bool skip = false;
104-
105- if (opt.s () == "core")
106+ const CompString &name = opt.s ();
107+ if (name == "core" || blackList.find (name) != blackList.end ())
108 continue;
109
110 for (; it != initialPlugins.end (); it++)
111 {
112- if ((*it) == opt.s())
113+ if (*it == name)
114 {
115 skip = true;
116 break;
117@@ -979,15 +929,10 @@
118 }
119
120 if (!skip)
121- {
122- pList.at (j++).set (opt.s ());
123- }
124+ pList.push_back (name);
125 }
126
127- assert (j == pList.size ());
128-
129- /* j is initialized to 1 to make sure we never pop the core plugin */
130- unsigned int i;
131+ unsigned int i, j;
132 for (i = j = 1; j < plugin.list ().size () && i < pList.size (); i++, j++)
133 {
134 if (plugin.list ().at (j).s () != pList.at (i).s ())
135@@ -1033,7 +978,8 @@
136
137 if (p == 0 && !failedPush)
138 {
139- p = CompPlugin::load (pList[i].s ().c_str ());
140+ const CompString &name = pList[i].s ();
141+ p = CompPlugin::load (name.c_str ());
142
143 if (p)
144 {
145@@ -1043,6 +989,17 @@
146 p = 0;
147 }
148 }
149+ else
150+ {
151+ /*
152+ * Plugins that can't load go on the black list. If we keep
153+ * trying to load something that can't be loaded then it
154+ * forces lots of unnecessary popping and pushing every update.
155+ * And some plugins aren't stable enough to be repeatedly
156+ * popped and pushed...
157+ */
158+ blackList.insert (name);
159+ }
160 }
161
162 if (p)
163@@ -1051,9 +1008,6 @@
164
165 foreach (CompPlugin *pp, pop)
166 CompPlugin::unload (pp);
167-
168- if (!dirtyPluginList)
169- screen->setOptionForPlugin ("core", "active_plugins", plugin);
170 }
171
172 /* from fvwm2, Copyright Matthias Clasen, Dominik Vogt */
173@@ -4626,9 +4580,6 @@
174
175 optionSetToggleWindowShadedKeyInitiate (CompScreenImpl::shadeWin);
176
177- if (isDirtyPluginList ())
178- updatePlugins ();
179-
180 return true;
181 }
182

Subscribers

People subscribed via source and target branches