Attachment 46155 Details for Bug 34393 – Remove the per-connection link cache (v2)

[patch] Remove the per-connection link cache (v2)

0001-Remove-the-per-connection-link-cache.patch (text/plain), 4.63 KB, created by Simon McVittie on 2011-04-29 07:15:56 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Simon McVittie

Created: 2011-04-29 07:15:56 UTC

Size: 4.63 KB

patch

obsolete

>From edc69b257415300e3b22be6b7884034c5ba6e654 Mon Sep 17 00:00:00 2001
>From: Simon McVittie <simon.mcvittie@collabora.co.uk>
>Date: Fri, 29 Apr 2011 15:08:53 +0100
>Subject: [PATCH] Remove the per-connection link cache
>
>With fd.o#34393 fixed, retaking the lock to cache unused links
>significantly adds to locking overhead (-18% throughput in a synthetic
>benchmark on an ARM device). The cache is also unlimited in size, and
>probably contributes to memory growth and fragmentation by not being
>under the system malloc's control.
>
>Fixing fd.o #34393, but also dropping this cache entirely, turns out to
>lead to a 5% increase in throughput on the same synthetic benchmark.
>---
> dbus/dbus-connection.c |   61 ++++++++---------------------------------------
> 1 files changed, 11 insertions(+), 50 deletions(-)
>
>diff --git a/dbus/dbus-connection.c b/dbus/dbus-connection.c
>index 241c8f1..04b246a 100644
>--- a/dbus/dbus-connection.c
>+++ b/dbus/dbus-connection.c
>@@ -286,9 +286,6 @@ struct DBusConnection
> 
>   DBusDispatchStatus last_dispatch_status; /**< The last dispatch status we reported to the application. */
> 
>-  DBusList *link_cache; /**< A cache of linked list links to prevent contention
>-                         *   for the global linked list mempool lock
>-                         */
>   DBusObjectTree *objects; /**< Object path handlers registered with this connection */
> 
>   char *server_guid; /**< GUID of server if we are in shared_connections, #NULL if server GUID is unknown or connection is private */
>@@ -393,27 +390,15 @@ _dbus_connection_unlock (DBusConnection *connection)
>   RELEASING_LOCK_CHECK (connection);
>   _dbus_mutex_unlock (connection->mutex);
> 
>-  for (iter = _dbus_list_get_first_link (&expired_messages);
>+  for (iter = _dbus_list_pop_first_link (&expired_messages);
>       iter != NULL;
>-      iter = _dbus_list_get_next_link (&expired_messages, iter))
>+      iter = _dbus_list_pop_first_link (&expired_messages))
>     {
>       DBusMessage *message = iter->data;
> 
>       dbus_message_unref (message);
>-      iter->data = NULL;
>+      _dbus_list_free_link (iter);
>     }
>-
>-  /* Take the lock back for a moment, to copy the links into the link
>-   * cache. FIXME: with this extra cost, is it still advantageous to have
>-   * the link cache? */
>-  _dbus_mutex_lock (connection->mutex);
>-
>-  for (iter = _dbus_list_pop_first_link (&expired_messages);
>-      iter != NULL;
>-      iter = _dbus_list_pop_first_link (&expired_messages))
>-    _dbus_list_prepend_link (&connection->link_cache, iter);
>-
>-  _dbus_mutex_unlock (connection->mutex);
> }
> 
> /**
>@@ -671,11 +656,7 @@ _dbus_connection_message_sent_unlocked (DBusConnection *connection,
> 
>   /* It's OK that in principle we call the notify function, because for the
>    * outgoing limit, there isn't one */
>-  _dbus_message_remove_counter (message, connection->outgoing_counter,
>-                                &link);
>-
>-  /* Save this link in the link cache also */
>-  _dbus_list_prepend_link (&connection->link_cache, link);
>+  _dbus_message_remove_counter (message, connection->outgoing_counter, NULL);
> 
>   /* The message will actually be unreffed when we unlock */
> }
>@@ -1967,31 +1948,13 @@ _dbus_connection_preallocate_send_unlocked (DBusConnection *connection)
>   if (preallocated == NULL)
>     return NULL;
> 
>-  if (connection->link_cache != NULL)
>-    {
>-      preallocated->queue_link =
>-        _dbus_list_pop_first_link (&connection->link_cache);
>-      preallocated->queue_link->data = NULL;
>-    }
>-  else
>-    {
>-      preallocated->queue_link = _dbus_list_alloc_link (NULL);
>-      if (preallocated->queue_link == NULL)
>-        goto failed_0;
>-    }
>-  
>-  if (connection->link_cache != NULL)
>-    {
>-      preallocated->counter_link =
>-        _dbus_list_pop_first_link (&connection->link_cache);
>-      preallocated->counter_link->data = connection->outgoing_counter;
>-    }
>-  else
>-    {
>-      preallocated->counter_link = _dbus_list_alloc_link (connection->outgoing_counter);
>-      if (preallocated->counter_link == NULL)
>-        goto failed_1;
>-    }
>+  preallocated->queue_link = _dbus_list_alloc_link (NULL);
>+  if (preallocated->queue_link == NULL)
>+    goto failed_0;
>+
>+  preallocated->counter_link = _dbus_list_alloc_link (connection->outgoing_counter);
>+  if (preallocated->counter_link == NULL)
>+    goto failed_1;
> 
>   _dbus_counter_ref (preallocated->counter_link->data);
> 
>@@ -2783,8 +2746,6 @@ _dbus_connection_last_unref (DBusConnection *connection)
>       _dbus_list_free_link (connection->disconnect_message_link);
>     }
> 
>-  _dbus_list_clear (&connection->link_cache);
>-  
>   _dbus_condvar_free_at_location (&connection->dispatch_cond);
>   _dbus_condvar_free_at_location (&connection->io_path_cond);
> 
>-- 
>1.7.4.4
>

Actions: View | Splinter Review

Attachments on bug 34393: 43814 | 43815 | 43816 | 43817 | 43818 | 43819 | 43820 | 43821 | 43822 | 43823 | 44207 | 44314 | 44317 | 44353 | 46155 | 46157 | 48281

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.