BP_Groups_Group::get_total_member_count() is highly inefficient, and runs out of memory

[dd32]

When adding/removing a user to a group, BP_Groups_Member::refresh_total_member_count_for_group() is called which is designed to update the member count for the group.

It quickly calls BP_Groups_Group::get_total_member_count() only for it to then call groups_get_group_members() which loads all the WP_User objects and XProfile data for those users. This quickly exhausts all PHP memory.

On WordPress.org, we've run into this with a group of 50,000 members. Although I suspect we ran past this limitation back at 40k users.

The resulting SQL query generated by BP_Group_Member_Query to fetch & order the WP_User objects is over 800,000 characters long..

A PR is attached, which is a bit hacky IMHO, but seemingly does the job by avoiding the WP_User querying, but it's done by using a non-standard and quirks of BP_Group_Member_Query. Ideally, perhaps the code path for "Just get me a count of the users in this group" could be optimised within BP_Group_Member_Query.

Trac ticket: ​https://buddypress.trac.wordpress.org/ticket/8688

[dd32]

I would like to include the fatal error being encountered after calling groups_join_group() or groups_leave_group()... but it's really not exactly obvious.. so I'll include a partial stack trace instead that might help understanding of it.

For those with access to BuddyPress.org fatal errors, you'll find similar errors to the below in there, but the details is not at all helpful for debugging.

E_ERROR: Allowed memory size of 268435456 bytes exhausted (tried to allocate 19846800 bytes)

...

PHP  16. groups_leave_group($group = 17, $user_id = 14842180)
PHP  17. BP_Groups_Member::delete($user_id = 14842180, $group_id = 17) wp-content/plugins/buddypress/bp-groups/bp-groups-functions.php:628
PHP  18. BP_Groups_Member::refresh_total_member_count_for_group($group_id = 17) wp-content/plugins/buddypress/bp-groups/classes/class-bp-groups-member.php:521
PHP  19. BP_Groups_Group::get_total_member_count($group_id = 17, $skip_cache = TRUE) wp-content/plugins/buddypress/bp-groups/classes/class-bp-groups-member.php:489
PHP  20. groups_get_group_members($args = ['group_id' => 17, 'group_role' => [0 => 'member', 1 => 'admin', 2 => 'mod'], 'type' => 'active']) wp-content/plugins/buddypress/bp-groups/classes/class-bp-groups-group.php:1800
PHP  21. BP_User_Query->__construct($query = ['group_id' => 17, 'per_page' => FALSE, 'page' => FALSE, 'group_role' => [0 => 'member', 1 => 'admin', 2 => 'mod'], 'exclude' => FALSE, 'search_terms' => FALSE, 'type' => 'active']) wp-content/plugins/buddypress/bp-groups/bp-groups-functions.php:874
PHP  22. BP_User_Query->do_wp_user_query() wp-content/plugins/buddypress/bp-core/classes/class-bp-user-query.php:210
PHP  23. WP_User_Query->__construct($query = ['fields' => [0 => 'ID', 1 => 'user_login', 2 => 'user_pass', 3 => 'user_nicename', 4 => 'user_email', 5 => 'user_url', 6 => 'user_registered', 7 => 'user_activation_key', 8 => 'user_status', 9 => 'display_name', 10 => 'spam', 11 => 'deleted'], 'include' => [0 => '10937674', 1 => '15013885', 2 => '15536580', 3 => '13255989', ...], 'blog_id' => 0, 'count_total' => FALSE]) wp-content/plugins/buddypress/bp-core/classes/class-bp-user-query.php:594
PHP  24. WP_User_Query->query() wp-includes/class-wp-user-query.php:78
PHP  25. wpdb->get_results($query = 'SELECT  wp_users.ID, wp_users.user_login, wp_users.user_pass, wp_users.user_nicename, wp_users.user_email, wp_users.user_url, wp_users.user_registered, wp_users.user_activation_key, wp_users.user_status, wp_users.display_name, wp_users.spam, wp_users.deleted FROM  wp_users WHERE 1=1 AND  wp_users.ID IN (10937674,15013885,15536580,'..., $output = *uninitialized*) wp-includes/class-wp-user-query.php:793

[dd32]

Also: I realise this optimisation probably skips a whole bunch of filters, especially where someone is using meta and passing that's where adding just a return count from the class could maybe be beneficial, but I was running into all kinds of trouble trying that.

On WordPress.org, I'll probably just trigger bp_use_legacy_group_member_query and use that query instead; as it will produce a much more direct path for counting group members: ​https://github.com/buddypress/buddypress/blob/00f15df9f6753b340d5ded1baf7ef22ec0c76ebb/src/bp-groups/classes/class-bp-groups-member.php#L1259

[imath]

Hi @dd32

Thanks a lot for your feedbacks, investigation and patch. I agree this is something we need to improve.

I’ll look at it asap and we’ll include the fix into next minor release.

[imath]

I see this is something we've modified during the 10.0.0 dev cycle in [13103] see: ​https://buddypress.trac.wordpress.org/changeset/13103/trunk/src/bp-groups/classes/class-bp-groups-group.php

Before this change we were using a more straightforward query to get the count, so I believe there shouldn't be any downside with potential filters/meta data there.

[imath]

​8688.patch is my attempt to address Boone's concerns in #7614 and try to optimise the group members count query.

• We need to validate group members are existing into $wpdb->users.
• We need to make sure group members are active users (validated their account and logged once).

@dd32 if it's possible for you to test the patch to see if it's better on profiles.w.org, that would be cool.

[dd32]

Replying to imath:

dd32 if it's possible for you to test the patch to see if it's better on profiles.w.org, that would be cool.

Running 8688.patch​ shows it's better, not amazing, but at least it now completes in seconds rather than minutes.

I initially ran some custom scripts for benchmarking, which were marginally faster than these builtin BP commands, this is with the 50k member group (smaller groups are amazingly fast)

Original (similar for removal, this is CLI though so it's got 1G of memory to play with, and thus doesn't out-of-memory, unlike web requests)

$ time wp --url=https://profiles.wordpress.org/ bp group member add --group-id=17 --user-id=14842180
Success: Added user #14842180 to group #17 as member.

real	1m56.373s
user	0m37.416s
sys	0m24.400s

Patched:

$ time wp --url=https://profiles.wordpress.org/ bp group member add --group-id=17 --user-id=14842180
Success: Added user #14842180 to group #17 as member.

real	0m16.247s
user	0m6.788s
sys	0m2.484s

$ time wp --url=https://profiles.wordpress.org/ bp group member remove --group-id=17 --user-id=14842180
Success: Member #14842180 removed from the group #17.

real	0m14.470s
user	0m6.936s
sys	0m2.344s

It's obviously much faster for smaller groups:

$ time wp --url=https://profiles.wordpress.org/ bp group member add --group-id=55 --user-id=14842180
Success: Added user #14842180 to group #55 as member.

real	0m1.696s
user	0m1.220s
sys	0m0.412s

$ time wp --url=https://profiles.wordpress.org/ bp group member remove --group-id=55 --user-id=14842180
Success: Member #14842180 removed from the group #55.

real	0m1.673s
user	0m1.284s
sys	0m0.336s

When adding a single user, this is great, but when calling the function repeatedly I suspect it'll cause the re-counting to be run for every user. It might be worth considering something like wp_defer_term_counting(), but it might also be reasonable to just leave that up to plugins / scripts that are adding/removing many users.

[imath]

Hi @dd32

Thanks a lot for your very interesting feedback. I agree we should only count once in case of running a batch of groups_join_group(). We could enjoy this improvement in BuddyPress as we can add a list of users to a group from the WP Admin/Group screen.

I'm going to look at it.

[imath]

In ​8688.2.patch, I'm introducing the bp_groups_defer_group_members_count() function. It can be used before and after batch adding/removing user to/from a group. When it's the case, the group members count will happen only once instead of each time a user is added/removed to/from a group.

I've added a place where it's used into the plugin's code at the very beginning of the patch.

[dd32]

Replying to imath:

In ​8688.2.patch, I'm introducing the bp_groups_defer_group_members_count() function.

I haven't tested it, but from reading the code and how it's implemented, this looks great to me!

[dd32]

Replying to dd32:

but from reading the code and how it's implemented, this looks great to me!

Some thoughts though:

• It's a little strange that bp_groups_defer_group_members_count() doesn't call bp_groups_update_group_members_count() directly, when that's the function it's hooking back on. I can see from the code that it's just a wrapper, but it still feels odd.
• ( true === $groups_member instanceof BP_Groups_Member ) seems a little redundant, the true === part I mean; instanceof seems to be enough of a comparator to use there?
• bp_groups_defer_group_members_count( $group_id, true ); to defer counting, where it affects all group operation and not just the group_id passed is a little odd. I'd expect either just:
  • defer: `bp_groups_defer_group_members_count( true );
  • post-defer: bp_groups_defer_group_members_count( false, $group_id );
    • or bp_groups_defer_group_members_count( false ) and it to have known which $group_ids need reprocessing (which should be a singular group, but technically could've been more, maybe?)

Closing in favour of patches on ticket.

[imath]

Thanks for your feedback @dd32 👍.

I've applied most of your suggestions except for the post-defer part. If on the front end we could use bp_get_current_group_id() to guess the group id, the only place where we are using it: WP Admin Group Screen we are not setting the $bp->current_group's global.

I think only doing a count if the Group ID is provided, leaves the ability to developers wishing to update the members count for multiple groups to only use bp_groups_defer_group_members_count( false ) then use a custom groups loop to update their counts.

[dd32]

Replying to imath:

I've applied most of your suggestions except for the post-defer part.

All good by me! Thanks for working on this, it's muchly appreciated!

[imath]

In 13280:

Improve group members count query performance

[13103] introduced a change in 10.0.0 that can be very time consuming when a group has a lot of members.

To keep the main improvements of the referenced commit (only refreshing group members count when a user joins or leaves a group) but optimize queries performance, we are introducing a new way to count group members in the BP_Group_Member_Query.

We are also introducing a way to defer group members count when adding a batch of members to a group. Using bp_groups_defer_group_members_count() avoids to refresh the count each time a member of this batch is added. For more information about how to use this function, you can have a look at how BuddyPress is using it into src/bp-groups/bp-groups-admin.php.

Props dd32, espellcaste

See #8688 (trunk)

[imath]

In 13281:

Improve group members count query performance

[13103] introduced a change in 10.0.0 that can be very time consuming when a group has a lot of members.

To keep the main improvements of the referenced commit (only refreshing group members count when a user joins or leaves a group) but optimize queries performance, we are introducing a new way to count group members in the BP_Group_Member_Query.

We are also introducing a way to defer group members count when adding a batch of members to a group. Using bp_groups_defer_group_members_count() avoids to refresh the count each time a member of this batch is added. For more information about how to use this function, you can have a look at how BuddyPress is using it into src/bp-groups/bp-groups-admin.php.

Props dd32, espellcaste

Fixes #8688 (branch 10.0)