Bug 33646

Summary: DBusGProxy adds duplicate NameOwnerChanged match rules
Product: dbus Reporter: Alban Crequy <alban.crequy>
Component: GLibAssignee: Alban Crequy <alban.crequy>
Status: RESOLVED FIXED QA Contact: John (J5) Palmieri <johnp>
Severity: normal    
Priority: medium CC: robin.bateboerop
Version: unspecifiedKeywords: patch
Hardware: Other   
OS: All   
Whiteboard: review+
i915 platform: i915 features:
Attachments: Bug33646-DBusGProxy-remove-duplicate-owner-match-rules.patch
Bug33646-DBusGProxy-remove-duplicate-owner-match-rules-v2.patch
0001-DBusGProxy-remove-duplicate-owner-match-rules.patch

Description Alban Crequy 2011-01-28 05:23:06 UTC
dbus-glib adds one NameOwnerChanged match rule per DBusGProxy, even if several DBusGProxy are on the same D-Bus connection. This generates duplicate match rules. This is not necessary.

When Telepathy starts (with Empathy, MC, Gabble, etc.), 59% of D-Bus traffic is AddMatch() messages and about half of AddMatch() messages are NameOwnerChanged match rules for watching Gabble.

The patch attached to Bug #23846 fixes this but the patch mixes two changes:
- the NameOwnerChanged match rule problem (this bug)
- bind match rules to individual signals instead of the traditional one-match-per-interface

I am writing a smaller patch only for this bug.
Comment 1 Alban Crequy 2011-01-28 05:57:07 UTC
Created attachment 42643 [details] [review]
Bug33646-DBusGProxy-remove-duplicate-owner-match-rules.patch
Comment 2 Alban Crequy 2011-01-28 06:22:50 UTC
Restarting Telepathy is not a very consistent test, I get different results each time I try with or without the patch. But the patch still saves from 300 to 700 messages. Total amount of messages is around 7000.

So the improvement is not as huge as I thought, but it is still good.
Comment 3 Will Thompson 2011-02-02 07:20:11 UTC
Review of attachment 42643 [details] [review]:

::: dbus/dbus-gproxy.c
@@ +508,3 @@
         "',interface='" DBUS_INTERFACE_DBUS
         "',member='NameOwnerChanged'"
+    ",arg0='%s'", name);

This indentation doesn't line up.

@@ +1003,3 @@
+          g_hash_table_insert (manager->owner_match_rules, g_strdup (priv->name),
+              GINT_TO_POINTER (1));
+        }

The lookup_extended, steal, insert dance always bothers me. I always forget the difference between insert and replace and why steal is needed. Maybe the values could be slice-allocated guint *s? Then this would go:

guint *refcount = g_hash_table_lookup (...);

if (refcount != NULL)
  {
    g_assert (*refcount != 0);
    g_assert (*refcount < G_MAXUINT);
    *refcount++;
  }
else
  {
    refcount = g_slice_new (guint);
    *refcount = 1;
    g_hash_table_insert (...);
  }

Also you could use the rule as the key in the hash table maybe?
Comment 4 Alban Crequy 2011-02-03 05:04:05 UTC
Created attachment 42898 [details] [review]
Bug33646-DBusGProxy-remove-duplicate-owner-match-rules-v2.patch

(In reply to comment #3)
> Review of attachment 42643 [details] [review]:
> 
> ::: dbus/dbus-gproxy.c
> @@ +508,3 @@
>          "',interface='" DBUS_INTERFACE_DBUS
>          "',member='NameOwnerChanged'"
> +    ",arg0='%s'", name);
> 
> This indentation doesn't line up.

Fixed in the attached patch.

> @@ +1003,3 @@
> +          g_hash_table_insert (manager->owner_match_rules, g_strdup
> (priv->name),
> +              GINT_TO_POINTER (1));
> +        }
> 
> The lookup_extended, steal, insert dance always bothers me. I always forget the
> difference between insert and replace and why steal is needed.

Both insert and replace will destroy a key with key_destroy_func. The former will destroy the old key, and the latter will destroy the new key. In this context, the old key and the new key are the same and I want to destroy none of them. So I use steal to avoid the release and I can use either insert or replace indifferently.

> Maybe the values
> could be slice-allocated guint *s? Then this would go:

Then I would need a new GHashTable's value_destroy_func callback with g_slice_free(). I can do it if you think it worth it for the readability.

> Also you could use the rule as the key in the hash table maybe?

I could.... but then I would need to build the rule with get_owner_match_rule() both when the rule already exists and when it does not exist. It is not so expensive, but I don't see why it is better to use the rule as a key.
Comment 5 Alban Crequy 2011-02-25 06:14:30 UTC
Created attachment 43801 [details] [review]
0001-DBusGProxy-remove-duplicate-owner-match-rules.patch

Same patch with slice-allocated guint
Comment 6 Simon McVittie 2011-04-13 05:58:00 UTC
Review of attachment 43801 [details] [review]:

This looks good to me. Last chance to make any refinements you want; I'll merge it shortly.

::: dbus/dbus-gproxy.c
@@ +152,3 @@
+  GHashTable *owner_match_rules; /**< Hash to keep track of match rules of
+                                  * NameOwnerChanged.
+                                  * gchar *name -> guint *refcount

Not a problem here, but in future I think I'd prefer comments above struct members: this end-of-line style doesn't really work for long comments.

(FYI, /**< is a Doxygen thing, and in Bug #10890 I propose to destroy the last remnants of support for Doxygen...)
Comment 7 Simon McVittie 2011-04-19 03:11:54 UTC
Fixed in git for 0.93

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.