Merge lp:~kamstrup/dee/less-gtype-dancing into lp:dee

[Mikkel Kamstrup Erlandsen]

A series of optimizations for DeeSequenceModel. Notably benchmarks report improvements:

append+prepend: ~10%
sorted: ~5%
read_string: ~25%
read_row: ~50%
clear: ~10%
walk_next: ~50%
walk_pos: ~15%
FilterModel.collate,collate_desc,sort_uint: ~5-10%

Probably many small gains here and there.

[Mikkel Kamstrup Erlandsen (kamstrup)]

I am not entirely sure what the impact of this is...

269 - [CCode (array_length = false, array_null_terminated = true)]
270 - public GLib.Variant[] get_row (Dee.ModelIter iter, [CCode (array_length = false)] out GLib.Variant[] out_row_members = null);
271 + [NoWrapper]
272 + public abstract GLib.Variant get_row (Dee.ModelIter iter, GLib.Variant out_row_members = null);

[Michal Hruby (mhr3)]

Faster everything, yey!

This seems to have broken gir a little and vapigen now can't match the virtual get_row to the invoker method, could you move the whole get_row method to -custom.vala (with all the annotations there were + make it abstract).

review: Needs Fixing

[Mikkel Kamstrup Erlandsen (kamstrup)]

> Faster everything, yey!
>
> This seems to have broken gir a little and vapigen now can't match the virtual
> get_row to the invoker method, could you move the whole get_row method to
> -custom.vala (with all the annotations there were + make it abstract).

Pushed as r352

[Michal Hruby (mhr3)]

Awesome! :)

review: Approve

=== modified file 'src/dee-model.c'
--- src/dee-model.c	2012-01-26 18:30:57 +0000
+++ src/dee-model.c	2012-02-21 12:37:14 +0000
@@ -1436,26 +1436,19 @@
  *          or a newly allocated array otherwise which you must free
  *          with g_free(). The variants in the array will have a strong
  *          reference and needs to be freed with g_variant_unref().
- **/
+ */
 GVariant**
 dee_model_get_row (DeeModel      *self,
                    DeeModelIter  *iter,
                    GVariant     **out_row_members)
 {
-  guint            col, n_cols;
+  DeeModelIface *iface;
 
   g_return_val_if_fail (DEE_IS_MODEL (self), NULL);
-  g_return_val_if_fail (iter != NULL, NULL);
-
-  n_cols = dee_model_get_n_columns (self);
-
-  if (out_row_members == NULL)
-    out_row_members = g_new0 (GVariant*, n_cols + 1);
-
-  for (col = 0; col < n_cols; col++)
-    out_row_members[col] = dee_model_get_value (self, iter, col);
-
-  return out_row_members;
+
+  iface = DEE_MODEL_GET_IFACE (self);
+
+  return (* iface->get_row) (self, iter, out_row_members);
 }
 
 /**
 
=== modified file 'src/dee-model.h'
--- src/dee-model.h	2012-01-26 18:30:57 +0000
+++ src/dee-model.h	2012-02-21 12:37:14 +0000
@@ -215,6 +215,10 @@
                                      DeeModelIter   *iter,
                                      DeeModelTag    *tag,
                                      gpointer        value);
+  
+  GVariant**     (*get_row)         (DeeModel       *self,
+                                     DeeModelIter   *iter,
+                                     GVariant      **out_row_members);
 
   /*< private >*/
   void     (*_dee_model_1) (void);
@@ -224,7 +228,6 @@
   void     (*_dee_model_5) (void);
   void     (*_dee_model_6) (void);
   void     (*_dee_model_7) (void);
-  void     (*_dee_model_8) (void);
 };
 
 GType           dee_model_iter_get_type         (void);
 
=== modified file 'src/dee-sequence-model.c'
--- src/dee-sequence-model.c	2012-01-10 07:43:54 +0000
+++ src/dee-sequence-model.c	2012-02-21 12:37:14 +0000
@@ -128,6 +128,10 @@
                                                         DeeModelIter *iter,
                                                         guint         column);
 
+static GVariant**    dee_sequence_model_get_row        (DeeModel     *self,
+                                                        DeeModelIter *iter,
+                                                        GVariant    **out_row_members);
+
 static DeeModelIter* dee_sequence_model_get_first_iter  (DeeModel     *self);
 
 static DeeModelIter* dee_sequence_model_get_last_iter   (DeeModel     *self);
@@ -295,6 +299,7 @@
   iface->set_row              = dee_sequence_model_set_row;
   iface->set_value            = dee_sequence_model_set_value;
   iface->get_value            = dee_sequence_model_get_value;
+  iface->get_row              = dee_sequence_model_get_row;
   iface->get_first_iter       = dee_sequence_model_get_first_iter;
   iface->get_last_iter        = dee_sequence_model_get_last_iter;
   iface->get_iter_at_row      = dee_sequence_model_get_iter_at_row;
@@ -469,9 +474,7 @@
 
   g_return_if_fail (DEE_IS_SEQUENCE_MODEL (_self));
   g_return_if_fail (iter != NULL);
-
-  if (iter_ == dee_model_get_last_iter (self))
-    return;
+  g_return_if_fail (!g_sequence_iter_is_end (iter));
 
   if (iter)
     {
@@ -500,6 +503,7 @@
   g_return_if_fail (DEE_IS_SEQUENCE_MODEL (_self));
   g_return_if_fail (iter != NULL);
   g_return_if_fail (value != NULL);
+  g_return_if_fail (column < dee_model_get_n_columns (self));
   
   priv = _self->priv;
   
@@ -546,14 +550,9 @@
                                        guint          column,
                                        GVariant      *value)
 {
-  DeeSequenceModel        *_self = (DeeSequenceModel *)self;
   gpointer                *row;
 
-  g_return_if_fail (DEE_IS_SEQUENCE_MODEL (_self));
-  g_return_if_fail (iter != NULL);
-  g_return_if_fail (column < dee_model_get_n_columns (self));
   g_return_if_fail (g_variant_type_equal (g_variant_get_type (value), G_VARIANT_TYPE (dee_model_get_column_schema (self, column))));
-  g_return_if_fail (value != NULL);
 
   row = g_sequence_get ((GSequenceIter *) iter);
 
@@ -576,13 +575,8 @@
                                DeeModelIter *iter,
                                guint         column)
 {
-  DeeSequenceModel *_self = (DeeSequenceModel *)self;
   gpointer         *row;
 
-  g_return_val_if_fail (DEE_IS_SEQUENCE_MODEL (_self), NULL);
-  g_return_val_if_fail (iter != NULL, NULL);
-  g_return_val_if_fail (column < dee_model_get_n_columns (self), NULL);
-
   row = g_sequence_get ((GSequenceIter *) iter);
   if (G_UNLIKELY (row == NULL))
     {
@@ -600,6 +594,10 @@
                               DeeModelIter *iter,
                               guint         column)
 {
+  g_return_val_if_fail (DEE_IS_SEQUENCE_MODEL (self), NULL);
+  g_return_val_if_fail (iter != NULL, NULL);
+  g_return_val_if_fail (column < dee_model_get_n_columns (self), NULL);
+
   GVariant *val = dee_sequence_model_peek_value (self, iter, column);
 
   if (G_UNLIKELY (val == NULL))
@@ -613,6 +611,30 @@
   return g_variant_ref (val);
 }
 
+static GVariant**
+dee_sequence_model_get_row (DeeModel      *self,
+                            DeeModelIter  *iter,
+                            GVariant     **out_row_members)
+{
+  guint            col, n_cols;
+
+  g_return_val_if_fail (DEE_IS_SEQUENCE_MODEL (self), NULL);
+
+  n_cols = dee_model_get_n_columns (self);
+
+  if (out_row_members == NULL)
+    out_row_members = g_new0 (GVariant*, n_cols + 1);
+
+  /* We use peek_value() here because it saves us from some expensive checks
+   * compared to get_value(), that we can guarantee from this call site anyway
+   */
+  for (col = 0; col < n_cols; col++)
+    out_row_members[col] = g_variant_ref (
+                               dee_sequence_model_peek_value (self, iter, col));
+
+  return out_row_members;
+}
+
 static DeeModelIter*
 dee_sequence_model_get_first_iter (DeeModel     *self)
 {
@@ -794,7 +816,7 @@
 {
   g_return_val_if_fail (DEE_IS_SEQUENCE_MODEL (self), NULL);
   g_return_val_if_fail (iter, NULL);
-  g_return_val_if_fail (!dee_model_is_last (self, iter), NULL);
+  g_return_val_if_fail (!g_sequence_iter_is_end ((GSequenceIter*) iter), NULL);
 
   return (DeeModelIter *) g_sequence_iter_next ((GSequenceIter *)iter);
 }
@@ -805,7 +827,7 @@
 {
   g_return_val_if_fail (DEE_IS_SEQUENCE_MODEL (self), NULL);
   g_return_val_if_fail (iter, NULL);
-  g_return_val_if_fail (!dee_model_is_first (self, iter), NULL);
+  g_return_val_if_fail (!g_sequence_iter_is_begin ((GSequenceIter*) iter), NULL);
 
   return (DeeModelIter *) g_sequence_iter_prev ((GSequenceIter *)iter);
 }
 
=== modified file 'src/dee-serializable-model.c'
--- src/dee-serializable-model.c	2012-01-09 11:17:10 +0000
+++ src/dee-serializable-model.c	2012-02-21 12:37:14 +0000
@@ -144,6 +144,10 @@
                                                           DeeModelIter *iter,
                                                           guint         column);
 
+static GVariant**    dee_serializable_model_get_row         (DeeModel    *self,
+                                                             DeeModelIter *iter,
+                                                             GVariant    **out_row_members);
+
 static DeeModelIter* dee_serializable_model_get_first_iter  (DeeModel     *self);
 
 static DeeModelIter* dee_serializable_model_get_last_iter   (DeeModel     *self);
@@ -456,16 +460,16 @@
 static void
 dee_serializable_model_clear (DeeModel *self)
 {
-  DeeModelIter            *iter;
+  DeeModelIter            *iter, *end;
 
   g_return_if_fail (DEE_IS_SERIALIZABLE_MODEL (self));
 
   iter = dee_model_get_first_iter (self);
+  end = dee_model_get_last_iter (self);
 
-  while (!dee_model_is_last (self, iter))
+  while (iter != end)
     {
       dee_model_remove (self, iter);
-
       iter = dee_model_get_first_iter (self);      
     }
 }
@@ -614,6 +618,26 @@
   return NULL;
 }
 
+static GVariant**
+dee_serializable_model_get_row (DeeModel      *self,
+                                DeeModelIter  *iter,
+                                GVariant     **out_row_members)
+{
+  guint            col, n_cols;
+
+  g_return_val_if_fail (DEE_IS_SERIALIZABLE_MODEL (self), NULL);
+
+  n_cols = dee_model_get_n_columns (self);
+
+  if (out_row_members == NULL)
+    out_row_members = g_new0 (GVariant*, n_cols + 1);
+
+  for (col = 0; col < n_cols; col++)
+    out_row_members[col] = dee_model_get_value (self, iter, col);
+
+  return out_row_members;
+}
+
 static gboolean
 dee_serializable_model_get_bool (DeeModel     *self,
                                  DeeModelIter *iter,
@@ -1111,6 +1135,7 @@
   iface->set_value            = dee_serializable_model_set_value;
   iface->set_row              = dee_serializable_model_set_row;
   iface->get_value            = dee_serializable_model_get_value;
+  iface->get_row              = dee_serializable_model_get_row;
   iface->get_first_iter       = dee_serializable_model_get_first_iter;
   iface->get_last_iter        = dee_serializable_model_get_last_iter;
   iface->get_iter_at_row      = dee_serializable_model_get_iter_at_row;
 
=== modified file 'vapi/Dee-1.0-custom.vala'
--- vapi/Dee-1.0-custom.vala	2012-02-19 17:19:33 +0000
+++ vapi/Dee-1.0-custom.vala	2012-02-21 12:37:14 +0000
@@ -1,5 +1,11 @@
 namespace Dee {
 
+  [CCode (type_id = "dee_model_get_type ()")]
+  public interface Model {
+    [CCode (array_length = false, array_null_terminated = true)]
+    public abstract GLib.Variant[] get_row (Dee.ModelIter iter, [CCode (array_length = false)] out GLib.Variant[] out_row_members = null);
+  }
+
 	public struct Filter {
 		[CCode (cname = "destroy")]
 		public GLib.DestroyNotify destroy_notify;
 
=== modified file 'vapi/Dee-1.0.metadata'
--- vapi/Dee-1.0.metadata	2012-01-26 10:51:42 +0000
+++ vapi/Dee-1.0.metadata	2012-02-21 12:37:14 +0000
@@ -6,7 +6,7 @@
 Model
 	.append skip=false
 	.get skip=false
-	.get_row.out_row_members default=null
+	.get_row skip
 	.insert skip=false
 	.insert_before skip=false
 	.insert_sorted skip=false
 
=== modified file 'vapi/dee-1.0.vapi'
--- vapi/dee-1.0.vapi	2012-02-19 17:19:33 +0000
+++ vapi/dee-1.0.vapi	2012-02-21 12:37:14 +0000
@@ -217,7 +217,7 @@
 		public abstract uint get_n_rows ();
 		public abstract uint get_position (Dee.ModelIter iter);
 		[CCode (array_length = false, array_null_terminated = true)]
-		public GLib.Variant[] get_row (Dee.ModelIter iter, [CCode (array_length = false)] out GLib.Variant[] out_row_members = null);
+		public abstract GLib.Variant[] get_row (Dee.ModelIter iter, [CCode (array_length = false)] out GLib.Variant[] out_row_members = null);
 		[CCode (array_length_pos = 0.1, array_length_type = "guint")]
 		public abstract unowned string[] get_schema ();
 		public abstract unowned string get_string (Dee.ModelIter iter, uint column);