Bug 23846 - make DBusGProxy signal-match rules more specific
Summary: make DBusGProxy signal-match rules more specific
Status: RESOLVED WONTFIX
Alias: None
Product: dbus
Classification: Unclassified
Component: GLib (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Alban Crequy
QA Contact: John (J5) Palmieri
URL:
Whiteboard: review-
Keywords: patch
Depends on:
Blocks:
 
Reported: 2009-09-10 10:10 UTC by Alban Crequy
Modified: 2013-09-10 17:16 UTC (History)
3 users (show)

See Also:
i915 platform:
i915 features:


Attachments
Implementation (9.12 KB, patch)
2009-10-05 14:42 UTC, Alban Crequy
Details | Splinter Review
Fixed implementation (9.71 KB, patch)
2009-10-08 08:51 UTC, Alban Crequy
Details | Splinter Review
Bug23846-v3.patch (14.94 KB, patch)
2009-10-09 10:34 UTC, Alban Crequy
Details | Splinter Review
Don't subscribe to all D-Bus signals but only those the user is interested in (11.06 KB, patch)
2011-09-30 09:04 UTC, Simon McVittie
Details | Splinter Review
correctly-attributed patch (11.06 KB, patch)
2011-09-30 09:10 UTC, Simon McVittie
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Alban Crequy 2009-09-10 10:10:06 UTC
Any DBusGProxy will subscribe with dbus_bus_add_match() to all signal from the object on an interface. It should be able to receive only signals which are interesting for the user of DBusGProxy.

The code is in: dbus/dbus-gproxy.c dbus_g_proxy_manager_register():
And the rule is defined by g_proxy_get_signal_match_rule():
g_strdup_printf ("type='signal',sender='%s',path='%s',interface='%s'",
                            priv->name, priv->path, priv->interface);

It should be possible to hook a precise match rule with dbus_bus_add_match() in dbus_g_proxy_connect_signal() and remove it later in dbus_g_proxy_disconnect_signal().
Comment 1 Alban Crequy 2009-10-05 14:42:27 UTC
Created attachment 30095 [details] [review]
Implementation
Comment 2 Simon McVittie 2009-10-06 05:15:23 UTC
A conservative version would be to detect whether we're talking to org.freedesktop.DBus, and use split-up match rules (like this) for the dbus-daemon, but combined match rules for everything else.
Comment 3 Alban Crequy 2009-10-07 07:27:56 UTC
(In reply to comment #1)
> Created an attachment (id=30095) [details]
> Implementation

2 bugs in that patch:
- When DBusGProxy is used in a peer to peer dbus connection (without dbus-daemon), it crashes on _dispose().
- The match rules are not removed correctly on _dispose().

Comment 4 Alban Crequy 2009-10-08 08:51:30 UTC
Created attachment 30165 [details] [review]
Fixed implementation
Comment 5 Alban Crequy 2009-10-09 10:34:15 UTC
Created attachment 30230 [details] [review]
Bug23846-v3.patch

Updated version: DBusGProxy avoids to subscribe several times to the same
NameOwnerChanged match rule.
Comment 6 Will Thompson 2009-11-11 08:22:18 UTC
Comment on attachment 30230 [details] [review]
Bug23846-v3.patch

Hmm. On the system bus, each connection can only add 512 match rules. This patch would make it easier to hit that limit by mistake… I'm also a tad skeptical that this helps for anything other than proxies for the bus daemon (where making a DBusGProxy would connect to all the signals on it, including NOC).

+  /**
+   * Set of signals with the DBusGProxy which subscribes to the signals
+   * gchar *signal_name -> GHashTable* of (DBusGProxy *proxy -> guint count)
+   */
+  GHashTable *signal_subscribed;

I'd call this signal_subscriptions. And it took me a few tries to parse the comment. How about: “FIXME”

+  GHashTable *owner_match_rule; /**< Hash to keep trace of match rules of 
+                                 * NameOwnerChanged.
+                                 * gchar *name -> guint refcount
+                                 */

I'd give the variable a plural name (owner_match_rules), and s/trace/track/, s/rules of/rules for/.

No caller of g_proxy_get_signal_match_rule passes a null signal_name, so that code path seems unnecessary.

+g_proxy_get_owner_match_rule (const gchar *name)

This function is now misleadingly named: it doesn't act on a DBusGProxy.

+      gpointer proxy_set; 
+      gpointer signal_name; 

+              dbus_bus_remove_match (manager->connection,
+                                     rule, NULL);
+              g_free (rule);      

+      if (!found || g_hash_table_size (proxy_set) == 0)
+        {
+          char *rule;
+  

+        {
+          g_hash_table_insert (proxy_set, proxy,
+              GINT_TO_POINTER (handler_count));
+        }
+      

Trailing whitespace. Git complained at me. Some of this is probably pre-existing, from code you've moved around, but it's probably worth cleaning it up at the same time.

+  /* We have to add match rules to the server,
+   * but only if the server is a message bus,
+   * not if it's a peer.
+   */
+  if (priv->name && list->signal_subscribed != NULL)

Why would list->signal_subscribed be NULL here if priv->name is not?

Otherwise this looks fine, apart from being skeptical that it makes any difference except for squashing the bug where making a proxy for the bus daemon subscribes you to NameOwnerChanged.
Comment 7 Alban Crequy 2009-11-16 05:59:14 UTC
My git branch for easier review:
http://git.collabora.co.uk/?p=user/alban/dbus-glib;a=shortlog;h=refs/heads/signalmatch

(In reply to comment #6)
> (From update of attachment 30230 [details] [review])
> Hmm. On the system bus, each connection can only add 512 match rules. This
> patch would make it easier to hit that limit by mistake…

The limit on the system bus is 512 per connection (default in bus/config-parser.c, usually not modified by system.conf). The limit on the session bus is 50000 (defined in /etc/dbus-1/session.conf)

I used the dbus-daemon patch attached on Bug #24307 to count the number of matche rules on an embedded hardware and on my GNOME desktop when this DBusGProxy patch is applied:

On the embedded device:
 - Total of 2532 match rules on the session bus (no problem here).
 - Total of 2049 match rules on the system bus, and the connection with the largest amount of match rules has 99 match rules. The second on top has 61 match rules, and then it is under 31.

On my Debian/GNOME desktop:
 - Total of 2209 match rules on the session bus (no problem here).
 - Total of 258 match rules on the system bus, and the connection with the largest amount of match rules has 30 match rules. It seems DBusGProxy is not used so much on the system bus...

So it does not reach the limit (30<512). And if it did, a part of reason to have a low limit (because
there was a linearly walking on each D-Bus message) is fixed in Bug #23117.

> I'm also a tad
> skeptical that this helps for anything other than proxies for the bus daemon
> (where making a DBusGProxy would connect to all the signals on it, including
> NOC).

Unfortunately, I didn't succeed to have any useful numbers.

> +  /**
> +   * Set of signals with the DBusGProxy which subscribes to the signals
> +   * gchar *signal_name -> GHashTable* of (DBusGProxy *proxy -> guint count)
> +   */
> +  GHashTable *signal_subscribed;
> 
> I'd call this signal_subscriptions.

Fixed

> And it took me a few tries to parse the
> comment. How about: “FIXME”

I rephrased

> +  GHashTable *owner_match_rule; /**< Hash to keep trace of match rules of 
> +                                 * NameOwnerChanged.
> +                                 * gchar *name -> guint refcount
> +                                 */
> 
> I'd give the variable a plural name (owner_match_rules), and s/trace/track/,
> s/rules of/rules for/.

Done

> No caller of g_proxy_get_signal_match_rule passes a null signal_name, so that
> code path seems unnecessary.

Fixed

> +g_proxy_get_owner_match_rule (const gchar *name)
> 
> This function is now misleadingly named: it doesn't act on a DBusGProxy.

Renamed to get_owner_match_rule

> +      gpointer proxy_set; 
> +      gpointer signal_name; 
> 
> +              dbus_bus_remove_match (manager->connection,
> +                                     rule, NULL);
> +              g_free (rule);      
> 
> +      if (!found || g_hash_table_size (proxy_set) == 0)
> +        {
> +          char *rule;
> +  
> 
> +        {
> +          g_hash_table_insert (proxy_set, proxy,
> +              GINT_TO_POINTER (handler_count));
> +        }
> +      
> 
> Trailing whitespace. Git complained at me. Some of this is probably
> pre-existing, from code you've moved around, but it's probably worth cleaning
> it up at the same time.

Removed

> +  /* We have to add match rules to the server,
> +   * but only if the server is a message bus,
> +   * not if it's a peer.
> +   */
> +  if (priv->name && list->signal_subscribed != NULL)
> 
> Why would list->signal_subscribed be NULL here if priv->name is not?

No reason. Fixed.

> Otherwise this looks fine, apart from being skeptical that it makes any
> difference except for squashing the bug where making a proxy for the bus daemon
> subscribes you to NameOwnerChanged.

Comment 8 Will Thompson 2009-12-09 10:28:32 UTC
(In reply to comment #7)
> So it does not reach the limit (30<512). And if it did, a part of reason to
> have a low limit (because
> there was a linearly walking on each D-Bus message) is fixed in Bug #23117.

Of course, having n match rules per (message type, interface) bucket rather than 1 makes checking the bucket a marginally bigger deal... :-)

I wonder whether it'd be enough to make DBusGProxy bind to the whole interface, but only if at least one GObject-side signal's being listened to.

Anyone else have any opinions either way here?
Comment 9 Will Thompson 2010-02-20 05:30:18 UTC
(http://cgit.freedesktop.org/dbus/dbus/commit/?h=dbus-1.2&id=9c90fcd2dc4b1b7d818a35ef43d4686052902f59 claims to reference this bug, but in fact refers to bug #23925.)
Comment 10 Alban Crequy 2011-01-28 05:25:39 UTC
(In reply to comment #5)
> Updated version: DBusGProxy avoids to subscribe several times to the same
> NameOwnerChanged match rule.

I created Bug #33646 for this specific issue because I think it is more important than the controversial change to bind on individual signals.
Comment 11 Simon McVittie 2011-09-30 09:04:11 UTC
Created attachment 51802 [details] [review]
Don't subscribe to all D-Bus signals but only those the user is interested in

I still don't think subscribing to individual signals is the right trade-off in general - I think one match rule per interface is fine, except for org.freedesktop.DBus which is special - but here's an updated patch for Alban's proposed behaviour, which can be applied after Bug #33646.
Comment 12 Simon McVittie 2011-09-30 09:09:06 UTC
(In reply to comment #8)
> I wonder whether it'd be enough to make DBusGProxy bind to the whole interface,
> but only if at least one GObject-side signal's being listened to.

One risk with this, which Alban's patch also has, is that the subscription does not take effect until AddMatch reaches the bus daemon - so you can't know that you'll be up-to-date on signals until you connect to the signal, *and* make a round-trip (method call) to the bus daemon or another process.

(In the common "connect to signal + recover state" pattern, the state-recovery is such a round-trip, so either way is OK; I think this risk probably only applies to D-Bus APIs that are already flawed.)
Comment 13 Simon McVittie 2011-09-30 09:10:28 UTC
Created attachment 51803 [details] [review]
correctly-attributed patch
Comment 14 Simon McVittie 2012-04-12 03:00:30 UTC
(In reply to comment #11)
> I still don't think subscribing to individual signals is the right trade-off in
> general

This is review- for this reason. I could be persuaded by benchmarks, but I'm afraid "the match rules are finer-grained" (without numbers on how much this actually reduces wakeups / how much throughput suffers as a result) isn't sufficiently convincing.
Comment 15 Simon McVittie 2013-09-10 17:16:41 UTC
dbus-glib is basically dead. No more significant development, especially if it's controversial, so WONTFIX.


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