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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alex.k’s picture

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?

jrglasgow’s picture

you are right, I attached the wrong screenshot

alex.k’s picture

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.

mstef’s picture

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.

mrf’s picture

Category: task » bug

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

Berdir’s picture

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

mrf’s picture

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.

drewish’s picture

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

drewish’s picture

Andre-B’s picture

Status: Active » Fixed

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

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