in user_relationships load the first line of the function declares
static $relationships = array();
This suggests that the author intends to cache query results so the same query doesn't get run multiple times on a singe page load. This is a noble intention but it seems that the code doesn't deliver.
As you will notice in the attached screenshot the query was run 20 times in this page load, and all previous page loads as well.
In the function the data in $relationships
is used if the first argument $param
is an integer, but if it is not the database is queried and the results are not stored back in $relationships
for further use. In addition later in the function, the $relationships
array is reset so even if there were some data stored there it is lost.
Comment | File | Size | Author |
---|---|---|---|
#2 | 9e3c8e4c2a3f97b116ce71517e03431c.png | 22.86 KB | jrglasgow |
96f53aeccd45dfcd782dcc68df7d553a.png | 21.55 KB | jrglasgow |
Comments
Comment #1
alex.k CreditAttribution: alex.k commentedA couple of comments:
1. Thanks for pointing this out, that's a strange piece of logic.
2. Resetting is the right thing to do, because the database is about to be queried next, and you don't want results of the previous query in $relationships.
3. Looking through the code, the only way caching will help is if someone needs to go back and query specific relationship objects after they've queried for some set. This was written before I got this module so I"m not sure of exact use case.
In any case, I'm not sure that caching is even the right thing to do. If someone queries and gets 10,000 relationships, you don't necessarily want those hanging around in memory for the duration of the request. It's much better to see what code repeatedly calls user_relationships_load with the same arguments and eliminate that bottleneck. Your screenshot is showing something else so I don't know where that spot is. Thoughts?
Comment #2
jrglasgow CreditAttribution: jrglasgow commentedyou are right, I attached the wrong screenshot
Comment #3
alex.k CreditAttribution: alex.k commentedThe query is trying to load relationships for uid=0. You need to see where this call originates, as that really doesn't make sense.
Comment #4
mstef CreditAttribution: mstef commentedI also noticed something a little strange about the caching logic..
If the relationships are cached.. they're returned like:
If they're not cached, they are altered via:
Meaning, if this function is called more than once on execution, and an alter hook is altering on $op == 'load', the returned results will be different.
Comment #5
mrf CreditAttribution: mrf commentedMoving this over to a bug report so it doesn't get lost in the task queue.
Comment #6
BerdirThis also affects 7.x I think the code is the same AFAIK.
Comment #7
mrf CreditAttribution: mrf commentedI think this would be better to tackle in 7.x then push it back to 6 if its a simple enough change. Wouldn't be too upset if this kind of optimization didn't ever make it in to 6 unless someone that needs it pushes it through.
Comment #8
drewish CreditAttribution: drewish commentedIt seems like we should drop the static variable and move to drupal_static() instead.
Comment #9
drewish CreditAttribution: drewish commentedI created #1765664: Switch static variable in user_relationships_types_load() to drupal_static() to deal with that static.
Comment #10
Andre-Bas #1765664: Switch static variable in user_relationships_types_load() to drupal_static() got commited I close this one as fixed