Re: Fast handling of class-closure-only signals

[Soeren Sandmann <sandmann daimi au dk>]

Tim Janik <timj gtk org> writes:

> so i suspect you're trying to optimize in an area where no one will ever
> notice the benefits of your optimizations (and that is at a significant
> cost in signal emission complexity).
> what i'd like to see to support your claims is profiling data showing that
> significant _time_ can be saved during signal emissions, and that
> this time is actually user visible.

I instrumented (patch attached) the part of gtk+ that does the bulk of
the work during opaque resize, gtk_container_idle_sizer(), to track
the average amount of time it took to execute that function.

Then I used the gnome-test-tool to record a lot of opaque resizing of
gnumeric. I replayed the resizing seven times and took the
average. 

Then I applied the patches, the glib one modified to check for
non-NULL class closure as the first thing in the NOP case, as you
suggested, and ran the script seven times again and took the average.

Before:

        0.0251s spent inside gtk_container_idle_sizer()

After

        0.0227s spent inside gtk_container_idle_sizer()

a speedup of 9.6%.

Profiling opaque resizing of Gnumeric usually shows
gtk_container_idle_sizer() to use about 80% of the total time, so the
effective speed-up would be 7.7%.

There are several error sources here, and in fact I think the real
number is probably higher, because the signal emission overhead I see
on profiles is often closer to 20% than to 10%.

> a few comments on the patches:
> - signal_check_skip_emission() does cheap and frequently failing
>   checks first, to reduce the cost of doing the check at all.
>   in your patched signal_check_skip_emission(), you're delaying
>   the check for a non-NULL class pointer until after all the expensive
>   checks have been performed, this introduces a performance penalty
>   in the very common case that there's a non-NULL class function pointer.
>   (if you do further profiling, please fix this first)

I moved the check for non-NULL class pointer to the front of the
function before measuring.

> - you're bypassing the return_type==G_TYPE_NONE check for class closures.
>   for the NOP emission optimization, the check is there because
>   normal emissions can't be skipped if return types are present since
>   that'd skip the return value initialization step which results in
>   bogus return values being returned.
>   also, if accumulators are present, we have to emit the signal
>   regardless, since the signal code may not guess the accumulator code
>   to be unimportant enough to simply be skipped.
>   while the first issue is probably being taken care of by the user
>   calling the class-function manually, i can only hope he's also fully
>   aware of any side effects the signal accumulator may have.

In this case the signal accumulator is just the standard boolean
accumulator which should be safe to leave out.


Søren

Index: gtk/gtkcontainer.c
===================================================================
RCS file: /cvs/gnome/gtk+/gtk/gtkcontainer.c,v
retrieving revision 1.124
diff -u -r1.124 gtkcontainer.c
--- gtk/gtkcontainer.c	8 Jul 2003 22:49:35 -0000	1.124
+++ gtk/gtkcontainer.c	5 Sep 2003 18:35:32 -0000
@@ -1087,11 +1087,34 @@
   return GTK_IS_RESIZE_CONTAINER (widget) ? (GtkContainer*) widget : NULL;
 }
 
+static gdouble avg = -1.0;
+static int n_samples;
+
+static void
+print_data (void)
+{
+    g_print ("average time per gtk_container_idle_sizer(): %f\n", avg);
+}
+
 static gboolean
 gtk_container_idle_sizer (gpointer data)
 {
+  static GTimer *timer;
+  gdouble elapsed;
+  gboolean do_time;
+
+  if (!timer)
+  {
+      timer = g_timer_new ();
+      g_atexit (print_data); 
+  }
+  
+  g_timer_reset (timer);
+
   GDK_THREADS_ENTER ();
 
+  do_time = container_resize_queue != NULL;
+  
   /* we may be invoked with a container_resize_queue of NULL, because
    * queue_resize could have been adding an extra idle function while
    * the queue still got processed. we better just ignore such case
@@ -1114,6 +1137,22 @@
 
   gdk_window_process_all_updates ();
 
+  if (do_time)
+    {
+      elapsed = g_timer_elapsed (timer, NULL);
+
+      if (avg < 0.0)
+      {
+	  avg = elapsed;
+	  n_samples = 1;
+      }
+      else
+      {
+	  n_samples++;
+	  avg = ((avg * (n_samples - 1)) + elapsed) / n_samples;
+      }
+    }
+  
   GDK_THREADS_LEAVE ();
 
   return FALSE;