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().
Created attachment 30095 [details] [review] Implementation
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.
(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().
Created attachment 30165 [details] [review] Fixed implementation
Created attachment 30230 [details] [review] Bug23846-v3.patch Updated version: DBusGProxy avoids to subscribe several times to the same NameOwnerChanged match rule.
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.
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.
(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?
(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.)
(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.
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.
(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.)
Created attachment 51803 [details] [review] correctly-attributed patch
(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.
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. How we collect and use information is described in our Privacy Policy.