Using the devel module, I saw that on some pages displaying a lot of content (and that's even worst using views) that when the same node author is being displayed more than once, the SQL request to load the user instance is being done as many times as the user name is displayed. There is something even worst is that the hook_user() is ran as many times as the SQL request is being done.
I did a small performance fix patch, using a simple static cache, that does not seems (yet) to break any known behavior, and works in most cases since it caches any user loaded with as key its uid and name, and avoid the same SQL request to be done twice if a module attempts to load this same user instance using either the name or the uid (which reprensents most of the cases a module would load a user).
I think that in order to avoid any unwanted behaviors that the objects could be cloned at return time, maybe not. It's currently working good on more than one custom site.
| Comment | File | Size | Author |
|---|---|---|---|
| #5 | user.module-6.19-user_load_static_cache.patch | 2.11 KB | pounard |
| #2 | user.module-6.19-user_load_static_cache.patch | 2.08 KB | pounard |
| #1 | user.module-6.19-user_load_static_cache.patch | 1.74 KB | pounard |
| user.module-6.19-user_load_static_cache.patch | 1.74 KB | pounard |
Comments
Comment #1
pounardRe done the patch using CVS diff.
EDIT: Oups, wrong one attached.
Comment #2
pounardRight patch.
Comment #4
pounardOkay, I did exactly what is said on the http://drupal.org/patch/create page, what's wrong ?
Comment #5
pounardNew attempt.
Comment #6
jide commentedChanging status for the bot to test the patch.
Comment #7
damien tournoud commentedThis is a duplicate of #281596: Performance: static caching of user objects in user_load(), respecting conditions, that was closed without good reason.
This is actually a significant API change, as some code might expect to get a fully refreshed object from a call to user_load(). At the minimum, we need to clone the returned object, and we might need to go as far as defaulting to non-cached.
Comment #8
pounardIn fact, there is no API change at all if we clone the object, the behavior will remain exactly the same. I think that the KISS principle can totally fit here, the patch is simple, and only provide a performance fix for simple cases (load by uid or name). I would agree on the fact that this is not the best solution out there, but at least it will gain (a lot) of useless SQL queries in a lot of cases. With the clone() added it won't change the function behavior, except maybe some missing hook_user() calls. But if any modules uses this hook for anything else than populating the object then they do a wrong usage of it.
Comment #9
pounardThe only problem we could have using this method is what you said: http://drupal.org/node/281596#comment-3473436 , I agree this is totally true. But if we look at it, I don't really see where it could break (on the many sites I did work with, user structure is not something that many module alter in another context than a form submit).
Comment #10
damien tournoud commentedIf you look at the more complete patch in #281596-24: Performance: static caching of user objects in user_load(), respecting conditions you will see how much this is an API change. Some code (even in core) assume that the user object is fresh when loaded from user_load().
Comment #11
pounardYep, figured that out. I would still propose the patch for people that would want to apply it.
This patch could be totally optional, it may live as a feature activated by a some variable, disabled per default, so only power user and wise site admins would change that.