From a480638774368497bce03b1db4297f9436725460 Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Thu, 21 Feb 2013 19:31:38 +0100 Subject: [PATCH] eds: avoid blocking event processing When there are many incoming D-Bus change notifications, processing of other D-Bus messages can be delayed considerably. This commit is a first step towards solving this by caching the change notifications and processing them with lower priority in a glib idle callback. Ordering of the changes relative to each other is preserved, so semantically this is the same as immediate processing. The worst-case scenario is when contacts get added one-by-one to EDS address books while folks is running, because then each change triggers one D-Bus signal. With 2000 contacts and four address books on a fast laptop, the process hosting folks became unresponsive for 137s. The patch reduced that to 0.1s. The downside is an increase of total test runtime of 4%. In the scenario where data is already in EDS when folks starts, the patch reduced response time from 2.3s to 0.2s. See https://bugs.freedesktop.org/show_bug.cgi?id=60851#c6 for details. --- backends/eds/lib/edsf-persona-store.vala | 127 +++++++++++++++++++++++++++++++ 1 file changed, 127 insertions(+) diff --git a/backends/eds/lib/edsf-persona-store.vala b/backends/eds/lib/edsf-persona-store.vala index 3d6d37b..dae25fa 100644 --- a/backends/eds/lib/edsf-persona-store.vala +++ b/backends/eds/lib/edsf-persona-store.vala @@ -2281,8 +2281,118 @@ public class Edsf.PersonaStore : Folks.PersonaStore } } + /* + * The EDS store receives change notifications as quickly as it + * can and then stores them for later processing in a glib idle + * callback. + * + * This avoids problems where many incoming D-Bus change notifications + * block processing of other incoming D-Bus messages. Delays of over a minute + * have been observed in worst-case (but not entirely unrealistic) scenarios + * (1000 contacts added one-by-one while folks is active). See + * https://bugzilla.gnome.org/show_bug.cgi?id=694385 (folks issue) and + * http://bugs.freedesktop.org/show_bug.cgi?id=60851 (SyncEvolution issue). + * + * Cannot store a GLib.SourceFunc directly + * in a LinkedList ("Delegates with target are not supported as generic type arguments", + * https://mail.gnome.org/archives/vala-list/2011-June/msg00002.html) + * and thus have to wrap it in a class. When dealing with delegates, we must be + * careful to transfer ownership, because they are not reference counted. + */ + class IdleTask + { + public GLib.SourceFunc callback; + } + + + private Gee.Queue _idle_tasks = new Gee.LinkedList (); + private uint _idle_handle = 0; + + /* + * Deal with some chunk of work encapsulated in the delegate later. + * As in any other SourceFunc, the callback may request to be called + * again by returning true. In contrast to Idle.add, _idle_queue + * ensures that only one task is processed per idle cycle. + */ + private void _idle_queue (owned GLib.SourceFunc callback) + { + IdleTask task = new IdleTask (); + task.callback = (owned) callback; + this._idle_tasks.add (task); + /* + * Ensure that there is an active idle callback to process + * the task, otherwise register it. We cannot just + * queue each task separately, because then we might + * end up with multiple tasks being done at once + * when the process gets idle, instead of one + * task at a time. + */ + if (this._idle_handle == 0) + { + this._idle_handle = Idle.add (this._idle_process); + } + } + + private bool _idle_process () + { + IdleTask? task = this._idle_tasks.peek (); + if (task != null) + { + if (task.callback ()) + { + /* Task is not done yet, run it again later. */ + return true; + } + this._idle_tasks.poll (); + } + + /* Check for future work. */ + task = this._idle_tasks.peek (); + if (task == null) + { + /* + * Remember that we need to re-register idle + * processing when _idle_queue is called again. + */ + this._idle_handle = 0; + /* Done, will remove idle handler. */ + return false; + } + else + { + /* Continue processing. */ + return true; + } + } + + private Gee.LinkedList _copy_contacts (GLib.List contacts) + { + var copy = new Gee.LinkedList (); + foreach (E.Contact c in contacts) + { + copy.add (c); + } + return copy; + } + + private Gee.LinkedList _copy_contacts_ids (GLib.List contacts_ids) + { + var copy = new Gee.LinkedList (); + foreach (unowned string s in contacts_ids) + { + copy.add (s); + } + return copy; + } + private void _contacts_added_cb (GLib.List contacts) { + var copy = this._copy_contacts (contacts); + this._idle_queue (() => { return this._contacts_added_idle (copy); }); + } + + private bool _contacts_added_idle (Gee.List contacts) + { HashSet added_personas; /* If the persona store hasn't yet reached quiescence, queue up the @@ -2318,10 +2428,19 @@ public class Edsf.PersonaStore : Folks.PersonaStore { this._emit_personas_changed (added_personas, null); } + + /* Done. */ + return false; } private void _contacts_changed_cb (GLib.List contacts) { + var copy = this._copy_contacts (contacts); + this._idle_queue (() => { return this._contacts_changed_idle (copy); }); + } + + private bool _contacts_changed_idle (Gee.List contacts) + { foreach (E.Contact c in contacts) { var iid = Edsf.Persona.build_iid_from_contact (this.id, c); @@ -2331,10 +2450,17 @@ public class Edsf.PersonaStore : Folks.PersonaStore ((!) persona)._update (c); } } + return false; } private void _contacts_removed_cb (GLib.List contacts_ids) { + var copy = this._copy_contacts_ids (contacts_ids); + this._idle_queue (() => { return this._contacts_removed_idle (copy); }); + } + + private bool _contacts_removed_idle (Gee.List contacts_ids) + { var removed_personas = new HashSet (); foreach (string contact_id in contacts_ids) @@ -2359,6 +2485,7 @@ public class Edsf.PersonaStore : Folks.PersonaStore { this._emit_personas_changed (null, removed_personas); } + return false; } private void _contacts_complete_cb (Error err) -- 2.7.4