Bug 33646

Created attachment 42643 [details] [review]
Bug33646-DBusGProxy-remove-duplicate-owner-match-rules.patch

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.

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?

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.

Created attachment 43801 [details] [review]
0001-DBusGProxy-remove-duplicate-owner-match-rules.patch

Same patch with slice-allocated guint

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...)

Fixed in git for 0.93