Remove buddylist_get_buddies from hook_user load $op
| Project: | Buddylist |
| Version: | 5.x-1.x-dev |
| Component: | Code |
| Category: | task |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | needs review |
Jump to:
Buddylist is unbelievably taxing in a situation where buddylist_get_buddies() is called in a large social network. Simply put, eventually buddylist_get_buddies() effectively loads all the users in the entire extended network a single user. We have a site where a single call to buddylist_get_buddies() generates over 20,000 queries!! The user had about 600 buddies, and the result was crippling.
Here's a rundown of what happens:
- A single user is loaded via user_load().
- Buddylist's hook_user calls buddylist_get_buddies() to set the return to $user->buddies
- In buddylist_get_buddies(), every user that is a buddy of the original user is loaded.
- Buddylist's hook_user calls buddylist_get_buddies() to set the return to the buddy's $user->buddies
- Cycle continues until every user that is a buddy of any user is loaded.
So this is a minor fix that gives a drastic improvement in performance. This convenience of having $user->buddies simply isn't feasible. Especially considering buddylist already provides handy functions like buddylist_get_buddylist() that make the need for these users to be there all the time unnecessary.
| Attachment | Size |
|---|---|
| buddylist_remove_user_load.patch | 928 bytes |
#1
I see the issue here, and this clearly has to get solved for this module to scale.
However, tearing it out will likely break a lot of sites that depend on it for simple functionality, so this change would likely need to be acoompanieid by a branch and some documentation.
I'd like to be able to make this change. Perhaps as part of a new branch to begin working towards a better/stronger/faster drupal 6 version?
#2
A new branch sounds optimal for this change, a clear API breakage. We definitely should also start a changelog, and begin following these changes between 1.x and 2.x.
#3
Hi Gang,
Sorry I've dropped of the buddylist planet for quite awhile - I've seen this issue before and I removed that code from my own local copy. What about making this a variable_get() driven behavior that way it can be disabled yet preserve the API?
I'm mostly commening because I see mention of doing a Drupal 6 version - has something changed with buddylist 2 - I was sort of under the impression this version of the module was going to fade away. If plans are being made to do a new version of this code for 6 I'd be interested and I'd suggest a complete rewrite. This code base is a mess and could be way better and simpler.
Cheers!
#4
Following up better late than never, I actually don't see this. There's no user_load in buddylist_get_buddies(), which would be the cause of such recursion:
<?php
function buddylist_get_buddies($uid = NULL, $key = 'uid') {
static $buddies;
if (!$uid) {
global $user;
$uid = $user->uid;
}
if (!isset($buddies[$key][$uid])) {
$buddies[$key][$uid] = array();
$sql = 'SELECT b.buddy, u.name, u.mail, u.uid FROM {buddylist} b
INNER JOIN {users} u ON b.buddy = u.uid
WHERE b.uid = %d';
$result = db_query($sql, $uid);
while ($row = db_fetch_object($result)) {
$buddies[$key][$uid][$row->buddy]['uid'] = $row->uid;
$buddies[$key][$uid][$row->buddy]['name'] = $row->name;
$buddies[$key][$uid][$row->buddy]['mail'] = $row->mail;
if (variable_get('buddylist_buddygroups', FALSE)) {
$buddies[$key][$uid][$row->buddy]['groups'] = buddylist_get_buddy_groups($uid, $row->buddy);
}
$buddies[$key][$uid][$row->buddy]['online'] = 0;
$selectlist .= $row->buddy.",";
}
// Add the online flag
if (db_num_rows($result)) {
$sql = 'SELECT uid FROM {sessions} WHERE uid IN (%s) AND timestamp > %d';
$result = db_query($sql, substr($selectlist,0,-1), time()-1800);
while ($row = db_fetch_object($result)) {
$buddies[$key][$uid][$row->uid]['online'] = 1;
}
}
}
return $buddies[$key][$uid];
}
?>
I'll keep an eye on this as we work towards 6.0 though. Please update me if I'm wrong here.
#5
Oh... looks like I've misdirected my hostility. buddylist_get_buddylist() is the function that contains a user_load(), not buddylist_get_buddies(). Hmm, I'm not sure there's a problem here after all.
On our particular site, our User #1 was automatically friending all new users (similar to a mySpace scenario). What we were seeing was the individual queries caused by the buddy-groups option (which does an individual SQL query per user in the list during buddylist_get_buddies()).
Well, I don't think this is the problem I thought it was. Simultaneously, I still feel it's wasted queries to call buddylist_get_buddies on every user_load(). The function is easy enough I can call it manually whenever needed. So I'd still vouch for including this patch, even though it's not the nightmare that I thought it was.
#6
This is definitely an issue. On our site, there was no way to keep it live without doing this:
function buddylist_user($type, &$edit, &$thisuser, $category = NULL) {
....
else if ($type == 'load') {
/** @logomod, LOGOT-349: Kill the module invoke all hook_user load cycle for the buddylist module so a user's buddies aren't loaded every time the user is loaded.
$thisuser->buddies = buddylist_get_buddies($thisuser->uid);
*/
}
...
The things is that for every buddy of user A, each buddy triggers the hook _user with $type 'load', which means that for each and every buddy of user A, all the buddies of user A's buddies also get loaded. There's a non-linear collapse of scalability here.
Huge +1 from me.