WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 15 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 3 years ago.
Proof of concept static cache
get_user_by_grind.png (23.7 KB) - added by kurtpayne 3 years ago.
Before and after static cache
21120.2.patch (497 bytes) - added by kurtpayne 3 years ago.
Use wp_cache_get and wp_cache_set
21120.3.patch (2.9 KB) - added by kurtpayne 3 years ago.
Adding an "update_author_cache" query var to WP_Query
map_meta_cap_calls.png (19.7 KB) - added by kurtpayne 3 years ago.
webgrind of map_meta_cap calls
map_meta_cap.patch (1.3 KB) - added by kurtpayne 3 years ago.
Memoizing map_meta_cap
21120.2.diff (976 bytes) - added by nacin 3 years 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 3 years ago.
21120.4.diff (38.4 KB) - added by nacin 3 years ago.
Always returns a WP_User object.
21120.5.diff (634 bytes) - added by SergeyBiryukov 3 years ago.
21120.6.diff (2.6 KB) - added by SergeyBiryukov 3 years ago.

Download all attachments as: .zip

Change History (51)

@kurtpayne3 years ago

Proof of concept static cache

@kurtpayne3 years ago

Before and after static cache

comment:1 @kurtpayne3 years 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: @scribu3 years 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 @nacin3 years 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.

@kurtpayne3 years ago

Use wp_cache_get and wp_cache_set

comment:4 in reply to: ↑ 2 ; follow-ups: @kurtpayne3 years 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 @scribu3 years 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 @scribu3 years 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.

@kurtpayne3 years ago

Adding an "update_author_cache" query var to WP_Query

comment:7 @kurtpayne3 years 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 @scribu3 years ago

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

comment:9 @scribu3 years ago

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

comment:10 @kurtpayne3 years 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: @scribu3 years 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 @kurtpayne3 years 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: @scribu3 years 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 blog id, you end up in a weird state.

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

Last edited 3 years ago by scribu (previous) (diff)

comment:14 in reply to: ↑ 13 @kurtpayne3 years 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: @scribu3 years 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 @kurtpayne3 years 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)

@kurtpayne3 years ago

webgrind of map_meta_cap calls

@kurtpayne3 years ago

Memoizing map_meta_cap

comment:17 @kurtpayne3 years 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: @scribu3 years 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 3 years ago by scribu (previous) (diff)

comment:19 in reply to: ↑ 18 @kurtpayne3 years 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 @scribu3 years 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 @nacin3 years 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.

@nacin3 years ago

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

comment:22 @nacin3 years 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().

@nacin3 years ago

comment:23 @nacin3 years ago

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

comment:24 @ryan3 years 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 @nacin3 years ago

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

comment:26 @nacin3 years 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.

@nacin3 years ago

Always returns a WP_User object.

comment:27 @scribu3 years ago

Another decision is whether to deprecate wp_get_current_user() or not.

Version 0, edited 3 years ago by scribu (next)

comment:28 @markjaquith3 years 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: @nacin3 years 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 @nacin3 years 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 @nacin3 years 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 @nacin3 years ago

  • Milestone changed from Awaiting Review to 3.5

comment:33 @nacin3 years ago

In [21413]:

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

comment:34 in reply to: ↑ 29 @SergeyBiryukov3 years ago

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

Last edited 3 years ago by SergeyBiryukov (previous) (diff)

comment:35 @nacin3 years 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.

@SergeyBiryukov3 years ago

comment:36 @SergeyBiryukov3 years ago

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

@SergeyBiryukov3 years ago

comment:37 @SergeyBiryukov3 years ago

21120.6.diff includes map_meta_cap().

comment:38 @nacin3 years 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 @nacin3 years ago

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

Think this is good.

comment:40 @nacin15 months ago

#8911 was marked as a duplicate.

Note: See TracTickets for help on using tickets.