Multiple load users

All tests pass, profile module implements the new hook_user_load(), profile_browse() uses multiple loading.

With 50 users on the site, profile module enabled but no fields added, the benchmarks show no change one way or the other on either /user/1 or /profile.

Marking to needs review in the hope of some independent benchmarks :) Will try to benchmark on a real database with some real profiles myself soonish.

without debug

Found a strange issue.

_menu_load_objects is sending along more than one argument to user_load(), which means it's being called with $reset = array('user', '1')- resulting in two full loads on user/1 instead of one. Not sure why that is.

Benchmarks are very inconclusive in any direction, however even with the above bug, devel shows a big drop on queries - user_load() hasn't got a static cache currently so there's a drop in queries there too.

Real profile and user tables from a d6 site - the site has 4176 users, 25300 rows in profile_value 29 rows in profile_field.

/profile after a couple of page refreshes, devel reports:

HEAD:
Executed 85 queries

Patch:
Executed 26 queries

On user/1
HEAD:
Executed 44 queries

Patch:
Executed 26 queries

Marking to needs work pending some kind of answer on that menu load issue. Attached patch does some small cleanup in profile.pages.inc to make use of the static better.

Re-rolled to use drupal_strtolower() instead of strcasecamp() per Damien, when filtering the static cache - which means we're multi-byte safe now. Although this is yet another reason to add a lowercase column to {users} and {term_data} since it could cause issues on obscure collations.

I left the existing parameters for user_load() apart from adding the reset argument - so where those calls are in the patch it's likely lines which are moved around rather than anything introduced. Whereas it made sense to enforce a single nid or vid on node_load, users have unique names, e-mail addresses and passwords, so there's no reason to remove that support here. I'd very much like to kill all these superfluous array('uid' => $uid) in core, but assuming we keep things roughly as they are, it doesn't need to be part of this patch - they're already superfluous.

Note that while I'd love to get some more reviews and/or benchmarks of this, there's still the outstanding menu bug to track down (which may or may not be a direct result of this patch), but leaving status as is for now.

Fixed a typo, added hook documentation (hook_user docs haven't been de-opped yet). So this is now blocked by the LOWER() issue and that menu weirdness with $map getting passed to user_load() as the second parameter.

Removing LOWER() entirely is over at #279851: Replace LOWER() with db_select() and LIKE() where possible - needs refreshing but still remains the cleanest way to deal with this without breaking either other dbms or static caching.

Now with patch.

There's already taxonomy_get_term_by_name() which will be a wrapper soon #343788: Taxonomy module doesn't use it's own APIs - and drupalGetNodeByTitle() in dwtc - which was again a wrapper because the tests (and almost nowhere else) used node_load(array('title' => 'foo)) so much. I think user_load_name() and user_load_mail() fit into this nicely enough. If I'm around later I'll try to get to that failing test.

drewish's changes look good, was still getting one fail in user admin tests - added a $reset=TRUE to the last user_load() and it's fine for me now. Also added $reset parameters to the _by_name and _by_mail functions. No other changes.

Re-rolled after some changes to user module.

Re-rolled for new db_placeholders replacement.

Re-rolled with two changes:

Killed that commented bit in dblog.test
Removed $conditions/static cache matching - so we can work on resolving the LOWER() issue elsewhere. This means if you use $conditions, then we don't get the advantage of the static cache - but user_load() doesn't have a static at all now, so we haven't lost anything.

The menu system calls user_load(1, $map) somewhere on user/n. Since $map is treated as TRUE this calls the reset, and means we get two full loads on that page instead of one. Haven't tracked down where that's happening yet. This makes it hard to benchmark individual user pages until that's fixed.

I've run benchmarks on /profile with a Drupal 6 database, but they were all within margin of error - so if someone else could do some with a bigger db (and a copy of d.o would be ideal), then that'd be great.

Coornail - thanks for the benchmarks, afaik the front page doesn't involve a call to user_load() though (or one at most) - would you mind rerunning the benchmarks on user/1 and /profile (with the profile module enabled)?

So while I couldn't get much differential in benchmarks before, kcachegrind is my new favourite toy. I did two requests each patched and unpatched to account for basic margin of error checking. The results are consistent enough to be useful I think.

This is on /profile with some devel generated users, with zero profile fields configured or populated - so pretty much vanilla HEAD. The values for each row are the 1st/2nd page request. All times are microseconds and are the sum of all time spent in the function during the request.

HEAD:
Total time spent in user_load() (called 22 times):
Incl: 59,874 / 51596
Self: 4199 / 3788

Patch:
Total time spent in microseconds spent in user_load_multiple (called twice):
Incl: 4971 / 5847
Self: 1024 / 1112

Time spent in PDOStatement->execute(). Self and incl are pretty much the same here since we're in native PHP anyway - PDOStatement->execute() is the most expensive function during the page request both patched and unpatched:
HEAD:
96 calls
36,383 / 26,348

Patch:
36 calls
12,319 / 12,751

On a user/n page (this calls user_load() which in turn calls user_load_multiple() - so we just take the incl for user_load in both cases as that's the like for like comparison. I think the very small speedup here ought to be due to the new static cache, although that's not being used to it's full extent due to the user_uid_optional_to_arg() issue from #3.

HEAD:
8 calls
Incl. 21233 / 21829

Patch:
9 calls
Incl. 16229 / 14397

So with a nearly empty database, there's a few milliseconds gained per request. I think we'll see user_load() get heavier once CCK fields can be attached directly as well.

test bot meltdown.

No longer applied after node/8 went in. This re-rolls the obvious stuff, but I'm getting 25 failures in the user_cancel() tests which I can't track down yet.

Now 15 and removed some notices found via manual testing (but strangely not picked up by simpletest - suggesting the environment and my install are behaving differently).

Tracked it down to the check for $hashed_password = user_pass_rehash() in user_cancel_confirm() - attached patch with this check commented out has 100% pass. Don't yet know why this is returning differently with and without multiple load though.

So the issue holding this one up is that $account->login in the user cancellation tests is used to fake the URL for a cancellation e-mail - and $account->login is different between the tested environment and the user object available to the tests - this means the results of user_rehash_pass() in both places are different, and results in failures. I'm not sure why this appears with the multiple load yet, removing the static caching doesn't help, moving a test into its own class doesn't help either.

Got this passing.

+      $users += $queried_users;
+      $user_cache += $queried_users;

Turns out to be different to:

+      $users = $users + $queried_users;
+      $user_cache = $user_cache + $queried_users;

So remaining todos are some fresh performance testing, and that call to user_load where $map gets passed as the second parameter on user/n. Back to CNR though.

Found the menu issue as well, user/%user/edit was being sent 'load arguments' with the full %map for no reason I can see. That's the only change in this patch. All tests should pass, and it makes benchmarking viable as well.

Benchmarks on user/1 (anonymous users given access to user profiles): ab -c1 -n1000 + devel query log summary.

HEAD:
Requests per second: 12.56 [#/sec] (mean)
Executed 55 queries in 19.46 milliseconds.

Patch:
Requests per second: 13.64 [#/sec] (mean)
Executed 26 queries in 15.74 milliseconds.

And /profile - no profile fields or user pictures on any of the users - obviously we'd see a bigger improvement when those were present.

HEAD:
Requests per second: 10.26 [#/sec] (mean)
Executed 90 queries in 33.77 milliseconds.

Patch:
Requests per second: 10.74 [#/sec] (mean)
Executed 29 queries in 15.81 milliseconds.

So user/n is just under 10% faster due to the introduction of the static - there's currently none - which means user_load and file_load issue queries multiple times on that page. And /profile with no fields or user pictures is 5% faster.

Renamed $user_pictures to $picture_fids following discussion with drewish in irc. Also only query for user pictures if the variable is set.

somehow the commented out user_pass_rehash() in function user_cancel() crept back in while re-rolling, which is why the tests pass..

So, fixing those tests failures turns out to be a complete pain in the arse.

Here's a pastebin of what happens - basically the $account in simpletest itself, and a straight db_query() of the users table in the tested site are different. With the static cache, user->login is 0 inside simpletest and after a couple of page requests the hashed password changes. Without the static, just the hashed password changes inside simpletest. In both cases, 14 fails.

Here's a patch just to keep up with HEAD in case anyone fancies debugging this, since I've pretty much given up for now.

edit: link http://drupalbin.com/8026

This one addresses the issues in #48. With the reset, it's only called where we're specifically recreating a fresh user object inside user_save() (which obviously happens in a single request), and in the test - so shouldn't be any more queries than necessary, none anywhere critical, and of course we do all those queries now already. Still 14 fails with this patch.

Berdir, thank you very, very much. I probably won't get a chance to look at this until at least later this evening, but my debugging had come to a complete dead end - looks like $edit = get_object_vars($account) was the bit I missed.

Note that the extra $reset = TRUE are only added in the tests (and in user_save()) otherwise the static is pretty much self-contained.

There shouldn't be much more to do here - would be great if someone could give it a final look over.

Re-rolled.

Berdir's patch from #53 is still good after the user/users revert. Re-uploaded.

hmm.

Thanks! Documented here http://drupal.org/node/224333#user_load_multiple

Thanks yched, patched.

Re-roll just uses the new style placeholders.

Sorry for double breakage, thanks for writing those OpenID tests c960657!