[folks] Mostly avoid an expensive multi-map iteration pattern
- From: Simon McVittie <smcv src gnome org>
- To: commits-list gnome org
- Cc:
- Subject: [folks] Mostly avoid an expensive multi-map iteration pattern
- Date: Fri, 8 Mar 2013 14:04:25 +0000 (UTC)
commit 3ead592946f3b9e9b41196c193228047af9b0834
Author: Simon McVittie <simon mcvittie collabora co uk>
Date: Fri Mar 8 13:55:29 2013 +0000
Mostly avoid an expensive multi-map iteration pattern
If you have a MultiMap with, say, 100 keys each with 1 value, and you
iterate over it like this (pseudocode):
for key in keys():
values = get(key)
for value in values:
do something(key, value)
then you have constructed one GObject for the result of keys(), and
100 GObjects for the results of get() (because it returns a read-only
view). If you iterate it like this:
iter = map_iterator()
while iter.next():
do something(iter.key(), iter.value())
there's only one extraneous GObject, the iterator itself.
When there are thousands of contacts, as in add-contacts-stress-test,
this really starts to matter.
This patch doesn't fix every use of get_keys() - some of them do
non-trivial work per key as well as per value, making it awkward to
convert to map_iterator() - but it's a start. It cuts 'user' CPU time
for IndividualAggregator quiescence for an e-d-s Google account with
2049 contacts (reading from cache) by around 3%.
Bug: https://bugzilla.gnome.org/show_bug.cgi?id=682903
Reviewed-by: Philip Withnall <philip tecnocode co uk>
[note added to HACKING as per Philip's review]
Signed-off-by: Simon McVittie <simon mcvittie collabora co uk>
HACKING | 4 +
backends/eds/lib/edsf-persona.vala | 19 +--
backends/key-file/kf-persona.vala | 19 +--
.../telepathy/lib/tpf-persona-store-cache.vala | 27 ++--
backends/telepathy/lib/tpf-persona-store.vala | 13 +-
folks/abstract-field-details.vala | 12 +-
folks/individual-aggregator.vala | 169 +++++++++++---------
folks/individual.vala | 32 +---
8 files changed, 140 insertions(+), 155 deletions(-)
---
diff --git a/HACKING b/HACKING
index c031cfd..0ff14f3 100644
--- a/HACKING
+++ b/HACKING
@@ -124,6 +124,10 @@ Vala-specific rules
than a specific constructor. This means that the initialisation doesn’t have
to be copied between multiple alternate constructors.
+14. When iterating over a MultiMap, try to use the map_iterator().
+ This is more efficient than iterating over the result of get_keys(),
+ then calling get() separately for each key.
+
Build health
============
diff --git a/backends/eds/lib/edsf-persona.vala b/backends/eds/lib/edsf-persona.vala
index f6c02b2..294c23a 100644
--- a/backends/eds/lib/edsf-persona.vala
+++ b/backends/eds/lib/edsf-persona.vala
@@ -1010,13 +1010,10 @@ public class Edsf.Persona : Folks.Persona,
{
if (prop_name == "im-addresses")
{
- foreach (var protocol in this._im_addresses.get_keys ())
- {
- var im_fds = this._im_addresses.get (protocol);
+ var iter = this._im_addresses.map_iterator ();
- foreach (var im_fd in im_fds)
- callback (protocol + ":" + im_fd.value);
- }
+ while (iter.next ())
+ callback (iter.get_key () + ":" + iter.get_value ().value);
}
else if (prop_name == "local-ids")
{
@@ -1031,14 +1028,10 @@ public class Edsf.Persona : Folks.Persona,
}
else if (prop_name == "web-service-addresses")
{
- foreach (var web_service in this.web_service_addresses.get_keys ())
- {
- var web_service_addresses =
- this._web_service_addresses.get (web_service);
+ var iter = this.web_service_addresses.map_iterator ();
- foreach (var ws_fd in web_service_addresses)
- callback (web_service + ":" + ws_fd.value);
- }
+ while (iter.next ())
+ callback (iter.get_key () + ":" + iter.get_value ().value);
}
else if (prop_name == "email-addresses")
{
diff --git a/backends/key-file/kf-persona.vala b/backends/key-file/kf-persona.vala
index e9f742e..950bb91 100644
--- a/backends/key-file/kf-persona.vala
+++ b/backends/key-file/kf-persona.vala
@@ -440,24 +440,17 @@ public class Folks.Backends.Kf.Persona : Folks.Persona,
{
if (prop_name == "im-addresses")
{
- foreach (var protocol in this._im_addresses.get_keys ())
- {
- var im_addresses = this._im_addresses.get (protocol);
+ var iter = this._im_addresses.map_iterator ();
- foreach (var im_fd in im_addresses)
- callback (protocol + ":" + im_fd.value);
- }
+ while (iter.next ())
+ callback (iter.get_key () + ":" + iter.get_value ().value);
}
else if (prop_name == "web-service-addresses")
{
- foreach (var web_service in this.web_service_addresses.get_keys ())
- {
- var web_service_addresses =
- this._web_service_addresses.get (web_service);
+ var iter = this.web_service_addresses.map_iterator ();
- foreach (var ws_fd in web_service_addresses)
- callback (web_service + ":" + ws_fd.value);
- }
+ while (iter.next ())
+ callback (iter.get_key () + ":" + iter.get_value ().value);
}
else
{
diff --git a/backends/telepathy/lib/tpf-persona-store-cache.vala
b/backends/telepathy/lib/tpf-persona-store-cache.vala
index 4556786..720214e 100644
--- a/backends/telepathy/lib/tpf-persona-store-cache.vala
+++ b/backends/telepathy/lib/tpf-persona-store-cache.vala
@@ -148,16 +148,14 @@ internal class Tpf.PersonaStoreCache : Folks.ObjectCache<Tpf.Persona>
Variant[] parameters = new Variant[afd.parameters.size];
uint f = 0;
- foreach (var key in afd.parameters.get_keys ())
- {
- foreach (var val in afd.parameters.get (key))
- {
- parameters[f++] = new Variant.tuple ({
- new Variant.string (key), // Key
- new Variant.string (val) // Value
- });
- }
- }
+
+ var iter = afd.parameters.map_iterator ();
+
+ while (iter.next ())
+ parameters[f++] = new Variant.tuple ({
+ new Variant.string (iter.get_key ()),
+ new Variant.string (iter.get_value ())
+ });
output_variants[i++] = new Variant.tuple ({
afd.value, // Variant value (e.g. e-mail address)
@@ -185,11 +183,10 @@ internal class Tpf.PersonaStoreCache : Folks.ObjectCache<Tpf.Persona>
// Sort out the IM addresses (there's guaranteed to only be one)
string? im_protocol = null;
- foreach (var protocol in persona.im_addresses.get_keys ())
- {
- im_protocol = protocol;
- break;
- }
+ var iter = persona.im_addresses.map_iterator ();
+
+ if (iter.next ())
+ im_protocol = iter.get_key ();
// Avatar
var avatar_file = (persona.avatar != null && persona.avatar is FileIcon) ?
diff --git a/backends/telepathy/lib/tpf-persona-store.vala b/backends/telepathy/lib/tpf-persona-store.vala
index 0c6c5d1..ff5897d 100644
--- a/backends/telepathy/lib/tpf-persona-store.vala
+++ b/backends/telepathy/lib/tpf-persona-store.vala
@@ -1485,13 +1485,14 @@ public class Tpf.PersonaStore : Folks.PersonaStore
string[] values = { afd.value };
string[] parameters = {};
- foreach (var param_name in afd.parameters.get_keys ())
+ var iter = afd.parameters.map_iterator ();
+
+ while (iter.next ())
{
- var param_values = afd.parameters[param_name];
- foreach (var param_value in param_values)
- {
- parameters += @"$param_name=$param_value";
- }
+ var param_name = iter.get_key ();
+ var param_value = iter.get_value ();
+
+ parameters += @"$param_name=$param_value";
}
if (parameters.length == 0)
diff --git a/folks/abstract-field-details.vala b/folks/abstract-field-details.vala
index eab0c71..00a0c51 100644
--- a/folks/abstract-field-details.vala
+++ b/folks/abstract-field-details.vala
@@ -220,14 +220,10 @@ public abstract class Folks.AbstractFieldDetails<T> : Object
*/
public void extend_parameters (MultiMap<string, string> additional)
{
- foreach (var name in additional.get_keys ())
- {
- var values = additional.get (name);
- foreach (var val in values)
- {
- this.add_parameter (name, val);
- }
- }
+ var iter = additional.map_iterator ();
+
+ while (iter.next ())
+ this.add_parameter (iter.get_key (), iter.get_value ());
}
/**
diff --git a/folks/individual-aggregator.vala b/folks/individual-aggregator.vala
index d8cdaed..e3fb81e 100644
--- a/folks/individual-aggregator.vala
+++ b/folks/individual-aggregator.vala
@@ -517,16 +517,30 @@ public class Folks.IndividualAggregator : Object
this._link_map.size);
debug.indent ();
- foreach (var link_key in this._link_map.get_keys ())
+ string? last_key = null;
+ var iter = this._link_map.map_iterator ();
+
+ while (iter.next ())
{
- debug.print_line (domain, level, "%s → {", link_key);
- debug.indent ();
+ var link_key = iter.get_key ();
- foreach (var individual in this._link_map.get (link_key))
+ if (last_key == null || link_key != (!) last_key)
{
- debug.print_line (domain, level, "%p", individual);
+ if (last_key != null)
+ {
+ debug.unindent ();
+ debug.print_line (domain, level, "}");
+ }
+
+ debug.print_line (domain, level, "%s → {", link_key);
+ debug.indent ();
}
+ debug.print_line (domain, level, "%p", iter.get_value ());
+ }
+
+ if (last_key != null)
+ {
debug.unindent ();
debug.print_line (domain, level, "}");
}
@@ -1020,33 +1034,35 @@ public class Folks.IndividualAggregator : Object
debug ("Emitting individuals-changed-detailed with %u mappings:",
_changes.size);
- foreach (var removed_ind in _changes.get_keys ())
+ var iter = _changes.map_iterator ();
+
+ while (iter.next ())
{
- foreach (var added_ind in _changes.get (removed_ind))
+ var removed_ind = iter.get_key ();
+ var added_ind = iter.get_value ();
+
+ debug (" %s (%p) → %s (%p)",
+ (removed_ind != null) ? ((!) removed_ind).id : "",
+ removed_ind,
+ (added_ind != null) ? ((!) added_ind).id : "", added_ind);
+
+ if (removed_ind != null)
{
- debug (" %s (%p) → %s (%p)",
- (removed_ind != null) ? ((!) removed_ind).id : "",
- removed_ind,
- (added_ind != null) ? ((!) added_ind).id : "", added_ind);
+ debug (" Removed individual's personas:");
- if (removed_ind != null)
+ foreach (var p in ((!) removed_ind).personas)
{
- debug (" Removed individual's personas:");
-
- foreach (var p in ((!) removed_ind).personas)
- {
- debug (" %s (%p)", p.uid, p);
- }
+ debug (" %s (%p)", p.uid, p);
}
+ }
- if (added_ind != null)
- {
- debug (" Added individual's personas:");
+ if (added_ind != null)
+ {
+ debug (" Added individual's personas:");
- foreach (var p in ((!) added_ind).personas)
- {
- debug (" %s (%p)", p.uid, p);
- }
+ foreach (var p in ((!) added_ind).personas)
+ {
+ debug (" %s (%p)", p.uid, p);
}
}
}
@@ -1219,11 +1235,13 @@ public class Folks.IndividualAggregator : Object
* iterating over it. */
var transitive_updates = new HashSet<Individual?> ();
- foreach (var k in individuals_changes.get_keys ())
+ var iter = individuals_changes.map_iterator ();
+
+ while (iter.next ())
{
- if (i in individuals_changes.get (k))
+ if (i == iter.get_value ())
{
- transitive_updates.add (k);
+ transitive_updates.add (iter.get_key ());
}
}
@@ -1396,10 +1414,14 @@ public class Folks.IndividualAggregator : Object
* there's no iterator object. FIXME: bgo#675067 */
var remove_keys = new string[0];
- foreach (var link_key in this._link_map.get_keys ())
+ var iter = this._link_map.map_iterator ();
+
+ while (iter.next ())
{
- if (this._link_map.get (link_key).contains (individual))
+ if (iter.get_value () == individual)
{
+ var link_key = iter.get_key ();
+
debug (" %s → %s (%p)",
link_key, individual.id, individual);
@@ -1550,27 +1572,29 @@ public class Folks.IndividualAggregator : Object
/* Extract the deprecated added and removed sets from
* individuals_changes, to be used in the individuals_changed
* signal. */
- foreach (var old_ind in individuals_changes.get_keys ())
+ var iter = individuals_changes.map_iterator ();
+
+ while (iter.next ())
{
- foreach (var new_ind in individuals_changes.get (old_ind))
- {
- assert (old_ind != null || new_ind != null);
+ var old_ind = iter.get_key ();
+ var new_ind = iter.get_value ();
- if (old_ind != null)
- {
- removed_individuals.add ((!) old_ind);
- }
+ assert (old_ind != null || new_ind != null);
- if (new_ind != null)
- {
- added_individuals.add ((!) new_ind);
- this._connect_to_individual ((!) new_ind);
- }
+ if (old_ind != null)
+ {
+ removed_individuals.add ((!) old_ind);
+ }
- if (old_ind != null && new_ind != null)
- {
- replaced_individuals.set ((!) old_ind, (!) new_ind);
- }
+ if (new_ind != null)
+ {
+ added_individuals.add ((!) new_ind);
+ this._connect_to_individual ((!) new_ind);
+ }
+
+ if (old_ind != null && new_ind != null)
+ {
+ replaced_individuals.set ((!) old_ind, (!) new_ind);
}
}
@@ -1596,21 +1620,24 @@ public class Folks.IndividualAggregator : Object
/* Validate the link map. */
if (this._debug.debug_output_enabled == true)
{
- foreach (var link_key in this._link_map.get_keys ())
+ var link_map_iter = this._link_map.map_iterator ();
+
+ while (link_map_iter.next ())
{
- foreach (var individual in this._link_map.get (link_key))
+ var individual = link_map_iter.get_value ();
+
+ if (this._individuals.get (individual.id) != individual)
{
- if (this._individuals.get (individual.id) != individual)
+ var link_key = link_map_iter.get_key ();
+
+ warning ("Link map contains invalid mapping:\n" +
+ " %s → %s (%p)",
+ link_key, individual.id, individual);
+ warning ("Individual %s (%p) personas:", individual.id,
+ individual);
+ foreach (var p in individual.personas)
{
- warning ("Link map contains invalid mapping:\n" +
- " %s → %s (%p)",
- link_key, individual.id, individual);
- warning ("Individual %s (%p) personas:", individual.id,
- individual);
- foreach (var p in individual.personas)
- {
- warning (" %s (%p)", p.uid, p);
- }
+ warning (" %s (%p)", p.uid, p);
}
}
}
@@ -1998,33 +2025,21 @@ public class Folks.IndividualAggregator : Object
if (persona is ImDetails)
{
ImDetails im_details = (ImDetails) persona;
+ var iter = im_details.im_addresses.map_iterator ();
/* protocols_addrs_set = union (all personas' IM addresses) */
- foreach (var protocol in im_details.im_addresses.get_keys ())
- {
- var im_addresses = im_details.im_addresses.get (protocol);
-
- foreach (var im_address in im_addresses)
- {
- protocols_addrs_set.set (protocol, im_address);
- }
- }
+ while (iter.next ())
+ protocols_addrs_set.set (iter.get_key (), iter.get_value ());
}
if (persona is WebServiceDetails)
{
WebServiceDetails ws_details = (WebServiceDetails) persona;
+ var iter = ws_details.web_service_addresses.map_iterator ();
/* web_service_addrs_set = union (all personas' WS addresses) */
- foreach (var web_service in
- ws_details.web_service_addresses.get_keys ())
- {
- var ws_addresses =
- ws_details.web_service_addresses.get (web_service);
-
- foreach (var ws_fd in ws_addresses)
- web_service_addrs_set.set (web_service, ws_fd);
- }
+ while (iter.next ())
+ web_service_addrs_set.set (iter.get_key (), iter.get_value ());
}
if (persona is LocalIdDetails)
diff --git a/folks/individual.vala b/folks/individual.vala
index a6b8e6c..ac373e0 100644
--- a/folks/individual.vala
+++ b/folks/individual.vala
@@ -1754,17 +1754,11 @@ public class Folks.Individual : Object,
var im_details = persona as ImDetails;
if (im_details != null)
{
- foreach (var cur_protocol in
- im_details.im_addresses.get_keys ())
- {
- var cur_addresses =
- im_details.im_addresses.get (cur_protocol);
+ var iter = im_details.im_addresses.map_iterator ();
- foreach (var address in cur_addresses)
- {
- new_im_addresses.set (cur_protocol, address);
- }
- }
+ while (iter.next ())
+ new_im_addresses.set (iter.get_key (),
+ iter.get_value ());
}
}
@@ -1804,19 +1798,11 @@ public class Folks.Individual : Object,
var web_service_details = persona as WebServiceDetails;
if (web_service_details != null)
{
- foreach (var cur_web_service in
- web_service_details.web_service_addresses.get_keys ())
- {
- var cur_addresses =
- web_service_details.web_service_addresses.get (
- cur_web_service);
+ var iter = web_service_details.web_service_addresses.map_iterator ();
- foreach (var ws_fd in cur_addresses)
- {
- new_web_service_addresses.set (cur_web_service,
- ws_fd);
- }
- }
+ while (iter.next ())
+ new_web_service_addresses.set (iter.get_key (),
+ iter.get_value ());
}
}
[
Date Prev][
Date Next] [
Thread Prev][
Thread Next]
[
Thread Index]
[
Date Index]
[
Author Index]
