Skip to:
Content

Opened 10 months ago

Last modified 3 months ago

#7442 new enhancement

Use JOIN rather than Subquery on user search

Reported by: brandonliles Owned by:
Milestone: 3.0 Priority: normal
Severity: normal Version:
Component: Core Keywords: has-patch dev-feedback early
Cc:

Description

The user search relies on two subqueries. This is a known weakness of MySQL performance. Using joins will improve performance.

Attachments (3)

bp-search-use-joins.patch (2.2 KB) - added by brandonliles 10 months ago.
bp-search-use-joins2.patch (2.0 KB) - added by brandonliles 9 months ago.
bp-search-optimize.patch (2.3 KB) - added by brandonliles 7 months ago.
Subquery Materialization Approach

Download all attachments as: .zip

Change History (13)

#1 @r-a-y
10 months ago

Thanks for creating this ticket and for the patch, @brandonliles!

However, the patch is breaking multiple unit tests.

If you are able to, can you set up WordPress for PHP unit testing?
https://make.wordpress.org/core/handbook/testing/automated-testing/phpunit/

Once you have done so, navigate to the buddypress directory and run phpunit in a command prompt. Then, you will be able to see what testcases are breaking.

If you need further help in setting up PHPUnit, let us know.

#2 @brandonliles
9 months ago

Thanks for the feedback @r-a-y just uploaded a second version of the patch that doesn't break stuff :)

#3 @hnla
7 months ago

  • Keywords has-patch dev-feedback added
  • Milestone changed from Awaiting Review to 2.9

@brandonliles
7 months ago

Subquery Materialization Approach

#4 @brandonliles
7 months ago

After seeing the query still perform poorly on a site I took another look, although joins can improve performance, MySQL still tends to look at too many rows in the bp_activity table. Determining the matching user_ids first and plugging those values into the query is a major performance improvement.

New patch bp-search-optimize.patch added.

Last edited 7 months ago by brandonliles (previous) (diff)

This ticket was mentioned in Slack in #buddypress by hnla. View the logs.


7 months ago

#6 @r-a-y
7 months ago

Thanks for the new patch, @brandonliles.

bp-search-optimize.patch passes existing unit tests. The rationale behind the "query for IDs first" approach makes sense to me.

Looks good, but needs sign off from a lead dev. We might need some new unit tests to cover this change though.

Last edited 7 months ago by r-a-y (previous) (diff)

This ticket was mentioned in Slack in #buddypress by hnla. View the logs.


7 months ago

This ticket was mentioned in Slack in #buddypress by r-a-y. View the logs.


6 months ago

#9 @hnla
5 months ago

  • Keywords early added
  • Milestone changed from 2.9 to 3.0

#10 @boonebgorges
3 months ago

@brandonliles Thanks for the ticket and for your patches!

For some background on your original suggestion: We intentionally converted these (and other) user-related queries to subqueries a number of years ago. BuddyPress must remain compatible with configurations where the wp_users table does not live in the same database - or even the same server - as wp_bp_xprofile_data. See eg https://buddypress.trac.wordpress.org/ticket/4060#comment:25

Separating the subquery into a separate $wpdb->get_col() seems like an OK move to me. It'd be even better if we could use this opportunity to wrap a layer of caching around the search. Do you happen to have any benchmarks related to the performance improvements you've mentioned?

I think we have sufficient test coverage for this kind of change. I can't think of a situation where the suggested change would break something that wouldn't be detected by the tests we've already got.

Note: See TracTickets for help on using tickets.