diff options
| -rw-r--r-- | ChangeLog | 24 | ||||
| -rw-r--r-- | dbus/dbus-message.c | 4 | ||||
| -rw-r--r-- | dbus/dbus-string.c | 90 | ||||
| -rw-r--r-- | dbus/dbus-string.h | 10 | ||||
| -rw-r--r-- | glib/dbus-gmain.c | 4 | ||||
| -rw-r--r-- | test/glib/test-profile.c | 127 |
6 files changed, 189 insertions, 70 deletions
@@ -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; } + |