Merge lp:~kamstrup/dee/signal-by-id into lp:dee

[Mikkel Kamstrup Erlandsen]

Emit signal by id, not by name, in DeeSequenceModel for a small perf gain

The benchmark suite indicates a 0 > x > 1% perf gain from this - but non-zero!
I expect the numbers to be slightly better on lower end hardware, and also when
looking on the actual spend CPU time, which I haven't done.

[Michal Hruby (mhr3)]

I'd prefer if we just used static ints initialized during class_init (or perhaps instance init if it's not available during class_init). There's no need to add these to each and every instance.

review: Needs Fixing

[Mikkel Kamstrup Erlandsen (kamstrup)]

Totally agreed, that was a pretty braindead way of doing this :-)

Pushed r321 with the requested changes. Please re-review.

[lp:~kamstrup/dee/signal-by-id]

321. By Mikkel Kamstrup Erlandsen on 2011-12-09

Move signal ids for row-{added,changed,removed} into global static variables instead of instance members

[Michal Hruby (mhr3)]

> Totally agreed, that was a pretty braindead way of doing this :-)

I think that you actually thought about it quite a lot the first time ;) Doing g_signal_lookup during class_init of the type you're looking up does sound very racy, I'm actually surprised that it works.

The question is if we're going to assume that will continue working / move it to instance_init (with a GOnce guard) / or just use what was in r320.

review: Needs Information

[Mikkel Kamstrup Erlandsen (kamstrup)]

I'd assume we could use it in class_init() just fine.

It rests on the assumption that GObject works like this: When
instantiating a DeeSequenceModel for the first time glib first looks
up all the GTypes involved to figure out the hierarchy. Then it call
class_init()s from the top of the inheritance tree (imagine a memcpy()
on the class struct in between each step) like GObject.class_init(),
DeeModel.iface_init(), DeeSerializableModel.class_init(), and lastly
DeeSequenceModel.class_init().

If the parent class/ifaces are not fully initialized when entering
class_init() of the children all sorts of things would go b0rkedI
believe.

[Michal Hruby (mhr3)]

Very well then, let's assume a little. :)

review: Approve

=== modified file 'dee/dee-sequence-model.c'
--- dee/dee-sequence-model.c	2011-10-21 10:37:03 +0000
+++ dee/dee-sequence-model.c	2011-12-09 14:18:23 +0000
@@ -56,6 +56,11 @@
 #define DEE_SEQUENCE_MODEL_GET_PRIVATE(obj) \
   (G_TYPE_INSTANCE_GET_PRIVATE(obj, DEE_TYPE_SEQUENCE_MODEL, DeeSequenceModelPrivate))
 
+/* Signal ids for emitting row update signals a just a smidgeon faster */
+static guint sigid_row_added;
+static guint sigid_row_removed;
+static guint sigid_row_changed;
+
 /**
  * DeeSequenceModelPrivate:
  *
@@ -227,6 +232,11 @@
   obj_class->set_property = dee_sequence_model_set_property;
   obj_class->get_property = dee_sequence_model_get_property;
 
+  /* Find signal ids for the model modification signals */
+  sigid_row_added = g_signal_lookup ("row-added", DEE_TYPE_SEQUENCE_MODEL);
+  sigid_row_removed = g_signal_lookup ("row-removed", DEE_TYPE_SEQUENCE_MODEL);
+  sigid_row_changed = g_signal_lookup ("row-changed", DEE_TYPE_SEQUENCE_MODEL);
+
   /* Add private data */
   g_type_class_add_private (obj_class, sizeof (DeeSequenceModelPrivate));
 }
@@ -304,7 +314,7 @@
   priv->setting_many = FALSE;
 
   dee_serializable_model_inc_seqnum (self);
-  g_signal_emit_by_name (self, "row-added", iter);
+  g_signal_emit (self, sigid_row_added, 0, iter);
   
   return iter;
 }
@@ -330,7 +340,7 @@
   priv->setting_many = FALSE;
 
   dee_serializable_model_inc_seqnum (self);
-  g_signal_emit_by_name (self, "row-added", iter);
+  g_signal_emit (self, sigid_row_added, 0, iter);
   
   return iter;
 }
@@ -357,7 +367,7 @@
   priv->setting_many = FALSE;
 
   dee_serializable_model_inc_seqnum (self);
-  g_signal_emit_by_name (self, "row-added", iter);
+  g_signal_emit (self, sigid_row_added, 0, iter);
   
   return iter;
 }
@@ -380,7 +390,7 @@
       /* Emit the removed signal while the iter is still valid,
        * but after we increased the seqnum */
       dee_serializable_model_inc_seqnum (self);
-      g_signal_emit_by_name (self, "row-removed", iter_);
+      g_signal_emit (self, sigid_row_removed, 0, iter_);
       dee_sequence_model_free_row (_self, iter);
       g_sequence_remove (iter);
     }
@@ -410,7 +420,7 @@
   if (priv->setting_many == FALSE)
     {
       dee_serializable_model_inc_seqnum (self);
-      g_signal_emit_by_name (self, "row-changed", iter);
+      g_signal_emit (self, sigid_row_changed, 0, iter);
     }
 }
 
@@ -438,7 +448,7 @@
   if (priv->setting_many == FALSE)
     {
       dee_serializable_model_inc_seqnum (self);
-      g_signal_emit_by_name (self, "row-changed", iter);
+      g_signal_emit (self, sigid_row_changed, 0, iter);
     }
 }
 

=== modified file 'tests/test-benchmark.c'
--- tests/test-benchmark.c	2011-12-07 15:45:47 +0000
+++ tests/test-benchmark.c	2011-12-09 14:18:23 +0000
@@ -128,7 +128,7 @@
 bench_seqmodel_append_run (Benchmark *bench)
 {
   DeeModel *model;
-  guint     limit = 1000, i;
+  guint     limit = 2500, i;
   
   g_assert (DEE_IS_MODEL (bench->state));
   
@@ -144,7 +144,7 @@
 bench_seqmodel_prepend_run (Benchmark *bench)
 {
   DeeModel *model;
-  guint     limit = 1000, i;
+  guint     limit = 2500, i;
   
   g_assert (DEE_IS_MODEL (bench->state));
   
@@ -175,7 +175,7 @@
                                 bench_seqmodel_setup,
                                 bench_seqmodel_append_run,
                                 bench_gobject_teardown,
-                                10,
+                                100,
                                 NULL };
   add_benchmark (&seqmodel_append);
   
@@ -183,7 +183,7 @@
                                 bench_seqmodel_setup,
                                 bench_seqmodel_prepend_run,
                                 bench_gobject_teardown,
-                                10,
+                                100,
                                 NULL };
   add_benchmark (&seqmodel_prepend);