Make WordPress Core

Opened 7 years ago

Closed 2 years ago

Last modified 23 months ago

#38741 closed enhancement (fixed)

Introduce the concept of a large site in order to speed up the Users screen when there are many users

Reported by: johnbillion's profile johnbillion Owned by: spacedmonkey's profile spacedmonkey
Milestone: 6.0 Priority: normal
Severity: normal Version:
Component: Users Keywords: has-patch needs-dev-note
Focuses: administration, multisite, performance Cc:

Description

Given a single site install with a high number of users, the Users screen can become very slow due to the nasty query performed by count_users(). The result of this query is used to populate the count of users with each role in the role filtering links across the top.

SELECT
	COUNT(NULLIF(`meta_value` LIKE '%\"administrator\"%', false)),
	COUNT(NULLIF(`meta_value` LIKE '%\"editor\"%', false)),
	COUNT(NULLIF(`meta_value` LIKE '%\"author\"%', false)),
	COUNT(NULLIF(`meta_value` LIKE '%\"contributor\"%', false)),
	COUNT(NULLIF(`meta_value` LIKE '%\"subscriber\"%', false)),
	COUNT(NULLIF(`meta_value` = 'a:0:{}', false)), COUNT(*)
FROM
	wp_usermeta
WHERE
	meta_key = 'wp_capabilities'

On a site with tens of thousands of users, this SQL query can easily take tens of seconds.

Multisite has the concept of a large network (by default, one with > 10,000 users or > 10,000 sites), but single site doesn't have this concept.

The query performed by count_users() can be skipped entirely. If it is skipped, the list of roles will simply display all available roles and won't display a count next to each role. I think this is a completely acceptable trade-off for such a performance gain on sites with a high number of users.

Attachments (12)

38741.diff (3.9 KB) - added by johnbillion 7 years ago.
38741.2.diff (5.5 KB) - added by tharsheblows 7 years ago.
with wp_is_large_site()
38741.3.diff (6.3 KB) - added by tharsheblows 7 years ago.
with tests and cleaned up a bit
38741.4.diff (8.5 KB) - added by tharsheblows 7 years ago.
renamed functions and apply filter before ms one
38741.5.diff (8.9 KB) - added by johnbillion 7 years ago.
38741.6.diff (8.4 KB) - added by johnbillion 7 years ago.
38741.7.diff (9.1 KB) - added by johnbillion 7 years ago.
38741.8.diff (12.2 KB) - added by spacedmonkey 6 years ago.
38741.9.diff (15.6 KB) - added by spacedmonkey 6 years ago.
38741.10.diff (16.2 KB) - added by macbookandrew 6 years ago.
Adds another fix for bulk-editing of posts
38741.11.diff (16.9 KB) - added by macbookandrew 5 years ago.
patch file for 5.2.1
38741.12.diff (1.3 KB) - added by peterwilsoncc 2 years ago.

Download all attachments as: .zip

Change History (120)

@johnbillion
7 years ago

#1 @johnbillion
7 years ago

  • Keywords has-patch needs-testing added

38741.diff is a partial patch which implements the performance improvement on the Users screen, but only implements a no-oped version of a wp_is_large_site() function.

To test the patch, change the return value of wp_is_large_site() to true.

TODO: Implement logic in wp_is_large_site() which determines if the site has a large number of users or not. This will probably need to be converted to a cached value in the same way that Multisite uses a cached value for wp_is_large_network().

#2 @Mista-Flo
7 years ago

+1 on this, we should focus on this performance issue.

#4 @tharsheblows
7 years ago

There's also an instance in wp-includes/update.php so I've updated that one too. This makes the count a transient with a max expiration of 12 hours and takes the query down from about 2.5s to 50.3ms with user table of 184k.

I'm not sure if:

  1. the value should be a transient
  2. the expiration time should be filterable

Still needs testing and tests.

@tharsheblows
7 years ago

with wp_is_large_site()

@tharsheblows
7 years ago

with tests and cleaned up a bit

#5 @tharsheblows
7 years ago

With tests and cleaned up a bit

#6 @johnbillion
7 years ago

  • Keywords has-unit-tests added
  • Milestone changed from Awaiting Review to 4.8

#7 @tharsheblows
7 years ago

Updated function names and applies before the 'wp_is_large_network' too.

@tharsheblows
7 years ago

renamed functions and apply filter before ms one

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


7 years ago

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


7 years ago

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


7 years ago

#11 @obenland
7 years ago

  • Milestone changed from 4.8 to Future Release

#12 @johnbillion
7 years ago

Needs some more testing and review.

#13 @johnbillion
7 years ago

@tharsheblows I just remembered that we already have get_user_count() which stores a count of all the users on the network. This can be used in place of the new user count functions that you've added in your patches.

Do you want to update your patch? I'd like to see about getting this into 4.9.

#14 @tharsheblows
7 years ago

@johnbillion that's multisite only, isn't it? In ms-functions.php.

I'd love to get this in 4.9! :)

@johnbillion
7 years ago

@johnbillion
7 years ago

@johnbillion
7 years ago

#15 @miss_jwo
7 years ago

Looks good to me. Thansk for changing the function name to wp_is_large_user_count() as the original suggestion would have been misleading in many ways.

#16 @johnbillion
7 years ago

  • Keywords needs-testing removed
  • Milestone changed from Future Release to 4.9
  • Owner set to johnbillion
  • Status changed from new to reviewing

#17 @johnbillion
6 years ago

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

In 41613:

Users: Introduce the concept of a large site in order to speed up the Users screen when there are many users.

Calling the count_users() function is expensive, regardless of the counting strategy that's used, and it gets
slower the more users there are on a site. In order to speed up the Users screen in the admin area, calling
count_users() can be avoided entirely while still displaying the total count for users.

This introduces some new functions:

  • wp_is_large_user_count()
  • wp_get_active_user_count()
  • wp_update_active_user_count()

A corresponding wp_is_large_user_count filter is also introduced.

Props tharsheblows, johnbillion

Fixes #38741

#18 @johnbillion
6 years ago

In 41614:

Users: There is not, in fact, 12345 users on every WordPress installation.

Props spacedmonkey

See #38741

#19 @johnbillion
6 years ago

  • Keywords needs-patch needs-unit-tests added; has-patch has-unit-tests removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Some issues have arisen:

  • The function names need improving as nothing here relates to whether a user is "active" or not.
  • COUNT(ID) should be used to better utilise the table index.
  • The tests are failing ¯\_(ツ)_/¯

#20 @johnbillion
6 years ago

In 41623:

Users: Remove the failing tests added in [41613] while they're investigated.

See #38741

#21 @jrchamp
6 years ago

If you don't want to worry about the right field to use to match the available indexes, COUNT(1) might be a simpler, consistent option that the database can optimize easily.

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


6 years ago

#23 @johnbillion
6 years ago

  • Focuses multisite added
  • Status changed from reopened to accepted

I'm going back to the idea of moving get_user_count() out of the Multisite files and making it available for single site installations too. Otherwise we have separate functions (get_user_count() and the soon-to-be-renamed wp_get_active_user_count()) for essentially doing the same thing.

Patch coming up.

Adding the multisite focus even though this functionality doesn't actually affect Multisite.

#24 @johnbillion
6 years ago

  • Keywords needs-revert added; needs-patch needs-unit-tests removed

Regrettably I think this needs some more work and I don't want to rush it in today. I'm going to revert this and get it into 5.0 early.

#25 @johnbillion
6 years ago

In 41753:

Users: Revert [41613], [41614], and [41623] as this feature needs some more work.

See #38741

#26 @johnbillion
6 years ago

  • Keywords early needs-patch added; needs-revert removed
  • Milestone changed from 4.9 to Future Release

#27 @johnbillion
6 years ago

  • Milestone changed from Future Release to 5.0

This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.


6 years ago

#29 @tharsheblows
6 years ago

I'm currently using 38741.7.diff(ish — with 4.8.3) on a production site and will let you know if it all goes horribly wrong. (It hasn't yet.)

#30 @flixos90
6 years ago

@johnbillion I like the idea of tying in more with the existing multisite functionality, and I actually prefer the initial name wp_is_large_site() as it would align well with the existing wp_is_large_network(). It could have a parameter defaulting to users, which determines what criteria to use to determine whether the site is considered large. This would make it future-proof for other similar ideas (maybe wp_is_large_site( 'posts' ) could come in handy at some point too.
I don't like the wp_is_large_user_count() as it's unclear how that works (it counts all users, in multisite that's the entire setup). I also don't think we should need a separate additional filter to wp_is_large_network() just for that purpose. The new function should be there for a site. It should be available for a single site, but just as useful when having multisite enabled.

I propose the following:

  • Introduce wp_is_large_site( $using = 'users', $site_id = null ). The second parameter defaults to the current site, but allows to do the check for another site in multisite.
  • The option where the number is stored should be called user_count. I don't see why the 'active' is needed here at all.
  • The SQL query to count the users can remain as is for single site (single all users = site users here), but in multisite it should have a meta table join and do WHERE umeta.meta_key = 'wp_X_capabilities' (depending on the site checked).
  • wp_update_active_user_count() should be wp_update_site_user_counts( $site_id = null ) instead, to align with wp_update_network_user_counts( $network_id = null ).
  • I really like the idea of leveraging get_user_count() for sites as well, however I'm not sure how to sanely do so as it already has a $network_id as first parameter. We could create get_site_user_count() but that'd be kind of incoherent (unless we'd also have get_network_user_count() - but maybe we could consider refactoring this).

#31 @tharsheblows
6 years ago

@flixos90 It'd be useful for me to take a step back and clarify what wp_is_large_site('users') is meant to do – in my mind it's a kill switch for functions that are too expensive to run when a table is large, eg count_users() on a single site install. In this context only the size of the table matters. For example, if a site in a multisite network only has 10 users but the network has 20,000 then it's not a good idea to run a slow query on it.

My worry is that people will use wp_is_large_site( 'users' ) as a check before running expensive queries on both single site and multisite installs when this might not be what they wanted. One possible workaround is to use wp_is_large_network() as the multisite check for wp_is_large_site('users') rather than the query on usermeta when it's used on multisite and keep the rest the same because those are useful functions. Extending to posts and other tables could then be done on a site by site basis.

This ticket was mentioned in Slack in #core-multisite by tharsheblows. View the logs.


6 years ago

This ticket was mentioned in Slack in #core-multisite by spacedmonkey. View the logs.


6 years ago

#34 @flixos90
6 years ago

We reviewed and discussed this in today's multisite office-hours (see https://wordpress.slack.com/archives/C03BVB47S/p1527609680000818 for the chat log). Here are the decisions made during the conversation:

  • Multisite's get_user_count() has historically contained the global user count already. While it's stored on every network, it is actually a global count. Let's make this function available for single site.
    • The function supports an optional $network_id parameter. This was just recently introduced (by myself, and I regret it given these circumstances), so we can deprecate it again. This should no longer be considered a network-specific function.
    • On single-site, the user_count should be stored as a regular option. When setting up multisite, it should be migrated to become a network setting.
  • wp_is_large_user_count() should be introduced as a globally available function to check the user count. It should call get_user_count() to get its value and use 10000 as a maximum border by default. A filter should be present to modify the result of this function.
  • The multisite function wp_is_large_network() should be updated to call wp_is_large_user_count() if called with 'users' as the metric parameter. It won't have any functional change, just get rid of some logic that would otherwise be duplicated.
  • wp_is_large_user_count() should be used in count_users() to short-circuit as needed.
Version 0, edited 6 years ago by flixos90 (next)

@spacedmonkey
6 years ago

#35 @spacedmonkey
6 years ago

  • Keywords has-patch dev-feedback needs-testing added; needs-patch removed

38741.8.diff is a completely new patch based on the feedback from @flixos90 and @jeremyfelt in multisite chat.

What this does, it makes get_user_count (and related functions) available in single site. This also means that there is a scheduled event that runs on single site twice a day now to get this count and store in options. This is a better implementation than doing a count on every site on a multisite, as for large networks, running this on every site is expensive.

If the site is seen as large, it now doesn't include counts and all the registered roles are displayed. This is a functional change and may require some discussion.

#36 @tharsheblows
6 years ago

Moving get_user_count to wp-includes/functions.php so it's available for single sites might cause issues with plugins using the function name to do the same thing. I vaguely remember discussing this with @johnbillion at a contributor day which is why the initial patches didn't do that. Sorry that discussion wasn't included here!

#37 @spacedmonkey
6 years ago

Well I simple solution to that problem would be calling the function in wp_get_global_user_count, which is a better name, deprecate get_user_count and get it call wp_get_global_user_count inside it.

#38 @flixos90
6 years ago

Even more than with registered post types, metadata, options and anything else, declaring functions without a prefix is an absolute no-go unless you are a 100% sure you know what you're doing. I don't think we should change the name of get_user_count() just because someone might have declared it. We should only change the name if we actually think the name is not correct or precise enough.

This change will be documented as well, so if someone is in fact declaring get_user_count() on their own, they should at least be aware of what they're doing is critical and be responsible of following core development in that regard closely enough.

#39 @tharsheblows
6 years ago

:) Y'all are harsh.

Having "network" in the function names for single sites feels strange to me, it's contextless and a bit confusing. I would think it's probably worth doing what you suggested @spacedmonkey to have clearer names in all those functions.

I'm not sure that single site needs a scheduled twice daily user count update on single sites either, it's fine to run it on every registration and deletion, it's a quick query. For multisite, the spam and deleted columns aren't indexed so it probably makes sense there, I think that's how it currently works. (But if this function is to be used in a kill switch for expensive queries on the users table, then it needs to be the same query as single site, ie SELECT COUNT(ID) FROM $wpdb->users – it's the size of the table that matters then)

In general, I love that WordPress is sympathetic to new developers and those who might have done it wrong in the past and never fixed it. If avoiding a function name collision so that we don't brick their sites is easy, then why not do it? Using something like wp_get_user_count has the added benefit of fitting in with other WP function names.

#40 @spacedmonkey
6 years ago

I agree. Here is what I think we should do.

We should deprecate get_user_count as it currently allows for network_id to be passed. It is confusing in a single site context. We should remove all references (there is only 4 in code).

Introduce wp_get_user_count function, that doesn't have any params on it. This fixes the naming issues and is a valid reason to get rid of the get_user_count.

When I get a minute, I will update the patch.

#41 @flixos90
6 years ago

That is an argument I agree with. :)

Before we deprecate, I'd like to know though how often this function is used by plugins. We probably wanna be kind if tons of plugins use it.
And one aside, if we deprecate get_user_count(), we should as well consider deprecating get_blog_count() as this should then rather be named get_network_site_count() (contrary to get_user_count(), the current get_blog_count() actually works per network). This would not be part of this ticket, but something to be aware of and think about.

@spacedmonkey
6 years ago

#42 @spacedmonkey
6 years ago

As of 38741.9.diff there are the following changes

  • Added wp_get_user_count function, with the logic contained in get_user_count but no network id param
  • Deprecated get_user_count
  • Deprecated $network_id in wp_is_large_network as unable to pass to get_user_count as wp_get_user_count now in use.
  • Removed all references to get_user_count in code.

#43 @macbookandrew
6 years ago

We need this for a site we’re currently working on. I manually applied 38741.9.diff to WP core and it seems to be working great.

Is there anything I can do to help with this ticket?

@macbookandrew
6 years ago

Adds another fix for bulk-editing of posts

#44 follow-up: @macbookandrew
6 years ago

Found another place where a large number of users is a performance hit: wp-admin/includes/class-wp-posts-list-table.php calls wp_dropdown_users when generating the list of authors for bulk edit mode.

38741.10.diff includes a fix for that.

Is it ok to include here or should I open a separate ticket instead?

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


6 years ago

#46 in reply to: ↑ 44 @spacedmonkey
6 years ago

Replying to macbookandrew:

Found another place where a large number of users is a performance hit: wp-admin/includes/class-wp-posts-list-table.php calls wp_dropdown_users when generating the list of authors for bulk edit mode.

38741.10.diff includes a fix for that.

Is it ok to include here or should I open a separate ticket instead?

Thanks for the input @macbookandrew . However, I believe that the issues with should addressed in another tickets. For a little history have a look at this ticket #40613 that would likely fix issues wp_dropdown_users.

#48 @johnbillion
5 years ago

  • Milestone changed from 5.0 to 5.1

#49 @pento
5 years ago

  • Milestone changed from 5.1 to 5.2

This ticket was mentioned in Slack in #core-multisite by spacedmonkey. View the logs.


5 years ago

#51 @desrosj
5 years ago

  • Milestone changed from 5.2 to 5.3

This has not seen any action in several months, and is marked early. Since we are late in the 5.2 timeline, going to punt to 5.3.

@macbookandrew
5 years ago

patch file for 5.2.1

#52 @Mista-Flo
5 years ago

Patch looks good to me, great job @macbookandrew :)

#53 @johnjamesjacoby
5 years ago

Coming in blind to this issue, the proposed solutions seem somewhat compromised and inconsistent. Multisite is already a mess of function names, and these new ones don't provide much in the way of obviousness or clarity.

I agree with @flixos90, and do not like wp_is_large_user_count() as a function name - its intended purpose is not immediately clear without the additional context in the comments in this ticket, especially without corresponding wp_is_large_site_count() and wp_is_large_network_count() functions to complete the use-case coverage.

I'm also reluctant to deprecate the $network_id parameter in wp_is_large_network() because its intended purpose is to determine whether or not any specific network is large, either by number of sites or users.

(_network_ functions should be doing Network things, just like _site_ functions (usually) do Site things.)

I also do not agree that not passing a $network_id (or using it when in single-site) is wrong here. In either case, get_current_blog_id() and get_current_network_id() should be trusted to do their jobs. If they can't be, they should be updated to allow them to be.

I suggest we introduce a new is_large_install() function (to use for both single-site and multi-site) because that appears to be the metric we are trying to gauge here. That would also partner up well with is_subdomain_install() and friends.


Regarding how the counts themselves are updated, Comments sets a precedent (as do Term Relationships) with how they defer their counts using wp_defer_comment_counting(), wp_update_comment_count(), and wp_update_comment_count_now(), and I believe their approaches are as sound as any.

They include queries, caches, and persistent values in the database, all relative to their object hierarchy.

I suggest we go that route for all of these counts instead of reinventing this wheel.

#54 @jb510
5 years ago

I found this looking into why count_users() was causing many pages in the admin to drag (and in this case only 6000 users). It's not just a problem on wp-admin/users.php.

+1 on sane function naming, but I find @johnjamesjacoby's suggestion confusing as well. I'm not clear what is_large_install() would specifically check for? Everything? Why? Does it still have functions to check for individual things being large?

We have very large sites in terms of posts, in terms of comments, in terms of users, in terms of media assets and probably other things ... and they don't generally coincide.

It doesn't make sense to me to short circuit comment counts just because you have a lot of users, or vice versa, which I presume is_large_install would do.

#55 @johnjamesjacoby
5 years ago

@jb510 good point about my suggested function name. You’re right that it is neither more obvious nor clear. Oops. 🤣

Ideally, my second suggestion (related to using the Defer approach to counts) eliminates the slowness of functions like count_users() making “large thing” type functions wholly unnecessary (because the counts would all be inexpensive to retrieve from their now-persistent locations.)

Rather than introduce new “large thing” functions, maybe we can remove the one we have instead?

#56 @jb510
5 years ago

I love the idea of deferred counting and caching these types of things, but I've never seen "core" be receptive to that sort of approach.

Coming into this myself, fixing count_users() directly rather than adding an abstraction that results in inconsistent results (why do I see user counts on some sites and not on others?!?!) seems a better approach. It just may not be possible. Even deferred a query into a 250k row table is going to time out for a lot of folks.

#57 @davidbaumwald
4 years ago

  • Keywords 2nd-opinion added
  • Milestone changed from 5.3 to Future Release

This ticket needs some additional direction, a final patch, and testing. With version 5.3 Beta 1 tomorrow, this is being punted to Future Release. This the remaining issues can be resolved and a patch committed in time, please feel free to move it back.

#58 @tripflex
3 years ago

4+ Years and this is still a major issue with running any kind of large site single installation WordPress site :(

#59 @spacedmonkey
3 years ago

@johnbillion It seems like this was fixed in [41613] in WP 4.9 release. I think this ticket can be closed out.

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

#60 follow-up: @johnbillion
3 years ago

Unfortunately not as that commit got reverted. See https://core.trac.wordpress.org/ticket/38741#comment:25 .

#61 in reply to: ↑ 60 @tripflex
3 years ago

Replying to johnbillion:

Unfortunately not as that commit got reverted. See https://core.trac.wordpress.org/ticket/38741#comment:25 .

Yeah was going to say, we have ~300k users on a site and this is still a major issue, had to write my own plugin to dynamically search for users (like WooCommerce does), filtering on the users_pre_query to only initially return user 1 (as to not load all users).

Doing this reduced our initial page load by almost 10+ seconds, and we're not running on a shared server or anything, this is on an elite plan with Pantheon so the resources are there but having 1m+ usermeta rows and 300k+ users def causes slowdowns in admin area anywhere wp_dropdown_users is used

#62 @johnbillion
3 years ago

  • Owner johnbillion deleted
  • Status changed from accepted to assigned

I'd love to work on this again but I don't have the time at the moment.

#63 @spacedmonkey
3 years ago

Hey @johnbillion can you provide some details on what is missing that would be need to get this into core? I personally believed that last patch I provided was good to go, maybe there was something I missed.

I think a good first step would be merge the patch into a github PR, so it is easier to review.

#64 @johnbillion
2 years ago

  • Milestone changed from Future Release to 6.0

#65 @SergeyBiryukov
2 years ago

Related: [meta10638], [meta11547] as an example of some workarounds.

#66 @costdev
2 years ago

  • Keywords needs-refresh added

Patch 38741.11.diff no longer applies cleanly against trunk. Needs a considered refresh.

@johnbillion Once the patch is refreshed, what do you think are the next steps for this ticket? Does it just need testing, or do the meta tickets in @SergeyBiryukov's comment above have an impact on the path forward?

This ticket was mentioned in PR #2410 on WordPress/wordpress-develop by dream-encode.


2 years ago
#67

  • Keywords needs-refresh removed

Trac ticket: https://core.trac.wordpress.org/ticket/38741.

This is almost a 1-1 copy of the most recent patch on Trac. The relevant version references have been updated to 6.0.0.

#68 @davidbaumwald
2 years ago

Per @spacedmonkey's suggestion, I created PR #2410 to get the ball rolling here again. This will need to move soon if it's to land early in 6.0.

dream-encode commented on PR #2410:


2 years ago
#69

Looks like tests are failing. The first big issues I see is "Unexpected deprecated notice for wp_is_large_network". We'll need to rework the tests to account for the deprecation.

#70 @davidbaumwald
2 years ago

  • Keywords needs-unit-tests added

#72 @spacedmonkey
2 years ago

I have created a scaled back patch at #2413. It keeps support for multiple networks and doesn't break backwards compatibility.

#73 @spacedmonkey
2 years ago

  • Owner set to spacedmonkey

spacedmonkey commented on PR #2413:


2 years ago
#74

CC @dream-encode

johnbillion commented on PR #2413:


2 years ago
#75

This is looking pretty solid and is working as expected on both a single site installation and a Multisite installation with tens of thousands of users.

I pushed a few commits which mostly tweak docs and formatting, but also one to add the $network_id parameter to the wp_is_large_user_count filter and one to add a wp_ prefix to the cron event name.

johnbillion commented on PR #2413:


2 years ago
#76

This discussion on the get_user_count() return value needs resolving one way or another: https://github.com/WordPress/wordpress-develop/pull/2413/files#r830770206

spacedmonkey commented on PR #2413:


2 years ago
#77

@johnbillion What are the next steps to get this committed?

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


2 years ago

#79 @johnbillion
2 years ago

  • Keywords has-unit-tests commit added; dev-feedback needs-testing 2nd-opinion needs-unit-tests removed

I'm happy with the state of Jonny's PR and I think we should get this committed so we've plenty of time for testing. @spacedmonkey over to you.

#80 @spacedmonkey
2 years ago

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

In 53011:

Users: Introduce the concept of a large site to single site installations.

Currently in WordPress multisite there is a concept of large networks. The function wp_is_large_network is used to determine if a network has a large number of sites or users. If a network is marked as large, then
expensive queries to calculate user counts are not run on page load but deferred to scheduled events. However there are a number of places in a single site installation where this functionality would also be useful, as
expensive calls to count users and roles can make screens in the admin extremely slow.

In this change, the get_user_count function and related functionality around it is ported to be available in a single site context. This means that expensive calls to the count_users function are replaced with
calls to get_user_count. This change also includes a new function called wp_is_large_user_count and a filter of the same name, to mark if a site is large.

Props johnbillion, Spacedmonkey, Mista-Flo, lumpysimon, tharsheblows, obenland, miss_jwo, jrchamp, flixos90, macbookandrew, pento, desrosj, johnjamesjacoby, jb510, davidbaumwald, costdev.
Fixes #38741.

spacedmonkey commented on PR #2413:


2 years ago
#81

Committed 🎉

#82 @SergeyBiryukov
2 years ago

In 53016:

I18N: Move code out of a translatable string in get_user_count() and related functions.

Follow-up to [53011].

See #38741.

#83 @SergeyBiryukov
2 years ago

In 53018:

Users: Move get_user_count() and related functions to wp-includes/user.php.

The new location is next to the pre-existing count_users() function, along with other user-specific functions, and should be a more appropriate place in terms of consistency.

This affects:

  • get_user_count()
  • wp_maybe_update_user_counts()
  • wp_update_user_counts()
  • wp_schedule_update_user_counts()
  • wp_is_large_user_count()

Follow-up to [53011], [53016].

See #38741.

#85 @dd32
2 years ago

@spacedmonkey after https://core.trac.wordpress.org/changeset/53011/trunk/src/wp-admin/includes/class-wp-posts-list-table.php the following PHP Notice will be triggered on large user counts:

E_NOTICE: Undefined variable: authors_dropdown
in wp-admin/includes/class-wp-posts-list-table.php:1752
Last edited 2 years ago by dd32 (previous) (diff)

#86 follow-up: @spacedmonkey
2 years ago

@dd32 Is this in a single or multisite?

#87 in reply to: ↑ 86 @dd32
2 years ago

Replying to spacedmonkey:

Is this in a single or multisite?

Multisite, but I don't think it matters; $authors_dropdown is only defined within that if branch, but is used within other branches which only have the post_type_supports() check.

#89 @spacedmonkey
2 years ago

Created a PR with a simple fix #2493.

Can @johnbillion or @dd32 take a look.

#90 @johnbillion
2 years ago

  • Keywords early has-unit-tests commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

#91 @tharsheblows
2 years ago

wp_dropdown_users() is filterable and people (including me 😊 ) will be using it even with large sites so it shouldn't be skipped.

What to do depends on the scope of this ticket -- I'm not sure if updating wp_dropdown_users() is within scope or not, there is an open ticket here #19867 so probably not.

Instead of doing the check on wp_is_large_user_count() in this patch: https://github.com/WordPress/wordpress-develop/pull/2493/files , take out both checks there (ie the patch and the reason for it). Then commit this and work on the wp_dropdown_users() ticket above.

I think but am not completely 100% sure that that's the only place wp_is_large_user_count() is used to conditionally load wp_dropdown_users() but if there are more in this patch, remove those as well.

#92 @spacedmonkey
2 years ago

In 53049:

Users: Fix notice error in WP_Posts_List_Table class.

Fix notice error introduced in [53011] as the variable $authors_dropdown is now conditionally loaded.

Follow-up to [53011].

Props Spacedmonkey, dd32, johnbillion.
See #38741.

This ticket was mentioned in Slack in #meta by ocean90. View the logs.


2 years ago

#94 follow-up: @georgestephanis
2 years ago

@spacedmonkey Following [53049] in #meta it was reported that this can change the author of a post.

Should we add an else condition that just injects a hidden field to preserve the existing author in cases where the dropdown doesn't show?

Last edited 2 years ago by georgestephanis (previous) (diff)

#95 in reply to: ↑ 94 @tobifjellner
2 years ago

Replying to georgestephanis:

@spacedmonkey Following [53049] in #meta it was reported that this can change the author of a post.

Should we add an else condition that just injects a hidden field to preserve the existing author in cases where the dropdown doesn't show?

Link to discussion in Slack: https://wordpress.slack.com/archives/C02QB8GMM/p1648818054123939

#96 @peterwilsoncc
2 years ago

  • Keywords needs-patch added; has-patch removed

Keyword updates to keep the reports clean.

@spacedmonkey do you have bandwidth to follow this up this week? HTML unseen, George's suggestion of a hidden field make sense to me.

#97 @spacedmonkey
2 years ago

I am currently on holiday for the next 2 weeks. So have limited to time look into anything.

I agree that the hidden form field sounds reasonable but don’t understand why it is needed.

Can either @peterwilsoncc or @johnbillion can a look into this?

#98 @peterwilsoncc
2 years ago

In 38741.12.diff I've followed @georgestephanis's suggestion of a hidden field.

As inline-edit-post.js is hard coded to expect a <select/> for the author input, I've added the element with a display:none !important.

With an input tag, the javascript was setting up the code <input type='hidden' ...><option /></input>. This seems more sub-optimal than a hiding the select via CSS.

This ticket was mentioned in PR #2520 on WordPress/wordpress-develop by spacedmonkey.


2 years ago
#99

  • Keywords has-patch added; needs-patch removed

#100 @spacedmonkey
2 years ago

Create a quick patch myself, that seems to work. #2520.

Either solution is just a simple workaround, to add a hidden select tag.

#101 follow-up: @jb510
2 years ago

  • Keywords needs-patch added; has-patch removed

There are some plugin compatibility items to consider with this fix. The original aim of this ticket was to "Speed up the user screen" and queries of users on large sites.

I'm only reading the patches (I don't have a trunk setup at the moment) but it seems like this would completely bypass wp_dropdown_users. This seems very problematic.

It's not like people don't need to change the post author on large sites, it's just that it doing makes an expensive query and creates a _massive_ select element.

We've mostly moved to a version of this: https://github.com/WebDevStudios/WDS-Dynamic-Dropdowns to move that to ajax on large sites which relies on wp_dropdown_users.

Sorry if I'm off base here, will try to test patches when I can set up trunk again.

#102 in reply to: ↑ 101 @peterwilsoncc
2 years ago

  • Keywords has-patch added; needs-patch removed

Replying to jb510:

I'm only reading the patches (I don't have a trunk setup at the moment) but it seems like this would completely bypass wp_dropdown_users. This seems very problematic.

It's not like people don't need to change the post author on large sites, it's just that it doing makes an expensive query and creates a _massive_ select element.

At the moment, the authors are only removed from the quick edit page. The option to change them remains on both the block and classic editor screens.

The block editor uses the REST API to populate the dropdown.

To test, my plugin was add_filter( 'wp_is_large_user_count', '__return_true' );

Sorry if I'm off base here, will try to test patches when I can set up trunk again.

Please don't apologize for asking questions or raising concerns.

It prompted me to do additional manual testing.

#103 @jb510
2 years ago

ty @peterwilsoncc - When I was involved with this ticket Gutenberg didn't exist, lol. It's always been primarily about the classic editor author meta box to me, or more generally anywhere that user query occurs. Anyway, thank you for addressing my questions.

This ticket was mentioned in Slack in #polyglots by amieiro. View the logs.


2 years ago

#105 @peterwilsoncc
2 years ago

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

In 53105:

Users: Prevent author changes in bulk editor on large sites.

On large sites (with over 1000 users), include a hidden post_author field in the bulk editing interface to preven unexpected authorship changes.

Follow up to [53011], [53049].

Props georgestephanis, tobifjellner, peterwilsoncc, spacedmonkey, jb510.
Fixes #38741.

#107 @OllieJones
2 years ago

I have just completed work on a plugin for handling large numbers of users. https://wordpress.org/plugins/index-wp-users-for-speed/

It works by putting wp_usermeta tags with keys like wp_iufsr:administrator and wp_uifsr:subscriber on each user (it does so in a series of wp-cron jobs). It then hooks WP_Query_Users operations and replaces the slow and unsargeable

meta_key = 'wp_capabilities' and meta_value LIKE '%"administrator"%'

query pattern with a much faster

meta_key = 'wp_iufsr:administrator'

pattern that exploits indexes. It does the right thing for multisite, for example by using 'wp_4_iufsr:administrator' for site 4.

It accelerates the Users page and fetching the lists of editors for the Posts, Pages, and Gutenberg editor AJAX operation.

It does not change any existing user CRUD operations, but simply adds this extra metadata and hooks queries.

Please give it a try if you're stymied by a slow dashboard on your site with many users.

As far as fixing this problem in core goes: the root cause of the slowness is the difficulty of using MariaDB / MySQL to parse out the role meta_values in wp_capabilities keys. Giving each role its own meta_key, with an empty meta_value, makes the DBMS able to search for roles quickly.

Last edited 2 years ago by OllieJones (previous) (diff)

#108 @spacedmonkey
23 months ago

  • Keywords needs-dev-note added

I have a dev note ready for review.

Friends ping to @flixos90 and @peterwilsoncc who are component maintainers, to take a look to see if they are happy.

Note: See TracTickets for help on using tickets.