Attachment 44207 Details for Bug 34393 – Remove the per-connection link cache

[patch] Remove the per-connection link cache

0001-Remove-the-per-connection-link-cache.patch (text/plain), 4.51 KB, created by Simon McVittie on 2011-03-07 09:55:06 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Simon McVittie

Created: 2011-03-07 09:55:06 UTC

Size: 4.51 KB

patch

obsolete

>From 81ca5ee36d115134643d875d325d36e270af85ef Mon Sep 17 00:00:00 2001
>From: Simon McVittie <simon.mcvittie@collabora.co.uk>
>Date: Mon, 7 Mar 2011 17:48:53 +0000
>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 |   59 +++++++++---------------------------------------
> 1 files changed, 11 insertions(+), 48 deletions(-)
>
>diff --git a/dbus/dbus-connection.c b/dbus/dbus-connection.c
>index 241c8f1..79c8fc9 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);
> }
> 
> /**
>@@ -673,9 +658,7 @@ _dbus_connection_message_sent_unlocked (DBusConnection *connection,
>    * 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_list_free_link (link);
> 
>   /* The message will actually be unreffed when we unlock */
> }
>@@ -1967,31 +1950,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 +2748,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.1
>

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.