GNOME Bugzilla – Bug 694385
non-blocking change processing
Last modified: 2013-03-26 15:56:12 UTC
While not specific to EDS, with EDS the problem is perhaps more noticable: when evolution-addressbook-factory sends many D-Bus messages to the process hosting folks, then the receiving process can end up with a long back log of unprocessed messages which prevent processing of other, more important messages. Delays of over a minute have been observed while importing 1000 contacts into EDS and from there into folks. There are multiple possible solutions, partly orthogonal to each other: - clear the EDS queue more quickly and process when idle - avoid D-Bus for reading contacts, use the direct read access instead - asynchronously process change notifications in the Individuals - general performance improvements
Created attachment 237091 [details] [review] clear the EDS queue more quickly and process when idle A first step. My performance test in SyncEvolution shows that it helps a lot already. I don't expect the issue to be resolved by this patch alone. More work will be needed on measuring and fixing performance.
Sounds like something worth fixing to me, so I've bumped the importance. There's certainly room for improvement here and possibly in EDS itself. It's only slightly related, but importing a 6,000-contact vCard file through Evolution to a Google contacts account took an incredibly long time. That's all below Folks (Evolution, EDS, and libgdata), but it's something that may be worth noting as performing badly.
Review of attachment 237091 [details] [review]: Looks pretty good. If it passes all the tests with the trivial changes below, please commit to master. Please include some details of the performance improvements this provides in your commit message so they don’t get lost. Thanks! ::: backends/eds/lib/edsf-persona-store.vala @@ +2289,3 @@ + // 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). Can you add a reference to this bug report please? @@ +2301,3 @@ + + + private Gee.LinkedList<IdleTask> _idle_tasks = new Gee.LinkedList<IdleTask> (); Could you change the type of _idle_tasks to be Gee.Queue<IdleTask> please? LinkedList implements Queue, but semantically the latter is what we want. Should prevent people accidentally using non-queue-like operations on _idle_tasks in the code below, too. @@ +2312,3 @@ + IdleTask task = new IdleTask (); + task.callback = (owned) callback; + this._idle_tasks.add (task); You might want to manually inspect the generated C code for this, just to ensure it’s correctly copying the delegate and its closure around, and not leaking anything. I’m not quite sure I trust valac to compile this correctly at the moment. Or you could poke me to take a look before committing. @@ +2323,3 @@ + this._idle_handle = Idle.add (this._idle_process); + } + } Missing blank line between the methods. @@ +2375,3 @@ private void _contacts_added_cb (GLib.List<E.Contact> contacts) { + var copy = _copy_contacts (contacts); Please explicitly use ‘this._copy_contacts()’ to make the method name/location explicit. @@ +2423,3 @@ private void _contacts_changed_cb (GLib.List<E.Contact> contacts) { + var copy = _copy_contacts (contacts); Same here. @@ +2443,3 @@ private void _contacts_removed_cb (GLib.List<string> contacts_ids) { + var copy = _copy_contacts_ids (contacts_ids); And here.
Created attachment 239770 [details] [review] clear the EDS queue more quickly and process when idle, II (In reply to comment #3) > Review of attachment 237091 [details] [review]: > > Looks pretty good. If it passes all the tests Getting them to run took a bit more work independently from this patch, will post the necessary patches here. > with the trivial changes below, Done. Updated patch attached, just in case that you want to have another look.
Created attachment 239771 [details] [review] do not depend on Telepathy for testing I had to patch autotools files a bit to get the EDS tests to run with Telepathy disabled.
Created attachment 239772 [details] [review] avoid false warning during debugging
Created attachment 239773 [details] [review] make set-phones pass with libphonenumber-capable EDS
Review of attachment 239770 [details] [review]: Still need some details of the performance improvements in the commit message (as per comment #3). A few things I missed in my first review are below. Sorry to be a pain. Please commit once they’re fixed! ::: backends/eds/lib/edsf-persona-store.vala @@ +2297,3 @@ + // 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. Please use /* this style of comment */ for consistency with the rest of the code. @@ +2330,3 @@ + private bool _idle_process () + { + IdleTask? task = this._idle_tasks.peek (); This line (and the rest of the method) are indented 2 spaces too far. (Sorry, I know this is really picky.)
Review of attachment 239771 [details] [review]: Looks good.
Review of attachment 239772 [details] [review]: ::: folks/backend-store.vala @@ -670,3 @@ } - /* We should have only .la .so and sub-directories */ - else if (mime != "application/x-sharedlib" && This check (against “application/x-sharedlib”) seems to have been lost.
Review of attachment 239773 [details] [review]: ::: tests/eds/set-phones.vala @@ +55,3 @@ this._test_set_phones_async.begin (); + TestUtils.loop_run_with_timeout (this._main_loop, 60); Why does the timeout need extending? @@ +129,3 @@ phone_1.set_parameter (AbstractFieldDetails.PARAM_TYPE, AbstractFieldDetails.PARAM_TYPE_HOME); + if (phone_fd.value == "1234") Would be useful to add a brief comment saying why we’re not using phone_fd.equal() here.
(In reply to comment #8) > Review of attachment 239770 [details] [review]: > > Still need some details of the performance improvements in the commit message > (as per comment #3). Right. I've redone benchmarks today and updated the commit comment: Author: Patrick Ohly <patrick.ohly@intel.com> Date: Thu Feb 21 19:31:38 2013 +0100 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. > A few things I missed in my first review are below. Sorry to be a pain. Please > commit once they’re fixed! > > ::: backends/eds/lib/edsf-persona-store.vala > @@ +2297,3 @@ > + // 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. > > Please use /* this style of comment */ for consistency with the rest of the > code. Okay. > @@ +2330,3 @@ > + private bool _idle_process () > + { > + IdleTask? task = this._idle_tasks.peek (); > > This line (and the rest of the method) are indented 2 spaces too far. Fixed. > (Sorry, I know this is really picky.) No problem.
(In reply to comment #10) > Review of attachment 239772 [details] [review]: > > ::: folks/backend-store.vala > @@ -670,3 @@ > } > - /* We should have only .la .so and sub-directories */ > - else if (mime != "application/x-sharedlib" && > > This check (against “application/x-sharedlib”) seems to have been lost. Ooops. That wasn't intentional. I'll remove that chunk. (In reply to comment #11) > Review of attachment 239773 [details] [review]: > > ::: tests/eds/set-phones.vala > @@ +55,3 @@ > this._test_set_phones_async.begin (); > > + TestUtils.loop_run_with_timeout (this._main_loop, 60); > > Why does the timeout need extending? Same here - I was in a hurry and skipped my own patch review before submitting, otherwise this wouldn't have crept into the patch. > @@ +129,3 @@ > phone_1.set_parameter (AbstractFieldDetails.PARAM_TYPE, > AbstractFieldDetails.PARAM_TYPE_HOME); > + if (phone_fd.value == "1234") > > Would be useful to add a brief comment saying why we’re not using > phone_fd.equal() here. Will do. Okay to commit with these fixes or shall I post here again?
(In reply to comment #13) > Okay to commit with these fixes or shall I post here again? Please do commit with those fixes. Don’t forget to update NEWS. Thanks!
Comment on attachment 239770 [details] [review] clear the EDS queue more quickly and process when idle, II commit a480638
Comment on attachment 239773 [details] [review] make set-phones pass with libphonenumber-capable EDS Revised commit 4a5d0e0 is in master.
Comment on attachment 239771 [details] [review] do not depend on Telepathy for testing commit 9064f78
Comment on attachment 239772 [details] [review] avoid false warning during debugging Revised commit f7035a is in master.