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

[Michal Hruby]

Avoid construction of row wrappers

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.

[Gord Allott (gordallott)]

+1 looks good to me

review: Approve

[Unity Merger (unity-merger)]

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.

=== modified file 'UnityCore/Model-inl.h'
--- UnityCore/Model-inl.h	2012-08-17 11:18:38 +0000
+++ UnityCore/Model-inl.h	2012-09-13 09:34:19 +0000
@@ -35,6 +35,9 @@
 template<class RowAdaptor>
 Model<RowAdaptor>::Model()
   : model_type_(ModelType::REMOTE)
+  , cached_adaptor1_(nullptr, nullptr, nullptr)
+  , cached_adaptor2_(nullptr, nullptr, nullptr)
+  , cached_adaptor3_(nullptr, nullptr, nullptr)
 {
   Init();
 }
@@ -42,6 +45,9 @@
 template<class RowAdaptor>
 Model<RowAdaptor>::Model (ModelType model_type)
   : model_type_(model_type)
+  , cached_adaptor1_(nullptr, nullptr, nullptr)
+  , cached_adaptor2_(nullptr, nullptr, nullptr)
+  , cached_adaptor3_(nullptr, nullptr, nullptr)
 {
   Init();
 
@@ -118,22 +124,31 @@
 template<class RowAdaptor>
 void Model<RowAdaptor>::OnRowAdded(DeeModel* model, DeeModelIter* iter)
 {
-  RowAdaptor it(model, iter, renderer_tag_);
-  row_added.emit(it);
+  // careful here - adding rows to the model inside the callback
+  // will invalidate the cached_adaptor!
+  // This needs to be used as a listener only!
+  cached_adaptor1_.SetTarget(model, iter, renderer_tag_);
+  row_added.emit(cached_adaptor1_);
 }
 
 template<class RowAdaptor>
 void Model<RowAdaptor>::OnRowChanged(DeeModel* model, DeeModelIter* iter)
 {
-  RowAdaptor it(model, iter, renderer_tag_);
-  row_changed.emit(it);
+  // careful here - changing rows inside the callback will invalidate
+  // the cached_adaptor!
+  // This needs to be used as a listener only!
+  cached_adaptor2_.SetTarget(model, iter, renderer_tag_);
+  row_changed.emit(cached_adaptor2_);
 }
 
 template<class RowAdaptor>
 void Model<RowAdaptor>::OnRowRemoved(DeeModel* model, DeeModelIter* iter)
 {
-  RowAdaptor it(model, iter, renderer_tag_);
-  row_removed.emit(it);
+  // careful here - removing rows from the model inside the callback
+  // will invalidate the cached_adaptor!
+  // This needs to be used as a listener only!
+  cached_adaptor3_.SetTarget(model, iter, renderer_tag_);
+  row_removed.emit(cached_adaptor3_);
 }
 
 template<class RowAdaptor>

=== modified file 'UnityCore/Model.h'
--- UnityCore/Model.h	2012-04-23 12:29:20 +0000
+++ UnityCore/Model.h	2012-09-13 09:34:19 +0000
@@ -88,6 +88,10 @@
   glib::SignalManager sig_manager_;
   DeeModelTag* renderer_tag_;
   ModelType model_type_;
+
+  RowAdaptor cached_adaptor1_;
+  RowAdaptor cached_adaptor2_;
+  RowAdaptor cached_adaptor3_;
 };
 
 }

=== modified file 'UnityCore/ModelRowAdaptor.cpp'
--- UnityCore/ModelRowAdaptor.cpp	2012-06-19 16:47:56 +0000
+++ UnityCore/ModelRowAdaptor.cpp	2012-09-13 09:34:19 +0000
@@ -45,7 +45,14 @@
   return *this;
 }
 
-std::string RowAdaptorBase::GetStringAt(int position)
+void RowAdaptorBase::SetTarget(DeeModel* model, DeeModelIter* iter, DeeModelTag* tag)
+{
+  model_ = model;
+  iter_ = iter;
+  tag_ = tag;
+}
+
+std::string RowAdaptorBase::GetStringAt(int position) const
 {
   if (!model_ || !iter_)
     return "";
@@ -56,28 +63,28 @@
     return ""; // std::strings don't like null.
 }
 
-bool RowAdaptorBase::GetBoolAt(int position)
+bool RowAdaptorBase::GetBoolAt(int position) const
 {
   if (!model_ || !iter_)
     return 0;
   return dee_model_get_bool(model_, iter_, position);
 }
 
-int RowAdaptorBase::GetIntAt(int position)
+int RowAdaptorBase::GetIntAt(int position) const
 {
   if (!model_ || !iter_)
     return 0;
   return dee_model_get_int32(model_, iter_, position);
 }
 
-unsigned int RowAdaptorBase::GetUIntAt(int position)
+unsigned int RowAdaptorBase::GetUIntAt(int position) const
 {
   if (!model_ || !iter_)
     return 0;
   return dee_model_get_uint32(model_, iter_, position);
 }
 
-float RowAdaptorBase::GetFloatAt(int position)
+float RowAdaptorBase::GetFloatAt(int position) const
 {
   if (!model_ || !iter_)
     return 0.0;

=== modified file 'UnityCore/ModelRowAdaptor.h'
--- UnityCore/ModelRowAdaptor.h	2012-06-19 16:47:56 +0000
+++ UnityCore/ModelRowAdaptor.h	2012-09-13 09:34:19 +0000
@@ -52,11 +52,13 @@
   RowAdaptorBase(RowAdaptorBase const& other);
   RowAdaptorBase& operator=(RowAdaptorBase const& other);
 
-  std::string GetStringAt(int position);
-  bool GetBoolAt(int position);
-  int GetIntAt(int position);
-  unsigned int GetUIntAt(int position);
-  float GetFloatAt(int position);
+  std::string GetStringAt(int position) const;
+  bool GetBoolAt(int position) const;
+  int GetIntAt(int position) const;
+  unsigned int GetUIntAt(int position) const;
+  float GetFloatAt(int position) const;
+
+  void SetTarget(DeeModel* model, DeeModelIter* iter, DeeModelTag* tag);
 
   template<typename T>
   void set_renderer(T renderer);

=== modified file 'UnityCore/ResultIterator.cpp'
--- UnityCore/ResultIterator.cpp	2012-08-13 08:18:25 +0000
+++ UnityCore/ResultIterator.cpp	2012-09-13 09:34:19 +0000
@@ -34,7 +34,6 @@
   , iter_(model ? dee_model_get_first_iter(model) : NULL)
   , tag_(NULL)
   , iter_result_(model_, iter_, tag_)
-  , cache_invalidated_(false)
 {
 }
 
@@ -43,7 +42,6 @@
   , iter_(iter)
   , tag_(tag)
   , iter_result_(model_, iter_, tag_)
-  , cache_invalidated_(false)
 {
 }
 
@@ -57,8 +55,7 @@
   model_ = rhs.model_;
   iter_ = rhs.iter_;
   tag_ = rhs.tag_;
-  iter_result_ = Result(model_, iter_, tag_);
-  cache_invalidated_ = false;
+  iter_result_.SetTarget(model_, iter_, tag_);
 
   return *this;
 }
@@ -66,7 +63,6 @@
 ResultIterator& ResultIterator::operator++()
 {
   iter_ = dee_model_next(model_, iter_);
-  cache_invalidated_ = true;
   return *this;
 }
 
@@ -78,7 +74,6 @@
   for (int index = 0; index < count; index++)
     iter_ = dee_model_next(model_, iter_);
   
-  cache_invalidated_ = true;
   return *this;
 }
 
@@ -99,7 +94,6 @@
 ResultIterator& ResultIterator::operator--()
 {
   iter_ = dee_model_prev(model_, iter_);
-  cache_invalidated_ = true;
   return *this;
 }
 
@@ -111,7 +105,6 @@
   for (int index = 0; index < count; index++)
     iter_ = dee_model_prev(model_, iter_);
 
-  cache_invalidated_ = true;
   return *this;
 }
 
@@ -131,8 +124,7 @@
 
 Result& ResultIterator::operator*()
 {
-  if (cache_invalidated_)
-    iter_result_ = Result(model_, iter_, tag_);
+  iter_result_.SetTarget(model_, iter_, tag_);
   return iter_result_;
 }
 

=== modified file 'UnityCore/ResultIterator.h'
--- UnityCore/ResultIterator.h	2012-08-13 08:18:25 +0000
+++ UnityCore/ResultIterator.h	2012-09-13 09:34:19 +0000
@@ -37,7 +37,7 @@
 public:
   ResultIterator(glib::Object<DeeModel> model);
   ResultIterator(glib::Object<DeeModel> model, DeeModelIter* iter_, DeeModelTag* tag);
-  ResultIterator(ResultIterator const& copy) : model_(copy.model_), iter_(copy.iter_), tag_(copy.tag_), iter_result_(copy.iter_result_), cache_invalidated_(false){};
+  ResultIterator(ResultIterator const& copy) : model_(copy.model_), iter_(copy.iter_), tag_(copy.tag_), iter_result_(copy.iter_result_){};
 
   ResultIterator& operator=(ResultIterator const& rhs);
 
@@ -93,7 +93,6 @@
   DeeModelIter* iter_;
   DeeModelTag* tag_;
   Result iter_result_;  
-  bool cache_invalidated_;
 };
 
 }

=== modified file 'dash/LensView.cpp'
--- dash/LensView.cpp	2012-08-31 15:46:18 +0000
+++ dash/LensView.cpp	2012-09-13 09:34:19 +0000
@@ -440,6 +440,8 @@
 void LensView::OnResultAdded(Result const& result)
 {
   try {
+    // Anything done in this method needs to be super fast, if in doubt, add
+    // it to the model_updated_timeout_ callback!
     PlacesGroup* group = categories_.at(result.category_index);
 
     std::string uri = result.uri;
@@ -453,9 +455,16 @@
       CheckNoResults(Lens::Hints());
     }
 
-    // Check if all results so far are from one category
-    // If so, then expand that category.
-    CheckCategoryExpansion();
+    if (!model_updated_timeout_)
+    {
+      model_updated_timeout_.reset(new glib::Idle([&] () {
+        // Check if all results so far are from one category
+        // If so, then expand that category.
+        CheckCategoryExpansion();
+        model_updated_timeout_.reset();
+        return false;
+      }, glib::Source::Priority::HIGH));
+    }
   } catch (std::out_of_range& oor) {
     LOG_WARN(logger) << "Result does not have a valid category index: "
                      << boost::lexical_cast<unsigned int>(result.category_index)

=== modified file 'dash/LensView.h'
--- dash/LensView.h	2012-08-29 14:50:19 +0000
+++ dash/LensView.h	2012-09-13 09:34:19 +0000
@@ -126,6 +126,7 @@
 
   UBusManager ubus_manager_;
   glib::Source::UniquePtr fix_rendering_idle_;
+  glib::Source::UniquePtr model_updated_timeout_;
   int last_good_filter_model_;
   glib::Source::UniquePtr fix_filter_models_idle_;
 };

=== modified file 'dash/ResultView.cpp'
--- dash/ResultView.cpp	2012-08-24 08:43:50 +0000
+++ dash/ResultView.cpp	2012-09-13 09:34:19 +0000
@@ -44,6 +44,7 @@
   : View(NUX_FILE_LINE_PARAM)
   , expanded(true)
   , renderer_(NULL)
+  , cached_result_(nullptr, nullptr, nullptr)
 {
   expanded.changed.connect([&](bool value)
   {
@@ -99,14 +100,14 @@
 
 void ResultView::OnRowAdded(DeeModel* model, DeeModelIter* iter)
 {
-  Result result(model, iter, renderer_tag_);
-  AddResult(result);
+  cached_result_.SetTarget(model, iter, renderer_tag_);
+  AddResult(cached_result_);
 }
 
 void ResultView::OnRowRemoved(DeeModel* model, DeeModelIter* iter)
 {
-  Result result(model, iter, renderer_tag_);
-  RemoveResult(result);
+  cached_result_.SetTarget(model, iter, renderer_tag_);
+  RemoveResult(cached_result_);
 }
 
 void ResultView::SetModel(glib::Object<DeeModel> const& model, DeeModelTag* tag)

=== modified file 'dash/ResultView.h'
--- dash/ResultView.h	2012-08-22 08:16:02 +0000
+++ dash/ResultView.h	2012-09-13 09:34:19 +0000
@@ -91,6 +91,8 @@
 private:
   void OnRowAdded(DeeModel* model, DeeModelIter* iter);
   void OnRowRemoved(DeeModel* model, DeeModelIter* iter);
+
+  Result cached_result_;
 };
 
 }