2004-11-13 Havoc Pennington <hp@redhat.com>

	* test/glib/test-profile.c: fix this thing up a bit

	* dbus/dbus-message.c (dbus_message_new_empty_header): increase
	preallocation sizes by a fair bit; not sure if this will be an
	overall performance win or not, but it does reduce reallocs.

	* dbus/dbus-string.c (set_length, reallocate_for_length): ignore
	the test hack that forced constant realloc if asserts are
	disabled, so we can profile sanely. Sprinkle in some
	_DBUS_UNLIKELY() which are probably pointless, but before I
	noticed the real performance problem I put them in.
	(_dbus_string_validate_utf8): micro-optimize this thing a little
	bit, though callgrind says it didn't help; then special-case
	ascii, which did help a lot; then be sure we detect nul bytes as
	invalid, which is a bugfix.
	(align_length_then_lengthen): add some more _DBUS_UNLIKELY
	superstition; use memset to nul the padding instead of a manual
	loop.
	(_dbus_string_get_length): inline this as a
	macro; it showed up in the profile because it's used for loop
	tests and so forth

6 files changed, 189 insertions, 70 deletions

diff --git a/ChangeLog b/ChangeLog
index 9c7cadbb..b70e8c8c 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,27 @@
+2004-11-13  Havoc Pennington  <hp@redhat.com>
+
+	* test/glib/test-profile.c: fix this thing up a bit
+
+	* dbus/dbus-message.c (dbus_message_new_empty_header): increase
+	preallocation sizes by a fair bit; not sure if this will be an
+	overall performance win or not, but it does reduce reallocs.
+
+	* dbus/dbus-string.c (set_length, reallocate_for_length): ignore
+	the test hack that forced constant realloc if asserts are
+	disabled, so we can profile sanely. Sprinkle in some
+	_DBUS_UNLIKELY() which are probably pointless, but before I
+	noticed the real performance problem I put them in.
+	(_dbus_string_validate_utf8): micro-optimize this thing a little
+	bit, though callgrind says it didn't help; then special-case
+	ascii, which did help a lot; then be sure we detect nul bytes as
+	invalid, which is a bugfix.
+	(align_length_then_lengthen): add some more _DBUS_UNLIKELY
+	superstition; use memset to nul the padding instead of a manual
+	loop.
+	(_dbus_string_get_length): inline this as a
+	macro; it showed up in the profile because it's used for loop
+	tests and so forth
+
 2004-11-10  Colin Walters  <walters@verbum.org>
 
 	* dbus/dbus-spawn.c (check_babysit_events): Handle EINTR,
diff --git a/dbus/dbus-message.c b/dbus/dbus-message.c
index df0ade26..0b3b1bf2 100644
--- a/dbus/dbus-message.c
+++ b/dbus/dbus-message.c
@@ -493,11 +493,13 @@ get_type_alignment (int type)
 
     case DBUS_TYPE_ARRAY:
       _dbus_assert_not_reached ("passed an ARRAY type to get_type_alignment()");
+      alignment = 0; /* quiet gcc */
       break;
 
     case DBUS_TYPE_INVALID:
     default:
       _dbus_assert_not_reached ("passed an invalid or unknown type to get_type_alignment()");
+      alignment = 0; /* quiet gcc */
       break;
     }
 
@@ -1355,7 +1357,7 @@ dbus_message_new_empty_header (void)
       ++i;
     }
   
-  if (!_dbus_string_init_preallocated (&message->header, 64))
+  if (!_dbus_string_init_preallocated (&message->header, 256))
     {
       dbus_free (message);
       return NULL;
diff --git a/dbus/dbus-string.c b/dbus/dbus-string.c
index 7381dab2..75d22103 100644
--- a/dbus/dbus-string.c
+++ b/dbus/dbus-string.c
@@ -380,20 +380,28 @@ reallocate_for_length (DBusRealString *real,
     new_allocated = real->allocated * 2;
 
   /* if you change the code just above here, run the tests without
-   * the following before you commit
+   * the following assert-only hack before you commit
    */
+  /* This is keyed off asserts in addition to tests so when you
+   * disable asserts to profile, you don't get this destroyer
+   * of profiles.
+   */
+#ifdef DBUS_DISABLE_ASSERT
+#else
 #ifdef DBUS_BUILD_TESTS
   new_allocated = 0; /* ensure a realloc every time so that we go
                       * through all malloc failure codepaths
                       */
-#endif
-      
+#endif /* DBUS_BUILD_TESTS */
+#endif /* !DBUS_DISABLE_ASSERT */
+
   /* But be sure we always alloc at least space for the new length */
-  new_allocated = MAX (new_allocated, new_length + ALLOCATION_PADDING);
+  new_allocated = MAX (new_allocated,
+                       new_length + ALLOCATION_PADDING);
 
   _dbus_assert (new_allocated >= real->allocated); /* code relies on this */
   new_str = dbus_realloc (real->str - real->align_offset, new_allocated);
-  if (new_str == NULL)
+  if (_DBUS_UNLIKELY (new_str == NULL))
     return FALSE;
 
   real->str = new_str + real->align_offset;
@@ -410,15 +418,15 @@ set_length (DBusRealString *real,
   /* Note, we are setting the length not including nul termination */
 
   /* exceeding max length is the same as failure to allocate memory */
-  if (new_length > real->max_length)
+  if (_DBUS_UNLIKELY (new_length > real->max_length))
     return FALSE;
   else if (new_length > (real->allocated - ALLOCATION_PADDING) &&
-           !reallocate_for_length (real, new_length))
+           _DBUS_UNLIKELY (!reallocate_for_length (real, new_length)))
     return FALSE;
   else
     {
       real->len = new_length;
-      real->str[real->len] = '\0';
+      real->str[new_length] = '\0';
       return TRUE;
     }
 }
@@ -780,6 +788,8 @@ _dbus_string_copy_to_buffer (const DBusString  *str,
     buffer[avail_len-1] = '\0';
 }
 
+/* Only have the function if we don't have the macro */
+#ifndef _dbus_string_get_length
 /**
  * Gets the length of a string (not including nul termination).
  *
@@ -792,6 +802,7 @@ _dbus_string_get_length (const DBusString  *str)
   
   return real->len;
 }
+#endif /* !_dbus_string_get_length */
 
 /**
  * Makes a string longer by the given number of bytes.  Checks whether
@@ -812,7 +823,7 @@ _dbus_string_lengthen (DBusString *str,
   DBUS_STRING_PREAMBLE (str);  
   _dbus_assert (additional_length >= 0);
 
-  if (additional_length > real->max_length - real->len)
+  if (_DBUS_UNLIKELY (additional_length > real->max_length - real->len))
     return FALSE; /* would overflow */
   
   return set_length (real,
@@ -869,7 +880,7 @@ align_length_then_lengthen (DBusString *str,
   _dbus_assert (alignment <= 8); /* it has to be a bug if > 8 */
 
   new_len = _DBUS_ALIGN_VALUE (real->len, alignment);
-  if (new_len > (unsigned long) real->max_length - then_lengthen_by)
+  if (_DBUS_UNLIKELY (new_len > (unsigned long) real->max_length - then_lengthen_by))
     return FALSE;
   new_len += then_lengthen_by;
   
@@ -879,21 +890,17 @@ align_length_then_lengthen (DBusString *str,
   if (delta == 0)
     return TRUE;
 
-  if (!set_length (real, new_len))
+  if (_DBUS_UNLIKELY (!set_length (real, new_len)))
     return FALSE;
 
   /* delta == padding + then_lengthen_by
    * new_len == old_len + padding + then_lengthen_by
+   * nul the padding if we had to add any padding
    */
   if (then_lengthen_by < delta)
     {
-      unsigned int i;
-      i = new_len - delta;
-      while (i < (new_len - then_lengthen_by))
-        {
-          real->str[i] = '\0';
-          ++i;
-        }
+      memset (&real->str[new_len - delta], '\0',
+              delta - then_lengthen_by);
     }
       
   return TRUE;
@@ -1509,8 +1516,11 @@ _dbus_string_replace_len (const DBusString *source,
       Len = 6;								      \
       Mask = 0x01;							      \
     }									      \
-  else									      \
-    Len = -1;
+  else                                                                        \
+    {                                                                         \
+      Len = 0;                                                               \
+      Mask = 0;                                                               \
+    }
 
 /**
  * computes length of a unicode character in UTF-8
@@ -1590,7 +1600,7 @@ _dbus_string_get_unichar (const DBusString *str,
   c = *p;
   
   UTF8_COMPUTE (c, mask, len);
-  if (len == -1)
+  if (len == 0)
     return;
   UTF8_GET (result, p, i, mask, len);
 
@@ -2431,29 +2441,51 @@ _dbus_string_validate_utf8  (const DBusString *str,
   
   while (p < end)
     {
-      int i, mask = 0, char_len;
+      int i, mask, char_len;
       dbus_unichar_t result;
-      unsigned char c = (unsigned char) *p;
+
+      const unsigned char c = (unsigned char) *p;
+
+      if (c == 0) /* nul bytes not OK */
+        break;
+      
+      /* Special-case ASCII; this makes us go a lot faster in
+       * D-BUS profiles where we are typically validating
+       * function names and such. We have to know that
+       * all following checks will pass for ASCII though,
+       * comments follow ... 
+       */
+      if (c < 128)
+        {
+          ++p;
+          continue;
+        }
       
       UTF8_COMPUTE (c, mask, char_len);
 
-      if (_DBUS_UNLIKELY (char_len == -1))
+      if (_DBUS_UNLIKELY (char_len == 0))  /* ASCII: char_len == 1 */
         break;
 
       /* check that the expected number of bytes exists in the remaining length */
-      if (_DBUS_UNLIKELY ((end - p) < char_len))
+      if (_DBUS_UNLIKELY ((end - p) < char_len)) /* ASCII: p < end and char_len == 1 */
         break;
         
       UTF8_GET (result, p, i, mask, char_len);
 
-      if (_DBUS_UNLIKELY (UTF8_LENGTH (result) != char_len)) /* Check for overlong UTF-8 */
+      /* Check for overlong UTF-8 */
+      if (_DBUS_UNLIKELY (UTF8_LENGTH (result) != char_len)) /* ASCII: UTF8_LENGTH == 1 */
         break;
-
-      if (_DBUS_UNLIKELY (result == (dbus_unichar_t)-1))
+#if 0
+      /* The UNICODE_VALID check below will catch this */
+      if (_DBUS_UNLIKELY (result == (dbus_unichar_t)-1)) /* ASCII: result = ascii value */
         break;
+#endif
 
-      if (_DBUS_UNLIKELY (!UNICODE_VALID (result)))
+      if (_DBUS_UNLIKELY (!UNICODE_VALID (result))) /* ASCII: always valid */
         break;
+
+      /* UNICODE_VALID should have caught it */
+      _dbus_assert (result != (dbus_unichar_t)-1);
       
       p += char_len;
     }
diff --git a/dbus/dbus-string.h b/dbus/dbus-string.h
index 1a0236d4..cf526e43 100644
--- a/dbus/dbus-string.h
+++ b/dbus/dbus-string.h
@@ -49,6 +49,13 @@ struct DBusString
   unsigned int dummy8 : 3; /**< placeholder */
 };
 
+/* Unless we want to run all the assertions in the function,
+ * inline this thing, it shows up high in the profile
+ */
+#ifdef DBUS_DISABLE_ASSERT
+#define _dbus_string_get_length(s) ((s)->dummy2)
+#endif
+
 dbus_bool_t   _dbus_string_init                  (DBusString        *str);
 void          _dbus_string_init_const            (DBusString        *str,
                                                   const char        *value);
@@ -91,7 +98,10 @@ dbus_bool_t   _dbus_string_copy_data_len         (const DBusString  *str,
 void          _dbus_string_copy_to_buffer        (const DBusString  *str,
                                                   char              *buffer,
 						  int                len);
+#ifndef _dbus_string_get_length
 int           _dbus_string_get_length            (const DBusString  *str);
+#endif /* !_dbus_string_get_length */
+
 dbus_bool_t   _dbus_string_lengthen              (DBusString        *str,
                                                   int                additional_length);
 void          _dbus_string_shorten               (DBusString        *str,
diff --git a/glib/dbus-gmain.c b/glib/dbus-gmain.c
index 30b1c4e0..a6c7d25c 100644
--- a/glib/dbus-gmain.c
+++ b/glib/dbus-gmain.c
@@ -327,7 +327,7 @@ remove_watch (DBusWatch *watch,
     return; /* probably a not-enabled watch that was added */
 
   watch_fd->removed = TRUE;
-  watch_fd->watch = NULL;  
+  watch_fd->watch = NULL;
   
   dbus_source->watch_fds = g_list_remove (dbus_source->watch_fds, watch_fd);
 
@@ -409,7 +409,7 @@ timeout_toggled (DBusTimeout *timeout,
 
 static void
 free_source (GSource *source)
-{
+{  
   g_source_destroy (source);
 }
 
diff --git a/test/glib/test-profile.c b/test/glib/test-profile.c
index 6a00ee5e..762e2fd9 100644
--- a/test/glib/test-profile.c
+++ b/test/glib/test-profile.c
@@ -21,18 +21,13 @@
  *
  */
 
-/* FIXME this test is wacky since both client and server keep
- * sending each other method calls, but nobody sends
- * a DBUS_MESSAGE_TYPE_METHOD_RETURN
- */
-
 #include <config.h>
 #include <glib.h>
 #include <dbus/dbus-glib-lowlevel.h>
 #include <stdlib.h>
 
 #define N_CLIENT_THREADS 1
-#define N_ITERATIONS 1000
+#define N_ITERATIONS 4000
 #define PAYLOAD_SIZE 30
 #define ECHO_PATH "/org/freedesktop/EchoTest"
 #define ECHO_INTERFACE "org.freedesktop.EchoTest"
@@ -41,8 +36,21 @@
 static const char *address;
 static unsigned char *payload;
 
+typedef struct
+{
+  int iterations;
+  GMainLoop *loop;
+} ClientData;
+
+typedef struct
+{
+  int handled;
+  GMainLoop *loop;
+  int n_clients;
+} ServerData;
+
 static void
-send_echo_message (DBusConnection *connection)
+send_echo_method_call (DBusConnection *connection)
 {
   DBusMessage *message;
 
@@ -51,7 +59,7 @@ send_echo_message (DBusConnection *connection)
   dbus_message_append_args (message,
                             DBUS_TYPE_STRING, "Hello World!",
                             DBUS_TYPE_INT32, 123456,
-#if 1
+#if 0
                             DBUS_TYPE_ARRAY, DBUS_TYPE_BYTE,
                             payload, PAYLOAD_SIZE,
 #endif
@@ -62,12 +70,25 @@ send_echo_message (DBusConnection *connection)
   dbus_connection_flush (connection);
 }
 
+static void
+send_echo_method_return (DBusConnection *connection,
+                         DBusMessage    *call_message)
+{
+  DBusMessage *message;
+
+  message = dbus_message_new_method_return (call_message);
+  
+  dbus_connection_send (connection, message, NULL);
+  dbus_message_unref (message);
+  dbus_connection_flush (connection);
+}
+
 static DBusHandlerResult
 client_filter (DBusConnection     *connection,
 	       DBusMessage        *message,
 	       void               *user_data)
 {
-  int *iterations = user_data;
+  ClientData *cd = user_data;
   
   if (dbus_message_is_signal (message,
                               DBUS_INTERFACE_ORG_FREEDESKTOP_LOCAL,
@@ -76,16 +97,15 @@ client_filter (DBusConnection     *connection,
       g_printerr ("Client thread disconnected\n");
       exit (1);
     }
-  else if (dbus_message_is_method_call (message,
-                                        ECHO_INTERFACE, ECHO_METHOD))
+  else if (dbus_message_get_type (message) == DBUS_MESSAGE_TYPE_METHOD_RETURN)
     {
-      *iterations += 1;
-      if (*iterations >= N_ITERATIONS)
+      cd->iterations += 1;
+      if (cd->iterations >= N_ITERATIONS)
         {
           g_print ("Completed %d iterations\n", N_ITERATIONS);
-          exit (0);
+          g_main_loop_quit (cd->loop);
         }
-      send_echo_message (connection);
+      send_echo_method_call (connection);
       return DBUS_HANDLER_RESULT_HANDLED;
     }
   
@@ -97,9 +117,8 @@ thread_func (void *data)
 {
   DBusError error;
   GMainContext *context;
-  GMainLoop *loop;
   DBusConnection *connection;
-  int iterations;
+  ClientData cd;
   
   g_printerr ("Starting client thread\n");
   
@@ -112,28 +131,31 @@ thread_func (void *data)
       exit (1);
     }
 
-  iterations = 1;
+  context = g_main_context_new ();
+
+  cd.iterations = 1;
+  cd.loop = g_main_loop_new (context, FALSE);
   
   if (!dbus_connection_add_filter (connection,
-				   client_filter, &iterations, NULL))
+				   client_filter, &cd, NULL))
     g_error ("no memory");
   
-  context = g_main_context_new ();
-  loop = g_main_loop_new (context, FALSE);
   
   dbus_connection_setup_with_g_main (connection, context);
 
   g_printerr ("Client thread sending message to prime pingpong\n");
-  send_echo_message (connection);
+  send_echo_method_call (connection);
   g_printerr ("Client thread sent message\n");
 
   g_printerr ("Client thread entering main loop\n");
-  g_main_loop_run (loop);
+  g_main_loop_run (cd.loop);
   g_printerr ("Client thread exiting main loop\n");
+
+  dbus_connection_disconnect (connection);
   
-  g_main_loop_unref (loop);
+  g_main_loop_unref (cd.loop);
   g_main_context_unref (context);
-
+  
   return NULL;
 }
 
@@ -142,18 +164,23 @@ server_filter (DBusConnection     *connection,
 	       DBusMessage        *message,
 	       void               *user_data)
 {
+  ServerData *sd = user_data;
+  
   if (dbus_message_is_signal (message,
                               DBUS_INTERFACE_ORG_FREEDESKTOP_LOCAL,
                               "Disconnected"))
     {
-      g_printerr ("Server thread disconnected\n");
-      exit (1);
+      g_printerr ("Client disconnected from server\n");
+      sd->n_clients -= 1;
+      if (sd->n_clients == 0)
+        g_main_loop_quit (sd->loop);
     }
   else if (dbus_message_is_method_call (message,
                                         ECHO_INTERFACE,
                                         ECHO_METHOD))
     {
-      send_echo_message (connection);
+      sd->handled += 1;
+      send_echo_method_return (connection, message);
       return DBUS_HANDLER_RESULT_HANDLED;
     }
   
@@ -164,14 +191,17 @@ static void
 new_connection_callback (DBusServer     *server,
                          DBusConnection *new_connection,
                          void           *user_data)
-{  
+{
+  ServerData *sd = user_data;
+  
   dbus_connection_ref (new_connection);
   dbus_connection_setup_with_g_main (new_connection, NULL);  
   
   if (!dbus_connection_add_filter (new_connection,
-                                   server_filter, NULL, NULL))
+                                   server_filter, sd, NULL))
     g_error ("no memory");
-  
+
+  sd->n_clients += 1;
 
   /* FIXME we leak the handler */  
 }
@@ -179,11 +209,13 @@ new_connection_callback (DBusServer     *server,
 int
 main (int argc, char *argv[])
 {
-  GMainLoop *loop;
   DBusError error;
   DBusServer *server;
+  GTimer *timer;
   int i;
-  
+  double secs;
+  ServerData sd;
+
   g_thread_init (NULL);
   dbus_g_thread_init ();
 
@@ -197,14 +229,20 @@ main (int argc, char *argv[])
       return 1;
     }
 
+#ifndef DBUS_DISABLE_ASSERT
+  g_printerr ("You should probably turn off assertions before you profile\n");
+#endif
+  
   address = dbus_server_get_address (server);
   payload = g_malloc (PAYLOAD_SIZE);
   
   dbus_server_set_new_connection_function (server,
                                            new_connection_callback,
-                                           NULL, NULL);
-  
-  loop = g_main_loop_new (NULL, FALSE);
+                                           &sd, NULL);
+
+  sd.handled = 0;
+  sd.n_clients = 0;
+  sd.loop = g_main_loop_new (NULL, FALSE);
 
   dbus_server_setup_with_g_main (server, NULL);
   
@@ -213,14 +251,27 @@ main (int argc, char *argv[])
       g_thread_create (thread_func, NULL, FALSE, NULL);
     }
 
+  timer = g_timer_new ();
+  
   g_printerr ("Server thread entering main loop\n");
-  g_main_loop_run (loop);
+  g_main_loop_run (sd.loop);
   g_printerr ("Server thread exiting main loop\n");
 
+  secs = g_timer_elapsed (timer, NULL);
+  g_timer_destroy (timer);
+
+  g_printerr ("%g seconds, %d round trips, %g seconds per pingpong\n",
+              secs, sd.handled, secs/sd.handled);
+#ifndef DBUS_DISABLE_ASSERT
+  g_printerr ("You should probably --disable-asserts before you profile as they have noticeable overhead\n");
+#endif
+
+  g_printerr ("The following g_warning is because we try to call g_source_remove_poll() after g_source_destroy() in dbus-gmain.c, I think we need to add a source free func that clears out the watch/timeout funcs\n");
   dbus_server_unref (server);
   
-  g_main_loop_unref (loop);
+  g_main_loop_unref (sd.loop);
   
   return 0;
 }
+