Bug 34040

Summary: add peak message sizes to Stats interface
Product: dbus Reporter: Simon McVittie <smcv>
Component: coreAssignee: D-Bus Maintainers <dbus>
Status: REOPENED --- QA Contact: D-Bus Maintainers <dbus>
Severity: enhancement    
Priority: medium CC: alban.crequy, hp, me, msniko14, robin.bateboerop, ross, smcv
Version: 1.5   
Hardware: All   
OS: All   
Whiteboard: review-
i915 platform: i915 features:
Bug Depends on: 33757    
Bug Blocks: 24307    
Attachments: Related to Bug #24307.
configure.in: add --enable-stats
[3] Add a stub .Debug.Stats interface if --enable-stats
[4] DBusMemPool: add usage stats
[5] DBusList: add usage stats
[6] BusConnections: add usage stats for well-known names, match rules
[7] DBusConnection, DBusTransport: add queue statistics
[8] Add an initial round of stats to the Stats interface
[9] Also record peak values for queued bytes/fds in connection stats
[10] Include size of link cache in per-connection statistics
[9 v2] Also record peak values for queued bytes/fds in connection stats
Include global peaks for message sizes, recipients counts and per interfaces

Description Simon McVittie 2011-02-08 08:00:45 UTC
+++ This bug was initially created as a clone of Bug #33757 +++

This branch adds a Stats interface. It doesn't yet solve Bug #24307 (it doesn't include the actual match rules), but it does include various potentially-interesting numbers.

In the longer term Colin wants such things to be on a secondary /var/run/dbus/system_bus_management_socket or some such, but splitting off interfaces now seems like a good start on that.
Comment 1 Simon McVittie 2011-02-08 08:03:53 UTC
Created attachment 43106 [details] [review]
Related to Bug #24307.
Comment 2 Simon McVittie 2011-02-08 08:04:20 UTC
Created attachment 43107 [details] [review]
configure.in: add --enable-stats
Comment 3 Simon McVittie 2011-02-08 08:04:58 UTC
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.
Comment 4 Simon McVittie 2011-02-08 08:05:35 UTC
Created attachment 43109 [details] [review]
[4] DBusMemPool: add usage stats
Comment 5 Simon McVittie 2011-02-08 08:06:16 UTC
Created attachment 43110 [details] [review]
[5] DBusList: add usage stats
Comment 6 Simon McVittie 2011-02-08 08:06:32 UTC
Created attachment 43111 [details] [review]
[6] BusConnections: add usage stats for well-known names, match rules
Comment 7 Simon McVittie 2011-02-08 08:06:49 UTC
Created attachment 43112 [details] [review]
[7] DBusConnection, DBusTransport: add queue statistics
Comment 8 Simon McVittie 2011-02-08 08:07:04 UTC
Created attachment 43113 [details] [review]
[8] Add an initial round of stats to the Stats interface
Comment 9 Simon McVittie 2011-02-08 08:07:23 UTC
Created attachment 43114 [details] [review]
[9] Also record peak values for queued bytes/fds in connection stats
Comment 10 Simon McVittie 2011-02-08 08:07:49 UTC
Created attachment 43115 [details] [review]
[10] Include size of link cache in per-connection statistics
Comment 11 Simon McVittie 2011-02-11 09:20:11 UTC
Created attachment 43259 [details] [review]
[9 v2] Also record peak values for queued bytes/fds in connection stats

Now actually initialized to 0...
Comment 12 Alban Crequy 2011-02-22 10:57:36 UTC
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
Comment 13 Cosimo Alfarano 2011-03-02 07:54:50 UTC
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.
Comment 14 Cosimo Alfarano 2011-03-02 09:52:20 UTC
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?
Comment 15 Cosimo Alfarano 2011-03-02 09:52:22 UTC
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?
Comment 16 Simon McVittie 2011-03-03 06:53:03 UTC
(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.
Comment 17 Simon McVittie 2011-03-03 06:54:52 UTC
(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.
Comment 18 Cosimo Alfarano 2011-03-03 08:34:53 UTC
The rest of the patches looks good (yet again, no hat).
Comment 19 Simon McVittie 2011-06-24 05:49:54 UTC
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.
Comment 20 Simon McVittie 2011-06-24 08:20:00 UTC
Patches 1-10 merged, based on review (+ acknowledgement IRL) from Cosimo.

Retitling this bug for Alban's additional patch.
Comment 21 Ross Burton 2014-09-23 14:08:08 UTC
Gaming bugzilla isn't nice.

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct.