Merge lp:~mhr3/unity/faster-models into lp:unity

Proposed by Michal Hruby
Status: Merged
Approved by: Michal Hruby
Approved revision: 2656
Merged at revision: 2688
Proposed branch: lp:~mhr3/unity/faster-models
Merge into: lp:unity
Diff against target: 334 lines (+67/-35)
10 files modified
UnityCore/Model-inl.h (+21/-6)
UnityCore/Model.h (+4/-0)
UnityCore/ModelRowAdaptor.cpp (+12/-5)
UnityCore/ModelRowAdaptor.h (+7/-5)
UnityCore/ResultIterator.cpp (+2/-10)
UnityCore/ResultIterator.h (+1/-2)
dash/LensView.cpp (+12/-3)
dash/LensView.h (+1/-0)
dash/ResultView.cpp (+5/-4)
dash/ResultView.h (+2/-0)
To merge this branch: bzr merge lp:~mhr3/unity/faster-models
Reviewer Review Type Date Requested Status
Gord Allott (community) Approve
Tim Penhey Pending
Review via email: mp+122421@code.launchpad.net

Commit message

Avoid construction of row wrappers

Description of the change

Stop re-constructing row wrapper instances when dee models change. Instead of complete instance creation, only set 3 pointers. This saves a lot of CPU cycles that were wasted by setting getter functions for nux properties, and improves responsiveness when there are lots of results in the dash.

Note that this change limits how we can manipulate the models - for example removing a row from within the row-removed handler could cause invalidation of the result, but since we're not using the models in such recursive way, it's safe.

To post a comment you must log in.
Revision history for this message
Gord Allott (gordallott) wrote :

+1 looks good to me

review: Approve
Revision history for this message
Unity Merger (unity-merger) wrote :

The Jenkins job https://jenkins.qa.ubuntu.com/job/automerge-unity/1282/console reported an error when processing this lp:~mhr3/unity/faster-models branch.
Not merging it.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'UnityCore/Model-inl.h'
2--- UnityCore/Model-inl.h 2012-08-17 11:18:38 +0000
3+++ UnityCore/Model-inl.h 2012-09-13 09:34:19 +0000
4@@ -35,6 +35,9 @@
5 template<class RowAdaptor>
6 Model<RowAdaptor>::Model()
7 : model_type_(ModelType::REMOTE)
8+ , cached_adaptor1_(nullptr, nullptr, nullptr)
9+ , cached_adaptor2_(nullptr, nullptr, nullptr)
10+ , cached_adaptor3_(nullptr, nullptr, nullptr)
11 {
12 Init();
13 }
14@@ -42,6 +45,9 @@
15 template<class RowAdaptor>
16 Model<RowAdaptor>::Model (ModelType model_type)
17 : model_type_(model_type)
18+ , cached_adaptor1_(nullptr, nullptr, nullptr)
19+ , cached_adaptor2_(nullptr, nullptr, nullptr)
20+ , cached_adaptor3_(nullptr, nullptr, nullptr)
21 {
22 Init();
23
24@@ -118,22 +124,31 @@
25 template<class RowAdaptor>
26 void Model<RowAdaptor>::OnRowAdded(DeeModel* model, DeeModelIter* iter)
27 {
28- RowAdaptor it(model, iter, renderer_tag_);
29- row_added.emit(it);
30+ // careful here - adding rows to the model inside the callback
31+ // will invalidate the cached_adaptor!
32+ // This needs to be used as a listener only!
33+ cached_adaptor1_.SetTarget(model, iter, renderer_tag_);
34+ row_added.emit(cached_adaptor1_);
35 }
36
37 template<class RowAdaptor>
38 void Model<RowAdaptor>::OnRowChanged(DeeModel* model, DeeModelIter* iter)
39 {
40- RowAdaptor it(model, iter, renderer_tag_);
41- row_changed.emit(it);
42+ // careful here - changing rows inside the callback will invalidate
43+ // the cached_adaptor!
44+ // This needs to be used as a listener only!
45+ cached_adaptor2_.SetTarget(model, iter, renderer_tag_);
46+ row_changed.emit(cached_adaptor2_);
47 }
48
49 template<class RowAdaptor>
50 void Model<RowAdaptor>::OnRowRemoved(DeeModel* model, DeeModelIter* iter)
51 {
52- RowAdaptor it(model, iter, renderer_tag_);
53- row_removed.emit(it);
54+ // careful here - removing rows from the model inside the callback
55+ // will invalidate the cached_adaptor!
56+ // This needs to be used as a listener only!
57+ cached_adaptor3_.SetTarget(model, iter, renderer_tag_);
58+ row_removed.emit(cached_adaptor3_);
59 }
60
61 template<class RowAdaptor>
62
63=== modified file 'UnityCore/Model.h'
64--- UnityCore/Model.h 2012-04-23 12:29:20 +0000
65+++ UnityCore/Model.h 2012-09-13 09:34:19 +0000
66@@ -88,6 +88,10 @@
67 glib::SignalManager sig_manager_;
68 DeeModelTag* renderer_tag_;
69 ModelType model_type_;
70+
71+ RowAdaptor cached_adaptor1_;
72+ RowAdaptor cached_adaptor2_;
73+ RowAdaptor cached_adaptor3_;
74 };
75
76 }
77
78=== modified file 'UnityCore/ModelRowAdaptor.cpp'
79--- UnityCore/ModelRowAdaptor.cpp 2012-06-19 16:47:56 +0000
80+++ UnityCore/ModelRowAdaptor.cpp 2012-09-13 09:34:19 +0000
81@@ -45,7 +45,14 @@
82 return *this;
83 }
84
85-std::string RowAdaptorBase::GetStringAt(int position)
86+void RowAdaptorBase::SetTarget(DeeModel* model, DeeModelIter* iter, DeeModelTag* tag)
87+{
88+ model_ = model;
89+ iter_ = iter;
90+ tag_ = tag;
91+}
92+
93+std::string RowAdaptorBase::GetStringAt(int position) const
94 {
95 if (!model_ || !iter_)
96 return "";
97@@ -56,28 +63,28 @@
98 return ""; // std::strings don't like null.
99 }
100
101-bool RowAdaptorBase::GetBoolAt(int position)
102+bool RowAdaptorBase::GetBoolAt(int position) const
103 {
104 if (!model_ || !iter_)
105 return 0;
106 return dee_model_get_bool(model_, iter_, position);
107 }
108
109-int RowAdaptorBase::GetIntAt(int position)
110+int RowAdaptorBase::GetIntAt(int position) const
111 {
112 if (!model_ || !iter_)
113 return 0;
114 return dee_model_get_int32(model_, iter_, position);
115 }
116
117-unsigned int RowAdaptorBase::GetUIntAt(int position)
118+unsigned int RowAdaptorBase::GetUIntAt(int position) const
119 {
120 if (!model_ || !iter_)
121 return 0;
122 return dee_model_get_uint32(model_, iter_, position);
123 }
124
125-float RowAdaptorBase::GetFloatAt(int position)
126+float RowAdaptorBase::GetFloatAt(int position) const
127 {
128 if (!model_ || !iter_)
129 return 0.0;
130
131=== modified file 'UnityCore/ModelRowAdaptor.h'
132--- UnityCore/ModelRowAdaptor.h 2012-06-19 16:47:56 +0000
133+++ UnityCore/ModelRowAdaptor.h 2012-09-13 09:34:19 +0000
134@@ -52,11 +52,13 @@
135 RowAdaptorBase(RowAdaptorBase const& other);
136 RowAdaptorBase& operator=(RowAdaptorBase const& other);
137
138- std::string GetStringAt(int position);
139- bool GetBoolAt(int position);
140- int GetIntAt(int position);
141- unsigned int GetUIntAt(int position);
142- float GetFloatAt(int position);
143+ std::string GetStringAt(int position) const;
144+ bool GetBoolAt(int position) const;
145+ int GetIntAt(int position) const;
146+ unsigned int GetUIntAt(int position) const;
147+ float GetFloatAt(int position) const;
148+
149+ void SetTarget(DeeModel* model, DeeModelIter* iter, DeeModelTag* tag);
150
151 template<typename T>
152 void set_renderer(T renderer);
153
154=== modified file 'UnityCore/ResultIterator.cpp'
155--- UnityCore/ResultIterator.cpp 2012-08-13 08:18:25 +0000
156+++ UnityCore/ResultIterator.cpp 2012-09-13 09:34:19 +0000
157@@ -34,7 +34,6 @@
158 , iter_(model ? dee_model_get_first_iter(model) : NULL)
159 , tag_(NULL)
160 , iter_result_(model_, iter_, tag_)
161- , cache_invalidated_(false)
162 {
163 }
164
165@@ -43,7 +42,6 @@
166 , iter_(iter)
167 , tag_(tag)
168 , iter_result_(model_, iter_, tag_)
169- , cache_invalidated_(false)
170 {
171 }
172
173@@ -57,8 +55,7 @@
174 model_ = rhs.model_;
175 iter_ = rhs.iter_;
176 tag_ = rhs.tag_;
177- iter_result_ = Result(model_, iter_, tag_);
178- cache_invalidated_ = false;
179+ iter_result_.SetTarget(model_, iter_, tag_);
180
181 return *this;
182 }
183@@ -66,7 +63,6 @@
184 ResultIterator& ResultIterator::operator++()
185 {
186 iter_ = dee_model_next(model_, iter_);
187- cache_invalidated_ = true;
188 return *this;
189 }
190
191@@ -78,7 +74,6 @@
192 for (int index = 0; index < count; index++)
193 iter_ = dee_model_next(model_, iter_);
194
195- cache_invalidated_ = true;
196 return *this;
197 }
198
199@@ -99,7 +94,6 @@
200 ResultIterator& ResultIterator::operator--()
201 {
202 iter_ = dee_model_prev(model_, iter_);
203- cache_invalidated_ = true;
204 return *this;
205 }
206
207@@ -111,7 +105,6 @@
208 for (int index = 0; index < count; index++)
209 iter_ = dee_model_prev(model_, iter_);
210
211- cache_invalidated_ = true;
212 return *this;
213 }
214
215@@ -131,8 +124,7 @@
216
217 Result& ResultIterator::operator*()
218 {
219- if (cache_invalidated_)
220- iter_result_ = Result(model_, iter_, tag_);
221+ iter_result_.SetTarget(model_, iter_, tag_);
222 return iter_result_;
223 }
224
225
226=== modified file 'UnityCore/ResultIterator.h'
227--- UnityCore/ResultIterator.h 2012-08-13 08:18:25 +0000
228+++ UnityCore/ResultIterator.h 2012-09-13 09:34:19 +0000
229@@ -37,7 +37,7 @@
230 public:
231 ResultIterator(glib::Object<DeeModel> model);
232 ResultIterator(glib::Object<DeeModel> model, DeeModelIter* iter_, DeeModelTag* tag);
233- ResultIterator(ResultIterator const& copy) : model_(copy.model_), iter_(copy.iter_), tag_(copy.tag_), iter_result_(copy.iter_result_), cache_invalidated_(false){};
234+ ResultIterator(ResultIterator const& copy) : model_(copy.model_), iter_(copy.iter_), tag_(copy.tag_), iter_result_(copy.iter_result_){};
235
236 ResultIterator& operator=(ResultIterator const& rhs);
237
238@@ -93,7 +93,6 @@
239 DeeModelIter* iter_;
240 DeeModelTag* tag_;
241 Result iter_result_;
242- bool cache_invalidated_;
243 };
244
245 }
246
247=== modified file 'dash/LensView.cpp'
248--- dash/LensView.cpp 2012-08-31 15:46:18 +0000
249+++ dash/LensView.cpp 2012-09-13 09:34:19 +0000
250@@ -440,6 +440,8 @@
251 void LensView::OnResultAdded(Result const& result)
252 {
253 try {
254+ // Anything done in this method needs to be super fast, if in doubt, add
255+ // it to the model_updated_timeout_ callback!
256 PlacesGroup* group = categories_.at(result.category_index);
257
258 std::string uri = result.uri;
259@@ -453,9 +455,16 @@
260 CheckNoResults(Lens::Hints());
261 }
262
263- // Check if all results so far are from one category
264- // If so, then expand that category.
265- CheckCategoryExpansion();
266+ if (!model_updated_timeout_)
267+ {
268+ model_updated_timeout_.reset(new glib::Idle([&] () {
269+ // Check if all results so far are from one category
270+ // If so, then expand that category.
271+ CheckCategoryExpansion();
272+ model_updated_timeout_.reset();
273+ return false;
274+ }, glib::Source::Priority::HIGH));
275+ }
276 } catch (std::out_of_range& oor) {
277 LOG_WARN(logger) << "Result does not have a valid category index: "
278 << boost::lexical_cast<unsigned int>(result.category_index)
279
280=== modified file 'dash/LensView.h'
281--- dash/LensView.h 2012-08-29 14:50:19 +0000
282+++ dash/LensView.h 2012-09-13 09:34:19 +0000
283@@ -126,6 +126,7 @@
284
285 UBusManager ubus_manager_;
286 glib::Source::UniquePtr fix_rendering_idle_;
287+ glib::Source::UniquePtr model_updated_timeout_;
288 int last_good_filter_model_;
289 glib::Source::UniquePtr fix_filter_models_idle_;
290 };
291
292=== modified file 'dash/ResultView.cpp'
293--- dash/ResultView.cpp 2012-08-24 08:43:50 +0000
294+++ dash/ResultView.cpp 2012-09-13 09:34:19 +0000
295@@ -44,6 +44,7 @@
296 : View(NUX_FILE_LINE_PARAM)
297 , expanded(true)
298 , renderer_(NULL)
299+ , cached_result_(nullptr, nullptr, nullptr)
300 {
301 expanded.changed.connect([&](bool value)
302 {
303@@ -99,14 +100,14 @@
304
305 void ResultView::OnRowAdded(DeeModel* model, DeeModelIter* iter)
306 {
307- Result result(model, iter, renderer_tag_);
308- AddResult(result);
309+ cached_result_.SetTarget(model, iter, renderer_tag_);
310+ AddResult(cached_result_);
311 }
312
313 void ResultView::OnRowRemoved(DeeModel* model, DeeModelIter* iter)
314 {
315- Result result(model, iter, renderer_tag_);
316- RemoveResult(result);
317+ cached_result_.SetTarget(model, iter, renderer_tag_);
318+ RemoveResult(cached_result_);
319 }
320
321 void ResultView::SetModel(glib::Object<DeeModel> const& model, DeeModelTag* tag)
322
323=== modified file 'dash/ResultView.h'
324--- dash/ResultView.h 2012-08-22 08:16:02 +0000
325+++ dash/ResultView.h 2012-09-13 09:34:19 +0000
326@@ -91,6 +91,8 @@
327 private:
328 void OnRowAdded(DeeModel* model, DeeModelIter* iter);
329 void OnRowRemoved(DeeModel* model, DeeModelIter* iter);
330+
331+ Result cached_result_;
332 };
333
334 }