in user_relationships load the first line of the function declares

<?php
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.

Comments

A 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?

StatusFileSize
new22.86 KB

you are right, I attached the wrong screenshot

The query is trying to load relationships for uid=0. You need to see where this call originates, as that really doesn't make sense.

I also noticed something a little strange about the caching logic..

If the relationships are cached.. they're returned like:

$arguments = array();
  if (is_numeric($param)) {
    if (!$reset && isset($relationships[$param])) {
      return is_object($relationships[$param]) ? drupal_clone($relationships[$param]) : $relationships[$param];
    }

If they're not cached, they are altered via:

$return = isset($rid) ? $relationships[$rid] : $relationships;
  _user_relationships_invoke('load', $return);
  return $return;

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.

Category:task» bug

Moving this over to a bug report so it doesn't get lost in the task queue.

This also affects 7.x I think the code is the same AFAIK.

Version:6.x-1.x-dev» 7.x-1.x-dev

I 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.

It seems like we should drop the static variable and move to drupal_static() instead.

Status:Active» Fixed

Status:Fixed» Closed (fixed)
Issue tags:-caching

Automatically closed -- issue fixed for 2 weeks with no activity.