GNOME Bugzilla – Bug 696104
add a performance test for realistically-sized e-d-s address books
Last modified: 2013-03-25 14:59:40 UTC
Bug #687161, Bug #682903, Bug #653239, Bug #694385 would all benefit from having a test-case for Folks performance with a realistically-sized (hundreds or low thousands of contacts) e-d-s address book. Ideally, this could optionally feed in from a developer's real address book (strictly one-way!) to check that the simulated contacts give a realistic picture of where the time is all going. This is also a necessary step to see whether the cache suggested in Bug #687671 is actually worth it.
Created attachment 239196 [details] [review] EdsTest.Backend: use an array for contacts
(In reply to comment #1) > EdsTest.Backend: use an array for contacts This means we don't have to worry about whether prepending is faster than appending, or about Vala getting confused about ownership in linked lists.
Created attachment 239197 [details] [review] EdsTest.Backend: add contacts as a batch, not one at a time This should make it much quicker to add contacts in bulk.
Created attachment 239198 [details] [review] EdsTest.Backend: allow direct access to EBookClient --- I used this at one point but I'm not sure whether I'm still relying on this, or on the two previous commits, so perhaps they can be rebased out if undesired.
Created attachment 239199 [details] [review] Add infrastructure to run helper test binaries I've included basic infrastructure to run them from an installed path instead of the builddir, because there seems to be considerable interest in that at the moment.
Created attachment 239200 [details] [review] eds test: add some helper programs for performance testing
Created attachment 239201 [details] [review] Centralize adjusting timeouts for valgrind/callgrind
Created attachment 239202 [details] [review] Add a performance test for contacts in e-d-s
Created attachment 239203 [details] [review] Add support for running tests under callgrind as well as memcheck --- Also include the valgrind infrastructure in the eds subdir. I'm using this to find "hot paths" in the e-d-s and aggregation code, so that I can focus on the bits that are slowing us down most.
Review of attachment 239196 [details] [review]: ++
Review of attachment 239197 [details] [review]: As long as you’ve given the generated C a quick check for sanity wrt the GSLists, this looks good.
Review of attachment 239198 [details] [review]: Fine to add if you still need it. If not, don’t bother.
Review of attachment 239199 [details] [review]: https://bugzilla.gnome.org/show_bug.cgi?id=690830#c30
Review of attachment 239200 [details] [review]: ::: tests/eds/helper-create-many-contacts.vala @@ +120,3 @@ + + private static int n_contacts = 2000; + private static string uid = ""; Private variables’ names should be prefixed with an underscore (to make it clearer they’re private). @@ +126,3 @@ + { "uid", 'u', 0, OptionArg.STRING, ref uid, + "Address book uid", "UID" }, + { null } Vala might automatically add the null terminator entry; you might want to check. ::: tests/eds/helper-delete-contacts.vala @@ +50,3 @@ + { "uid", 'u', 0, OptionArg.STRING, ref uid, + "Address book uid", "UID" }, + { null } As above. ::: tests/eds/helper-prepare-aggregator.vala @@ +45,3 @@ + + private static bool print_an_individual_id = false; + private static bool print_a_persona_uid = false; Need underscores, as above. @@ +54,3 @@ + ref print_an_individual_id, + "Print a more or less arbitrary Individual ID", null }, + { null } As above. @@ +64,3 @@ + Environment.get_variable ("FOLKS_BACKENDS_ALLOWED") != "eds" || + Environment.get_variable ("FOLKS_PRIMARY_STORE") == null || + Environment.get_variable ("FOLKS_PRIMARY_STORE") == null) FOLKS_PRIMARY_STORE is listed twice here. @@ +100,3 @@ + store.load_backends.begin (() => { prepared = true; }); + while (!prepared) + MainContext.default ().iteration (true); Instead of including all this main context iteration stuff multiple times, it would be cleaner to move the bulk of the code into a private async method and use ‘yield’ inside that. Then you only have to have the main context iteration stuff (or, in fact, MainLoop.run()) once in main(). @@ +138,3 @@ + var individual = iter.get_value (); + + debug ("%s => %s", iter.get_key (), individual.full_name); s/=>/→/, just because we can.
Review of attachment 239201 [details] [review]: This could be moved up into the TestCase class, to reduce duplication even further.
Review of attachment 239202 [details] [review]: Looking good. A few comments below. ::: tests/eds/perf.vala @@ +93,3 @@ + store.prepare.begin (() => { prepared = true; }); + while (!prepared) + MainContext.default ().iteration (true); This could be factored out by making aggregate() async and yielding instead. @@ +148,3 @@ + + while (iter.next ()) + debug ("%s => %s", iter.get_key (), iter.get_value ().full_name); s/=>/→/ because why not. @@ +209,3 @@ + map.size); + + /* FIXME: there really ought to be convenience API for some of this. */ Please file a bug! :-) @@ +227,3 @@ + * + * FIXME: surely there ought to be a generic way to do this without + * iteration? */ You could use Persona.split_uid() to extract the IID from the UID, and then key into personas using that. I agree, this could be improved. @@ +305,3 @@ + try + { + string stdout; Probably not a great idea to name this after the FILE.
Review of attachment 239203 [details] [review]: This should be documented in HACKING. Otherwise looks good.
Comment on attachment 239196 [details] [review] EdsTest.Backend: use an array for contacts 30e36ca1d
Comment on attachment 239197 [details] [review] EdsTest.Backend: add contacts as a batch, not one at a time c0935ea5b
Comment on attachment 239203 [details] [review] Add support for running tests under callgrind as well as memcheck 3ea5bb83
(In reply to comment #14) > Private variables’ names should be prefixed with an underscore (to make it > clearer they’re private). Done. > Vala might automatically add the null terminator entry; you might want to > check. Good idea, but no, it doesn't. > FOLKS_PRIMARY_STORE is listed twice here. Fixed. > Instead of including all this main context iteration stuff multiple times, it > would be cleaner to move the bulk of the code into a private async method and > use ‘yield’ inside that. OK, I added a TestUtils method for the one thing that wasn't already available as async (waiting for the backend to quiesce). > s/=>/→/, just because we can. It'll need a setlocale() call, but yes. New patch in a moment. (In reply to comment #15) > This could be moved up into the TestCase class, to reduce duplication even > further. I added some better infrastructure to TestUtils instead (since the helper binaries don't use TestCase).
Created attachment 239389 [details] [review] Simplify tests that can time out There are basically two patterns: * something should stop the main loop within n seconds; if not, that's failure (this is now loop_run_with_timeout) * run the main loop for n seconds, then check whether things we wanted to happen happened (this is now loop_run_with_non_fatal_timeout) The latter is basically a bug in itself; I'm not going to fix that right now, but I did mark it as deprecated. In both cases, we want to adjust the timeout for running under Valgrind or Callgrind, which makes everything slow.
Created attachment 239396 [details] [review] eds test: add some helper programs for performance testing --- The stuff with Posix.dup2 is pretty unfortunate, but I can't see how to avoid it without saving to a temporary file, which seems worse.
Created attachment 239397 [details] [review] Add a performance test for contacts in e-d-s --- This is what I'm going to be trying to optimize. On my laptop, aggregating 500 contacts takes about 420ms, but aggregating 3x500 copies of the same contacts (which turns out to be a pathological case) takes about 10 seconds. Using my real address book (which has large inline avatars resulting in a nearly 19MB vCard) for most of the contacts, it's more like 480ms for 500 contacts. With three copies of those 500 contacts, Folks crashes with: Couldn't cache avatar for Edsf.Persona 'eds:test6:pas-id-514A00CC00000D7F': Error opening file '/tmp/tmp.x7By5PtrAC/.cache/folks/avatars/eds%3Atest6%3Apas-id-514A00CC00000D7F': Too many open files for which I'll file a separate bug.
Created attachment 239398 [details] [review] Add missing setlocale calls to helper binaries --- Otherwise the debug() calls can't output UTF-8 (see Bug #696179).
Comment on attachment 239198 [details] [review] EdsTest.Backend: allow direct access to EBookClient unnecessary, I think
Review of attachment 239389 [details] [review]: Great!
Review of attachment 239396 [details] [review]: One out-of-date documentation comment, but otherwise looks good to commit. ::: tests/lib/test-utils.vala @@ +189,2 @@ * * @param aggregator the aggregator to prepare Looks like this documentation comment (for backend_prepare_and_wait_for_quiescence()) is now out of date.
Review of attachment 239397 [details] [review]: ::: tests/eds/perf.vala @@ +233,3 @@ + * + * FIXME: surely there ought to be a generic way to do this without + * iteration? */ You should be able to use persona_id (which should be named ‘persona_iid’ for disambiguation) as a key into persona_store.personas, avoiding the iteration.
Review of attachment 239398 [details] [review]: ++
(In reply to comment #29) > Review of attachment 239397 [details] [review]: > > ::: tests/eds/perf.vala > @@ +233,3 @@ > + * > + * FIXME: surely there ought to be a generic way to do this without > + * iteration? */ > > You should be able to use persona_id (which should be named ‘persona_iid’ for > disambiguation) as a key into persona_store.personas, avoiding the iteration. Oh, I’ve just fully parsed bug #696215 and I’m wrong. Ignore comment #29. :-(
Review of attachment 239389 [details] [review]: Looks good.
Review of attachment 239396 [details] [review]: + /** + * Prepare an aggregator and wait for it to reach quiescence. You copied the comment block from aggregator_prepare_and_wait_for_quiescence() but forgot to s/aggregator/backend/. I've fixed it.
Review of attachment 239397 [details] [review]: + "$FOLKS_TESTS_SLOW to enable"); I think it's clearer to say "FOLKS_TESTS_SLOW=1", so I've changed it accordingly. --- Unfortunately, I lost a race with Philip to review this and committed it without his suggestion. But we can fix this after the fact.
Review of attachment 239398 [details] [review]: Merged
Merged Simon's fantastic set of clean-ups and perf work: commit 0c5551e0215c95cff973e3dae616c48ee8260984 Author: Simon McVittie <simon.mcvittie@collabora.co.uk> Date: Wed Mar 20 16:03:14 2013 +0000 Simplify tests that can time out There are basically two patterns: * something should stop the main loop within n seconds; if not, that's failure (this is now loop_run_with_timeout) * run the main loop for n seconds, then check whether things we wanted to happen happened (this is now loop_run_with_non_fatal_timeout) The latter is basically a bug in itself; I'm not going to fix that right now, but I did mark it as deprecated. In both cases, we want to adjust the timeout for running under Valgrind or Callgrind, which makes everything slow. eds test: add some helper programs for performance testing Add a performance test for contacts in e-d-s Add missing setlocale calls to helper binaries tests/eds/Makefile.am | 27 +- tests/eds/add-persona.vala | 8 +- tests/eds/anti-linking.vala | 18 +- tests/eds/avatar-details.vala | 8 +- tests/eds/create-remove-stores.vala | 11 +- tests/eds/email-details.vala | 8 +- tests/eds/enable-disable-stores.vala | 11 +- tests/eds/helper-create-many-contacts.vala | 170 +++++++++ tests/eds/helper-delete-contacts.vala | 94 +++++ tests/eds/helper-prepare-aggregator.vala | 186 ++++++++++ tests/eds/im-details.vala | 8 +- tests/eds/individual-retrieval.vala | 8 +- tests/eds/link-personas-diff-stores.vala | 9 +- tests/eds/link-personas.vala | 15 +- tests/eds/linkable-properties.vala | 10 +- tests/eds/name-details.vala | 8 +- tests/eds/perf.vala | 406 +++++++++++++++++++++ tests/eds/persona-store-tests.vala | 8 +- tests/eds/phone-details.vala | 8 +- tests/eds/postal-address-details.vala | 8 +- tests/eds/remove-persona.vala | 8 +- tests/eds/removing-contacts.vala | 7 +- tests/eds/set-avatar.vala | 17 +- tests/eds/set-birthday.vala | 7 +- tests/eds/set-emails.vala | 7 +- tests/eds/set-gender.vala | 8 +- tests/eds/set-im-addresses.vala | 7 +- tests/eds/set-is-favourite.vala | 8 +- tests/eds/set-names.vala | 7 +- tests/eds/set-notes.vala | 7 +- tests/eds/set-phones.vala | 7 +- tests/eds/set-postal-addresses.vala | 7 +- tests/eds/set-properties-race.vala | 7 +- tests/eds/set-roles.vala | 7 +- tests/eds/set-structured-name.vala | 7 +- tests/eds/set-urls.vala | 7 +- tests/eds/store-removed.vala | 20 +- tests/eds/updating-contacts.vala | 7 +- tests/folks/aggregation.vala | 96 +---- tests/folks/init.vala | 11 +- tests/key-file/individual-retrieval.vala | 21 +- tests/lib/libsocialweb/test-case.vala | 7 +- tests/lib/test-utils.vala | 118 ++++++ tests/libsocialweb/aggregation.vala | 14 +- tests/libsocialweb/dummy-lsw.vala | 28 +- tests/telepathy/individual-properties.vala | 36 +- tests/telepathy/individual-retrieval.vala | 18 +- tests/telepathy/init.vala | 10 +- tests/telepathy/persona-store-capabilities.vala | 12 +- tests/tracker/add-contact.vala | 8 +- tests/tracker/add-persona.vala | 8 +- tests/tracker/additional-names-updates.vala | 8 +- tests/tracker/avatar-details-interface.vala | 8 +- tests/tracker/avatar-updates.vala | 8 +- tests/tracker/birthday-details-interface.vala | 8 +- tests/tracker/birthday-updates.vala | 8 +- tests/tracker/default-contact.vala | 8 +- tests/tracker/duplicated-emails.vala | 8 +- tests/tracker/duplicated-phones.vala | 8 +- tests/tracker/email-details-interface.vala | 8 +- tests/tracker/emails-updates.vala | 8 +- tests/tracker/family-name-updates.vala | 8 +- tests/tracker/favourite-details-interface.vala | 8 +- tests/tracker/favourite-updates.vala | 8 +- tests/tracker/fullname-updates.vala | 8 +- tests/tracker/gender-details-interface.vala | 8 +- tests/tracker/given-name-updates.vala | 8 +- tests/tracker/im-details-interface.vala | 8 +- tests/tracker/imaddresses-updates.vala | 8 +- tests/tracker/individual-retrieval.vala | 8 +- tests/tracker/link-personas-via-local-ids.vala | 8 +- tests/tracker/link-personas.vala | 8 +- tests/tracker/match-all.vala | 8 +- tests/tracker/match-email-addresses.vala | 8 +- tests/tracker/match-im-addresses.vala | 8 +- tests/tracker/match-known-emails.vala | 8 +- tests/tracker/match-name.vala | 8 +- tests/tracker/match-phone-number.vala | 8 +- tests/tracker/name-details-interface.vala | 8 +- tests/tracker/nickname-updates.vala | 8 +- tests/tracker/note-details-interface.vala | 8 +- tests/tracker/phone-details-interface.vala | 8 +- tests/tracker/phones-updates.vala | 8 +- .../tracker/postal-address-details-interface.vala | 8 +- tests/tracker/prefix-name-updates.vala | 8 +- tests/tracker/remove-contact.vala | 8 +- tests/tracker/remove-persona.vala | 8 +- tests/tracker/role-details-interface.vala | 8 +- tests/tracker/set-avatar.vala | 8 +- tests/tracker/set-birthday.vala | 8 +- tests/tracker/set-duplicate-email.vala | 8 +- tests/tracker/set-emails.vala | 8 +- tests/tracker/set-favourite.vala | 8 +- tests/tracker/set-full-name.vala | 8 +- tests/tracker/set-gender.vala | 8 +- tests/tracker/set-im-addresses.vala | 8 +- tests/tracker/set-nickname.vala | 8 +- tests/tracker/set-notes.vala | 8 +- tests/tracker/set-null-avatar.vala | 8 +- tests/tracker/set-phones.vala | 8 +- tests/tracker/set-postal-addresses.vala | 8 +- tests/tracker/set-roles.vala | 8 +- tests/tracker/set-structured-name.vala | 8 +- tests/tracker/set-urls.vala | 8 +- tests/tracker/suffix-name-updates.vala | 8 +- tests/tracker/url-details-interface.vala | 8 +- tests/tracker/website-updates.vala | 8 +- 107 files changed, 1127 insertions(+), 889 deletions(-)
Fixed in git for 0.9.2. I'll open more bugs for any additional changes or test coverage.