|
Description
Simon McVittie
2011-02-08 08:00:45 UTC
Created attachment 43106 [details] [review] Related to Bug #24307. Created attachment 43107 [details] [review] configure.in: add --enable-stats Created attachment 43108 [details] [review] [3] Add a stub .Debug.Stats interface if --enable-stats There are no actual statistics yet, just a count of how many times the method has been called, and (for the per-connection stats) the unique name. Created attachment 43109 [details] [review] [4] DBusMemPool: add usage stats Created attachment 43110 [details] [review] [5] DBusList: add usage stats Created attachment 43111 [details] [review] [6] BusConnections: add usage stats for well-known names, match rules Created attachment 43112 [details] [review] [7] DBusConnection, DBusTransport: add queue statistics Created attachment 43113 [details] [review] [8] Add an initial round of stats to the Stats interface Created attachment 43114 [details] [review] [9] Also record peak values for queued bytes/fds in connection stats Created attachment 43115 [details] [review] [10] Include size of link cache in per-connection statistics Created attachment 43259 [details] [review] [9 v2] Also record peak values for queued bytes/fds in connection stats Now actually initialized to 0... Created attachment 43674 [details] [review] Include global peaks for message sizes, recipients counts and per interfaces Limits: - some leaks if malloc fails in bus_stats_handle_get_stats() - is there a way to find the message size without using dbus_message_marshal()? - we could add cumulative sizes per interface I used this Python script to generate the following spreadsheet: http://people.collabora.co.uk/~alban/d/2011/02/messages_stats.py http://people.collabora.co.uk/~alban/d/2011/02/gnome-telepathy.csv Review of attachment 43106 [details] [review]: Seems ok, not wearing any dev hat. I'd say that in case "%d" turns out to be an important bit, DBUS_NUM_MESSAGE_TYPES can be used to identify unknown types. Review of attachment 43108 [details] [review]: the patch seems OK, without any dev hat, again. ::: bus/Makefile.am @@ +74,3 @@ signals.h \ + stats.c \ + stats.h \ Wouldn't it be better to add only when enabled? In driver.c it's published only if enabled anyway. ::: bus/driver.c @@ +31,3 @@ #include "selinux.h" #include "signals.h" +#include "stats.h" Same, include only when enabled? Review of attachment 43108 [details] [review]: the patch seems OK, without any dev hat, again. ::: bus/Makefile.am @@ +74,3 @@ signals.h \ + stats.c \ + stats.h \ Wouldn't it be better to add only when enabled? In driver.c it's published only if enabled anyway. ::: bus/driver.c @@ +31,3 @@ #include "selinux.h" #include "signals.h" +#include "stats.h" Same, include only when enabled? (In reply to comment #13) > I'd say that in case "%d" turns out to be an important bit, > DBUS_NUM_MESSAGE_TYPES can be used to identify unknown types. What I meant by the commit message is: it's not a regression that we no longer stringify unknown message types via "%d" after this patch, because we don't allow adding match rules with such message types anyway, so that code is never (meant to be) reached. (In reply to comment #14) > ::: bus/Makefile.am > @@ +74,3 @@ > signals.h \ > + stats.c \ > + stats.h \ > > Wouldn't it be better to add only when enabled? stats.c is entirely #ifdef DBUS_ENABLE_STATS, so it'll compile to nothing when disabled; I think that's clearer than putting conditionals in the Makefile.am, but if a reviewer disagrees, this would also be fine: if DBUS_ENABLE_STATS BUS_SOURCES += stats.c stats.h endif (automake is clever enough to distribute stats.[ch] whenever they're needed by any conditional.) > ::: bus/driver.c > @@ +31,3 @@ > #include "selinux.h" > #include "signals.h" > +#include "stats.h" > > Same, include only when enabled? It's one more #ifdef block for no real benefit (apart from an insignificant reduction in time to compile this file), but if that style would be preferred, it's an easy change. (In reply to comment #16) > if a reviewer disagrees, this would also be fine: > > if DBUS_ENABLE_STATS This would also require adding to configure.ac: AM_CONDITIONAL([DBUS_ENABLE_STATS], [test "x$enable_stats" = xyes]) Again, easy to do if preferred. The rest of the patches looks good (yet again, no hat). Review of attachment 43674 [details] [review]: I'd rather not apply this one right now. ::: bus/connection.c @@ +31,3 @@ #include "expirelist.h" #include "selinux.h" +#include <stdlib.h> Normally avoided outside sysdeps.[ch] @@ +85,3 @@ + * MessagesCount:u} + */ + DBusHashTable *many_recipient_messages; This would be much simpler as four hash tables, each { string => uint } - then you could use the D-Bus equivalent of GUINT_TO_POINTER for the values. Not correctly documented, the outer keys are now actually "signal com.example.Badgerable.Badgered" or whatever. @@ +501,3 @@ + + if (connections->many_recipient_messages == NULL) + goto failed_6; failed_6? Seriously? This constructor needs refactoring :-) I find that a better pattern is: failed: if (a != NULL) thing_free (a); if (b != NULL) other_thing_unref (b); ... @@ +2494,3 @@ + ret = dbus_message_marshal (message, &buffer, &size); + if (ret) + free (buffer); Doesn't dbus_message_marshal return a dbus_malloc'd buffer? Also, adding _dbus_message_get_size() would be better, IMO - even if it's #ifdef DBUS_ENABLE_STATS. @@ +2517,3 @@ + { + if (!_dbus_string_append_printf (&iface_member, "%s %s.%s", + dbus_message_type_to_string (type), interface, member)) interface could be NULL. member could be NULL if it's a reply. Some platforms' printf implementations crash on NULL strings (coping gracefully with NULL is a nonstandard GNU extension). @@ +2537,3 @@ + goto free_string; + inner = _dbus_hash_table_new (DBUS_HASH_STRING, NULL, + (DBusFreeFunction)dbus_free); Is dbus_free not a DBusFreeFunction? :'-( ::: bus/stats.c @@ +180,3 @@ +static dbus_bool_t +asv_add_asasvu (DBusMessageIter *iter, I haven't reviewed this in detail at all, because using a{su} would make most of this complexity unnecessary. Patches 1-10 merged, based on review (+ acknowledgement IRL) from Cosimo. Retitling this bug for Alban's additional patch. Gaming bugzilla isn't nice. -- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/dbus/dbus/issues/39. |
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.