WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 2 months ago

#36508 reviewing defect (bug)

Call cache_users() when 'fields'=>'all' in WP_User_Query

Reported by: danielbachhuber Owned by: boonebgorges
Milestone: Priority: normal
Severity: normal Version:
Component: Users Keywords: has-patch needs-testing needs-refresh 4.7-early
Focuses: performance Cc:

Description

Because roles and caps are populated (r32001), which is a meta call, we should proactively call cache_users() and pre-empt dozens of unnecessary meta SELECTs.

cc @boonebgorges

Attachments (1)

36508.diff (6.8 KB) - added by boonebgorges 3 years ago.

Download all attachments as: .zip

Change History (7)

#1 @boonebgorges
3 years ago

Yeah, we should do something here. Not totally sure what. A couple thoughts:

  • This is not a new issue. [32001] pointed the role query at the correct blog, but the query has always taken place.
  • fields=all doesn't interact with any cache - either for usermeta or for user objects themselves

If we call all of cache_users(), we have essentially eliminated the distinction between fields=all and fields=all_with_meta. See 36508.diff. On many sites this would be an improvement. But usermeta also tends to be abused, so that fetching usermeta for all users in a query can significantly affect memory footprint. This is especially worrisome given that all is the default value of fields.

A couple of other possibilities:

  1. We introduce a standalone function for priming the wp_x_capabilities cache, and call that - either instead of cache_users() or as an option within cache_users() - when fields=all.
  2. We lazyload usermeta in WP_User_Query loops. Usermeta, including capabilities, wouldn't be loaded until it's requested. This would mean some refactoring of WP_User, so that capabilities aren't loaded into user objects until they're requested (so WP_User::allcaps etc would be handled via magic method or something like that). This is obviously much more extensive, but has the potential to improve performance across the board when using either WP_User_Query or WP_User.

@danielbachhuber Do you have thoughts about the best strategy?

#2 @danielbachhuber
3 years ago

I think this option would be the most precise:

We introduce a standalone function for priming the wp_x_capabilities cache, and call that - either instead of cache_users() or as an option within cache_users() - when fields=all.

Solely priming the capabilities cache would continue to respect the distinction between fields=>all and fields=>all_with_meta, while also addressing the unnecessary queries issue.

I agree with this sentiment too:

But usermeta also tends to be abused, so that fetching usermeta for all users in a query can significantly affect memory footprint.

@boonebgorges
3 years ago

#3 @boonebgorges
3 years ago

  • Keywords has-patch needs-testing added; needs-patch removed
  • Milestone changed from Future Release to 4.6

This is more complicated than I'd originally thought. (Gee, I end up saying that a lot.) You can't prime WP's metadata cache for a single key. Cached data is keyed by object ID (such as user_id), and if get_metadata() finds that $cache[ $user_id ] is populated, it assumes that it's *completely* populated (ie, with all of $user_id's usermeta). Invalidation, cache priming, and a bunch of other stuff is tied up with this schema.

Instead, I opted in 36508.diff to run a very specific query within WP_User_Query, and then pass an array of $raw_caps to the WP_User object, which WP_User uses instead of get_user_meta( $cap_key ). The structure of WP_User makes it impossible to do this in a non-clunky way, but the patch works.

#4 follow-up: @ocean90
3 years ago

@boonebgorges After three months, what are you plans with 36508.diff?

#5 in reply to: ↑ 4 @boonebgorges
3 years ago

  • Keywords needs-refresh 4.7-early added
  • Milestone changed from 4.6 to Future Release
  • Owner set to boonebgorges
  • Status changed from new to reviewing

Replying to ocean90:

@boonebgorges After three months, what are you plans with 36508.diff?

Sorry this got lost in the shuffle. I continue to think the approach is clunky, but can't think of a better way to do it. I don't have the stomach for this kind of change at this point in the cycle, so let's pick it up for 4.7.

This ticket was mentioned in Slack in #core by helen. View the logs.


3 years ago

Note: See TracTickets for help on using tickets.