GNOME Bugzilla – Bug 688834
getting properties creates data structures over and over again
Last modified: 2012-11-24 14:33:31 UTC
My expectation is that once all properties of a FolksIndividual have been retrieved (and thus properties with delayed loading are loaded), and further access just returns pointers to existing data structures directly. What I saw instead in in "perf" output is that searching the loaded and stable FolksIndividuals spends most of its time with creating Gee data structures. For example, this happens in folks_individual_real_get_phone_numbers(). The reason is the obligatory update_phone_numbers: [CCode (notify = false)] public Set<PhoneFieldDetails> phone_numbers { get { => this._update_phone_numbers (true, false); return this._phone_numbers_ro; } _update_phone_numbers invokes the MultiValuedPropertySetter: private void _update_phone_numbers (bool create_if_not_exist, bool emit_notification = true) { this._update_multi_valued_property ("phone-numbers", create_if_not_exist, () => { return this._phone_numbers == null; }, [...] var new_phone_numbers = new HashSet<PhoneFieldDetails> ( (GLib.HashFunc) PhoneFieldDetails.hash, (GLib.EqualFunc) PhoneFieldDetails.equal); var phone_numbers_set = new HashMap<string, PhoneFieldDetails> ( null, null, (GLib.EqualFunc) PhoneFieldDetails.equal); [...] if (!Utils.set_afd_equal (new_phone_numbers, this._phone_numbers)) [...] All of that is not cheap. Is it possible to avoid the update check?
From IRC: pwithnall: pohly: That _update_*() method is a lazy loader. It should do no work the second time it's called pwithnall: Take a look at the implementation of _update_multi_valued_property() That does not match the definition (and implementation) of _update_multi_valued_property(). Its description explicitly says (emphasis mine): * If the property value is to be instantiated, ***or already has been * instantiated***, its value is updated by ``setter`` from the values of the * property in the individual's personas. And the code: if (prop_is_null ()) { ... } /* Re-populate the collection as the union of the values in the * individual's personas. */ if (setter () == true && emit_notification) ... So what is right - the implementation or the intended behavior of not doing any work when the property already exists?
(In reply to comment #1) > So what is right - the implementation or the intended behavior of not doing any > work when the property already exists? Unless I’m forgetting something, the intended behaviour is right. Properties in an individual should only be updated when notifications are received from the individual’s personas. A missing notification is a bug. So we have: • When called from a getter, _update_multi_valued_property() should create and populate the collection if it doesn’t exist, but should not do anything if it already exists. No notifications should be emitted. • When called from a notifier or from _update_fields(), _update_multi_valued_property() should update the collection if it already exists (and emit a notification if the collection changes as a result), but only emit a notification if the collection doesn’t exist. Patches welcome.
Created attachment 229726 [details] [review] avoid redundant updating in get()
Review of attachment 229726 [details] [review]: Looks good to me. Pushed to master with a suitable NEWS entry. Thanks! commit 90b514025017db3465bdce60ca80f56927743aa1 Author: Patrick Ohly <patrick.ohly@intel.com> Date: Fri Nov 23 15:06:58 2012 +0100 individual: avoid updating in get Properties with delayed loading always went through an update check each time the property was read. This slowed down reading unnecessarily, because constructing the value is costly and necessary updates are already dealt with via notifications. Fixed by introducing a new parameter "force_update" which is true by default (= same behavior as before) and false when used inside get(). Closes: https://bugzilla.gnome.org/show_bug.cgi?id=688834 NEWS | 1 + folks/individual.vala | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++-------------------------------- 2 files changed, 51 insertions(+), 32 deletions(-)