WordPress.org

Make WordPress Core

Opened 22 months ago

Closed 20 months ago

Last modified 3 months ago

#21120 closed enhancement (fixed)

Optimize get_user_by()

Reported by: kurtpayne Owned by:
Milestone: 3.5 Priority: normal
Severity: normal Version:
Component: Performance Keywords: dev-feedback has-patch
Focuses: Cc:

Description

The get_user_by() function is used repeatedly on the "All Posts" page.

I used the theme unit test sample posts to test this, and there were only 4 unique calls to get_user_by() but it was called 264 times to generate the first page of "All Posts" (20 posts of 25 total).

When viewing 26 posts, this function is called 264 times. Each time, a new WP_User object is created and WP_User::init() is called.

Attachments (11)

21120.diff (671 bytes) - added by kurtpayne 22 months ago.
Proof of concept static cache
get_user_by_grind.png (23.7 KB) - added by kurtpayne 22 months ago.
Before and after static cache
21120.2.patch (497 bytes) - added by kurtpayne 22 months ago.
Use wp_cache_get and wp_cache_set
21120.3.patch (2.9 KB) - added by kurtpayne 22 months ago.
Adding an "update_author_cache" query var to WP_Query
map_meta_cap_calls.png (19.7 KB) - added by kurtpayne 22 months ago.
webgrind of map_meta_cap calls
map_meta_cap.patch (1.3 KB) - added by kurtpayne 22 months ago.
Memoizing map_meta_cap
21120.2.diff (976 bytes) - added by nacin 22 months ago.
This ought to be API'd as suggested in the comment, but this does a number to the profiling output..
21120.3.diff (1.2 KB) - added by nacin 22 months ago.
21120.4.diff (38.4 KB) - added by nacin 21 months ago.
Always returns a WP_User object.
21120.5.diff (634 bytes) - added by SergeyBiryukov 20 months ago.
21120.6.diff (2.6 KB) - added by SergeyBiryukov 20 months ago.

Download all attachments as: .zip

Change History (51)

kurtpayne22 months ago

Proof of concept static cache

kurtpayne22 months ago

Before and after static cache

comment:1 kurtpayne22 months ago

To test this out, I put together a simple static cache for get_user_by() and ran a profile for the "All Posts" page before and after. It shaved off a good amount of time and duplicate work.

comment:2 follow-ups: scribu22 months ago

We already have a caching system. We should not invent ad-hoc systems for each function.

What we could do is collect all the user ids and call get_users() with them. This will in turn call cache_users().

comment:3 in reply to: ↑ 2 nacin22 months ago

Replying to scribu:

What we could do is collect all the user ids and call get_users() with them. This will in turn call cache_users().

Perhaps WP_Query should have a flag to do this. We already prime terms, meta, and optionally comments for a post. It then could be on by default in the admin.

kurtpayne22 months ago

Use wp_cache_get and wp_cache_set

comment:4 in reply to: ↑ 2 ; follow-ups: kurtpayne22 months ago

  • Keywords has-patch added

Replying to scribu:

We already have a caching system. We should not invent ad-hoc systems for each function.

I've uploaded a new patch that plays well with the caching system and the users group.

What we could do is collect all the user ids and call get_users() with them. This will in turn call cache_users().

Where do we draw the line on this? wp_is_large_network() ?

Replying to nacin:

Perhaps WP_Query should have a flag to do this.

I'm not sure where to put this. Hint? I'm also not sure if it's needed. Priming a user/cap cache on the "get all posts page" could net a positive performance benefit. Priming a user/cap cache on an "upload file" or "new post" page may be a waste.

comment:5 in reply to: ↑ 4 scribu22 months ago

Replying to kurtpayne:

What we could do is collect all the user ids and call get_users() with them. This will in turn call cache_users().

Where do we draw the line on this? wp_is_large_network() ?

We wouldn't fetch all the users, only the authors of the posts currently on display.

comment:6 in reply to: ↑ 4 scribu22 months ago

Replying to nacin:

Perhaps WP_Query should have a flag to do this.

I'm not sure where to put this. Hint? I'm also not sure if it's needed. Priming a user/cap cache on the "get all posts page" could net a positive performance benefit. Priming a user/cap cache on an "upload file" or "new post" page may be a waste.

On the contrary: the flag would be off by default, so you can choose when you need the user cache primed.

For an example, take a look at the 'update_post_term_cache' query var.

kurtpayne22 months ago

Adding an "update_author_cache" query var to WP_Query

comment:7 kurtpayne22 months ago

The 'all posts' page uses a list table which routes the requests through wp_edit_posts_query() which sends the query through wp(). There was no direct call to WP_Query as with the example for 'update_post_term_cache'

Here's the best workaround I could come up with. Please let me know what you think.

comment:8 scribu22 months ago

The workaround seems fine, but if you used get_users( array( 'include' => $user_ids ) ) you wouldn't need _prime_user_cache().

comment:9 scribu22 months ago

Because WP_User_Query internally does exactly what _prime_user_cache() from your patch does.

comment:10 kurtpayne22 months ago

This is the important behavior from _prime_user_cache that isn't replicated by WP_User_Query

$user->init( $userdata );

Right now the only place where WP_User::init is called is from get_user_by, so the added caching here is important. The author caching in the post query may be moot as a list of 20 posts that reference 4 authors will result in 4 cache misses either way.

comment:11 follow-up: scribu22 months ago

Actually, WP_User::init() is also called in WP_User::__construct() if you pass an id or a login.

comment:12 in reply to: ↑ 11 kurtpayne22 months ago

Replying to scribu:

Actually, WP_User::init() is also called in WP_User::__construct() if you pass an id or a login.

You're right. In WP_User_Query::query, though, the results of the query are cached, but not the results of WP_User::__construct called for each user in the query.

Perhaps this should be an additional optimization?

Here's the code in question from WP_User_Query::query

if ( 'all_with_meta' == $this->query_vars['fields'] ) {
	cache_users( $this->results );

	$r = array();
	foreach ( $this->results as $userid )
		$r[ $userid ] = new WP_User( $userid, '', $this->query_vars['blog_id'] );

	$this->results = $r;
}

comment:13 follow-up: scribu22 months ago

I'm not sure if caching the whole user object is a good idea, since it contains the capabilities for the current blog. Then, if you call for_blog() on it with a different ID, you end up in a weird state.

Besides, WP_User::init() doesn't seem that expensive.

Version 0, edited 22 months ago by scribu (next)

comment:14 in reply to: ↑ 13 kurtpayne22 months ago

Replying to scribu:

Besides, WP_User::init() doesn't seem that expensive.

It compounds when it's called 260+ times when it's only needed 4 times.

On my local dev server, this saves ~30ms of wall time (~5%). The xdebug profiling times (get_user_by_grind.png) are a little inflated because of the overhead, but this optimization does have a net positive impact.

I vote for leaving it at 21120.2.patch and dropping the post_author caching.

comment:15 follow-up: scribu22 months ago

Here's the workflow with 21120.2.patch:

First call:

get_user_by()
-> WP_User::get_data_by()
   -> wp_cache_get( $user_id, 'users' ) // gets the raw userdata
-> wp_cache_get( $user_id, 'users' ) // gets the raw userdata
-> wp_cache_set( $user_id, $full_user, 'users' )

Subsequent calls:

get_user_by()
-> WP_User::get_data_by( ... )
   -> wp_cache_get( $user_id, 'users' ) // gets the full userdata
-> wp_cache_get( $user_id, 'users' ) // gets the full userdata

That doesn't seem sane to me. At a minimum, you could eliminate the additional wp_cache_get() check from get_user_by().

The real question is why is get_user_by() called so many times?

comment:16 in reply to: ↑ 15 kurtpayne22 months ago

Replying to scribu:

The real question is why is get_user_by() called so many times?

get_user_by:
 - called 2 times from wp_validate_auth_cookie
   - called 1 time from get_currentuserinfo
   - called 1 time from from auth_redirect
 - called 262 times from get_userdata
   - called 2 times from get_avatar
   - called 20 times from setup_postdata (from WP_Post_List_Table::single_row)
   - called 240 times from map_meta_cap

Here's some of the places I could find that call map_meta_cap that are in a loop for showing posts:

 - get_edit_post_link( $post->ID );
 - current_user_can( $post_type_object->cap->edit_post, $post->ID );
 - current_user_can( 'read_post', $post->ID )
 - get_edit_post_link( $post->ID, true )
 - current_user_can( $post_type_object->cap->delete_post, $post->ID )
 - get_delete_post_link( $post->ID )
... then in WP_Posts_List_Table::get_inline_data() ...
 - current_user_can($post_type_object->cap->edit_post, $post->ID)

kurtpayne22 months ago

webgrind of map_meta_cap calls

kurtpayne22 months ago

Memoizing map_meta_cap

comment:17 kurtpayne22 months ago

The map_meta_cap.patch patch makes the following assumptions:

  1. calls to map_meta_cap are deterministic - a user is either capable or incapable of doing something in a given invocation
  2. if #1 isn't true, then the call to map_meta_cap can use $cache=false
  3. the cache should only be valid for the life of the process; using the wp_cache_* methods for persistent caching would be inefficient and unnecessary

In xdebug profiles, this has decreased run time of the "all posts" page by 12%. Without xdebug (running faster without the profiling overhead), run time (measured with Chrome's net panel) has decreased by about 8%.

comment:18 follow-up: scribu22 months ago

map_meta_cap() receives a variable number of arguments:

	$args = array_slice( func_get_args(), 2 );

You can't just add a $cache parameter to it.

Last edited 22 months ago by scribu (previous) (diff)

comment:19 in reply to: ↑ 18 kurtpayne22 months ago

Replying to scribu:

map_meta_cap() receives a variable number of arguments:

	$args = array_slice( func_get_args(), 2 );

You can't just add a $cache parameter to it.

Thanks, I missed the forest for the trees.

The extra parameter could be taken care of with a filter.

But since $args can be any object/scalar/array, the work it would take to generate a consistent key from this negate any cache savings.

Seems like caching in map_meta_cap is out.

@scribu, @nacin, Any ideas?

comment:20 scribu22 months ago

What we need to do is to avoid the get_user_by() calls, since we already have the full object for the current user.

comment:21 nacin22 months ago

I don't think it's a simple matter of optimizing the calls to get_user_by(). Rather, we should consider (consider) a WP_User::get_instance(), which is essentially get_user_by() but it instead returns a reference to an already existing WP_User object, if it has spun one up before. This is somewhat similar to how WP_Screen works.

Alternatively, we can look to make WP_User instantiation faster. Looking at some profiling output, that doesn't look simple — the bulk of the time spent is on unserializing metadata, initing caps/roles, and pulling the user's data from cache. Lazy-loading roles/caps won't help us either, as the bulk of our WP_User instantiations are for cap checks.

What we could do is make map_meta_cap() a bit smarter as well. Rather than calling get_userdata(), check if we're dealing with the current user, and if so, call wp_get_current_user().

Additionally, is_super_admin() is almost always passed a user ID in the capabilities API, but it is also almost always referring to the current user. When we do get a user_id in is_super_admin(), we should check against get_current_user_id() to see if we can get away with wp_get_current_user().

Again, this is where WP_User::get_instance() could come in — for when you're not sure if you need a new WP_User object, or if you can use wp_get_current_user(). Even if it doesn't keep a reference of *all* WP_Users it has spun up, just short-circuiting and calling wp_get_current_user() could make a huge performance difference.

nacin22 months ago

This ought to be API'd as suggested in the comment, but this does a number to the profiling output..

comment:22 nacin22 months ago

With 21120.2.diff, our "API" could simply be get_userdata(). We'd then just convert most new WP_User calls to get_userdata().

nacin22 months ago

comment:23 nacin22 months ago

Thanks to [UT902] (and the existing assertions) -- 21120.3.diff.

comment:24 ryan22 months ago

I've long wanted to deprecate get_userdata() since it is one of the pluggable functions that should not be. Now that we have deprecated-pluggable.php, why not either standardize on get_user_by() or a new get_user() function?

comment:25 nacin22 months ago

I'm happy with a wp_get_user() (or get_user()) and deprecate get_userdata(). I agree, pluggable-ness is lame.

comment:26 nacin21 months ago

The main decision to be made here is whether wp_get_user() should return WP_User on failure, or false on failure.

Currently, new WP_User and wp_get_current_user() will return a WP_User object. This object can then be checked with the exists() method.

get_userdata() returns false if the user does not exist. The same goes for get_user_by().

scribu and I had a long discussion in IRC, with ryan chiming in as well. We outlined the pros and cons of each approach, how we see it currently implemented throughout WordPress, and how we envision similar getters to operate in the future. See https://irclogs.wordpress.org/chanlog.php?channel=wordpress-dev&day=2012-07-19&sort=asc#m424509, starts at 15:36, ends at 16:14.

nacin21 months ago

Always returns a WP_User object.

comment:27 scribu21 months ago

Another decision is whether to deprecate wp_get_current_user() - because of it's pluggable-ness - or to let it be.

Last edited 21 months ago by scribu (previous) (diff)

comment:28 markjaquith21 months ago

I'm leaning towards always returning a WP_User object. For chaining ability, and for parity with some of the other objects/instantiation functions we've been making.

comment:29 follow-up: nacin21 months ago

In [21375]:

Don't call get_userdata() every time for the current user in map_meta_cap()'s read_post, edit_post, and delete_post branches. see #21120.

comment:30 nacin21 months ago

In [21376]:

Optimize get_user_by( 'id', $id ) to return wp_get_current_user() when the current user ID is requested.

Provides for a major performance improvement by preventing repeated instantiations of WP_User in the capabilities API.

see #21120.

comment:31 nacin21 months ago

In [21377]:

Use get_userdata() rather than new WP_User in is_super_admin(), to take advantage of the performance improvements in [21376]. see #21120.

comment:32 nacin21 months ago

  • Milestone changed from Awaiting Review to 3.5

comment:33 nacin21 months ago

In [21413]:

Move most instances of new WP_User to get_userdata(). see #21120.

comment:34 in reply to: ↑ 29 SergeyBiryukov20 months ago

[21376] caused a bug with changing password: ticket:13351:3.

Last edited 20 months ago by SergeyBiryukov (previous) (diff)

comment:35 nacin20 months ago

I think #13351 demonstrates that [21376] is probably too deep in the stack.

The big thing here was map_meta_cap() and is_super_admin(). If we can optimize those two to use wp_get_current_user() when appropriate, that handles almost all of our WP_User::init calls.

SergeyBiryukov20 months ago

comment:36 SergeyBiryukov20 months ago

21120.5.diff is an attempt at optimizing is_super_admin().

SergeyBiryukov20 months ago

comment:37 SergeyBiryukov20 months ago

21120.6.diff includes map_meta_cap().

comment:38 nacin20 months ago

In [21563]:

Move the optimization done to get_user_by() in [21376] higher up the stack, into map_meta_cap() and is_super_admin().

This provides nearly the same benefits without possibly receiving a stale object from get_userdata(),
which could affect authentication, and introduce side effects for plugins.

see #21120.

comment:39 nacin20 months ago

  • Resolution set to fixed
  • Status changed from new to closed

Think this is good.

comment:40 nacin3 months ago

#8911 was marked as a duplicate.

Note: See TracTickets for help on using tickets.