WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 3 years ago

#18536 closed task (blessed)

Improve performance of WP_Query core — at Version 19

Reported by: cheald Owned by:
Milestone: 3.4 Priority: normal
Severity: normal Version:
Component: Performance Keywords:
Focuses: Cc:

Description (last modified by nacin)

WP_Query, by default, selects wp_posts.* when building post queries. This can result in extremely large result sets when used with a large wp_posts table, resulting in MySQL being forced to use a temp table for sorting, which leads to all sorts of performance problems. SELECT * is generally considered to be a code smell as is, and is commonly known to cause problems at scale.

This patch breaks default WP_Query queries into two parts - one to select a list of IDs with the given selection criteria, and a second to actually select the posts once the IDs have been determined. This vastly reduces the amount of data that has to be processed by the database, resulting in fewer context switches and vastly improved performance.

I have implemented this in a production system with a wp_posts table with over 450k rows and reduced query times by two orders of magnitude. Database CPU burn is down by 90% and context switches have been drastically reduced.

This should be of some benefit even in small cases, since smaller datasets will result in faster sorts. The overhead of the second query is minimal. In large cases, the performance benefits range from "good" to "dramatic".

I have run the Wordpress PHPUnit test suite against the patch, and it did not break any tests. The suite, when I checked it out, had multiple failures present, but no new ones were introduced by this change.

Change History (21)

@cheald4 years ago

Query performance patch

comment:1 @nacin4 years ago

Impressive. Related: #10964.

Unfortunately, not many of our unit tests cover WP_Query. After some bad experiences in version 3.1 when we added taxonomy queries, we're not likely to significantly alter WP_Query again (for both this ticket and #10964, among otherS) until we have them. Building some unit tests for WP_Query would be quite the undertaking, but it'd be one that would move along tickets like this.

That said, this does look relatively simple. The main concern would be seeing how plugins that hook deeply into WP_Query might be affected.

Another concern would be figuring out where old and new intersect with regards to performance. I imagine for simple queries or smaller datasets, the current version would be more performant.

What's one of your queries that isn't performing well? If you're doing an interesting join or sorting or querying by an unindexed field, you might be doing a tablesort that you should work to avoid. Can we see some EXPLAIN results?

comment:2 @cheald4 years ago

The problem is indeed the tablesorts. This patch helps keep our queries out of tablesort territory by reducing the amount of data that the DB has to chew on. Here's a chunk from our slow query log:

# User@Host: prod[prod] @  [192.168.100.162]
# Thread_id: 213317196  Schema: prod  Last_errno: 0  Killed: 0
# Query_time: 0.542673  Lock_time: 0.000094  Rows_sent: 2  Rows_examined: 47995  Rows_affected: 0  Rows_read: 9108
# Bytes_sent: 10754  Tmp_tables: 1  Tmp_disk_tables: 1  Tmp_table_sizes: 50918492
# InnoDB_trx_id: 676FA764
# QC_Hit: No  Full_scan: No  Full_join: No  Tmp_table: Yes  Tmp_table_on_disk: Yes
# Filesort: Yes  Filesort_on_disk: No  Merge_passes: 0
#   InnoDB_IO_r_ops: 0  InnoDB_IO_r_bytes: 0  InnoDB_IO_r_wait: 0.000000
#   InnoDB_rec_lock_wait: 0.000000  InnoDB_queue_wait: 0.000000
#   InnoDB_pages_distinct: 10030
SET timestamp=1302277759;
SELECT SQL_CALC_FOUND_ROWS  wp_posts.* FROM wp_posts  INNER JOIN wp_term_relationships ON (wp_posts.ID = wp_term_relationships.object_id) INNER JOIN wp_term_taxonomy ON (wp_term_relationships.term_taxonomy_id = wp_term_taxonomy.term_taxonomy_id)  WHERE 1=1  AND wp_posts.ID NOT IN (332000,583467,583371,579313,583657,583185) AND wp_term_taxonomy.taxonomy = 'post_tag'  AND wp_term_taxonomy.term_id IN ('1862', '82', '19567', '553', '135', '43', '1174', '1100', '5819', '16796', '119', '74')  AND wp_posts.post_type = 'post' AND (wp_posts.post_status = 'publish') GROUP BY wp_posts.ID ORDER BY wp_posts.post_date DESC LIMIT 0, 2;

And here's the EXPLAIN:

+----+-------------+-----------------------+--------+--------------------------------------+------------------+---------+----------------------------------------+------+-----------------------------------------------------------+
| id | select_type | table                 | type   | possible_keys                        | key              | key_len | ref                                    | rows | Extra                                                     |
+----+-------------+-----------------------+--------+--------------------------------------+------------------+---------+----------------------------------------+------+-----------------------------------------------------------+
|  1 | SIMPLE      | wp_term_taxonomy      | range  | PRIMARY,term_id_taxonomy,taxonomy    | term_id_taxonomy | 106     | NULL                                   |   12 | Using where; Using index; Using temporary; Using filesort |
|  1 | SIMPLE      | wp_term_relationships | ref    | PRIMARY,term_taxonomy_id             | term_taxonomy_id | 8       | prod.wp_term_taxonomy.term_taxonomy_id |   15 | Using index                                               |
|  1 | SIMPLE      | wp_posts              | eq_ref | PRIMARY,post_status,type_status_date | PRIMARY          | 8       | prod.wp_term_relationships.object_id   |    1 | Using where                                               |
+----+-------------+-----------------------+--------+--------------------------------------+------------------+---------+----------------------------------------+------+-----------------------------------------------------------+

Nothing too fancy going on there,
The really interesting thing there is the temp table size - 50,918,492 bytes - just to get 2 posts! "Tmp_table_on_disk" explains the performance pretty easily - making the context switch to write the temp table isn't trivial!

Here's another:

# User@Host: prod[prod] @  [192.168.100.162]
# Thread_id: 213303997  Schema: prod  Last_errno: 0  Killed: 0
# Query_time: 0.909574  Lock_time: 0.000117  Rows_sent: 2  Rows_examined: 33733  Rows_affected: 0  Rows_read: 6785
# Bytes_sent: 11761  Tmp_tables: 1  Tmp_disk_tables: 1  Tmp_table_sizes: 40452708
# InnoDB_trx_id: 676ED8D4
# QC_Hit: No  Full_scan: No  Full_join: No  Tmp_table: Yes  Tmp_table_on_disk: Yes
# Filesort: Yes  Filesort_on_disk: No  Merge_passes: 0
#   InnoDB_IO_r_ops: 0  InnoDB_IO_r_bytes: 0  InnoDB_IO_r_wait: 0.000000
#   InnoDB_rec_lock_wait: 0.000000  InnoDB_queue_wait: 0.000000
#   InnoDB_pages_distinct: 7863
SET timestamp=1302277444;
SELECT SQL_CALC_FOUND_ROWS  wp_posts.* FROM wp_posts  INNER JOIN wp_term_relationships ON (wp_posts.ID = wp_term_relationships.object_id) INNER JOIN wp_term_taxonomy ON (wp_term_relationships.term_taxonomy_id = wp_term_taxonomy.term_taxonomy_id)  WHERE 1=1  AND wp_posts.ID NOT IN (249519,583467,583371,579313,583657,583185) AND wp_term_taxonomy.taxonomy = 'post_tag'  AND wp_term_taxonomy.term_id IN ('20747', '2009', '82', '131', '553', '135', '1156', '732', '1100', '119')  AND wp_posts.post_type = 'post' AND (wp_posts.post_status = 'publish') GROUP BY wp_posts.ID ORDER BY wp_posts.post_date DESC LIMIT 0, 2;

Explained:

+----+-------------+-----------------------+--------+--------------------------------------+------------------+---------+----------------------------------------+------+-----------------------------------------------------------+
| id | select_type | table                 | type   | possible_keys                        | key              | key_len | ref                                    | rows | Extra                                                     |
+----+-------------+-----------------------+--------+--------------------------------------+------------------+---------+----------------------------------------+------+-----------------------------------------------------------+
|  1 | SIMPLE      | wp_term_taxonomy      | range  | PRIMARY,term_id_taxonomy,taxonomy    | term_id_taxonomy | 106     | NULL                                   |   10 | Using where; Using index; Using temporary; Using filesort |
|  1 | SIMPLE      | wp_term_relationships | ref    | PRIMARY,term_taxonomy_id             | term_taxonomy_id | 8       | prod.wp_term_taxonomy.term_taxonomy_id |   15 | Using where; Using index                                  |
|  1 | SIMPLE      | wp_posts              | eq_ref | PRIMARY,post_status,type_status_date | PRIMARY          | 8       | prod.wp_term_relationships.object_id   |    1 | Using where                                               |
+----+-------------+-----------------------+--------+--------------------------------------+------------------+---------+----------------------------------------+------+-----------------------------------------------------------+

All the patch does is reduce the amount of data that MySQL has to manage to perform the sort - with small dataset sizes, this isn't likely to have much of an impact, but once the size of your potential resultset (with included post content) passes your query into filesort territory, the differences become *big*.

comment:3 @DrewAPicture4 years ago

  • Cc xoodrew@… added

comment:4 @scribu4 years ago

I think the major problem here isn't unit tests for WP_Query args, but plugin compatibility. There are a lot of plugins that use the SQL filters - posts_fields, posts_where etc.

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

comment:5 @scribu4 years ago

  • Keywords close added

Speaking of those filters, it's possible to use them to achieve the behavior you want without modifying Core. See #10964 for an example of this.

As such, suggest closing as duplicate.

comment:7 @cheald4 years ago

One possible solution might be as simple as a config switch; the administrator could be warned that the switch may break some plugins. It's certainly not a perfect solution, but for people with large installs, it might be a pretty significant quality-of-experience improvement.

My first draft of this fix implemented query rewrites as a plugin using the posts_request and posts_results filters, but it's a hack of a solution, and seems like the sort of thing that belongs in the core anyhow. https://github.com/cheald/wp_fasterquery/blob/master/filter.php is the plugin, but it's very clearly tailored specifically to our needs and install, and may not work well for others.

While it's possible to fix this with a plugin, it feels like the fact that it can happen at all is a somewhat self-limiting design issue that could use some more formal resolution. If the team disagrees, that's certainly fine, but I felt I'd be remiss if I didn't at least make an attempt! :)

comment:8 @aaroncampbell4 years ago

  • Cc aaroncampbell added

comment:9 @anointed4 years ago

  • Cc anointed added

@ryan4 years ago

comment:10 @ryan4 years ago

18536.diff modifies $fields after the posts_fields filter runs and avoids the second query if the first comes back empty.

comment:11 @ryan4 years ago

Some caching plugins return an empty query string from the posts_request filter. With the current query code, this causes get_results() to return null and skip the query. The plugin then fills in the posts array by fetching the posts from cache and returning them through the posts_results filter. This fails with 18536.diff applied. The get_col() call in the expand_ids block receives an empty query string. get_col() uses last_result when an empty query is passed. This results in some totally whacky stuff ending up in the SELECT * query in the expand IDs block.

comment:12 @ryan4 years ago

A quick fix would be to save the query string before calling the filter and use that with get_col() if the filtered query string is empty. But, who knows what other back compat problems await. I'd still like to give this a go, but I think we have hold off until 3.4 to give plugin authors time to test and accommodate.

comment:13 follow-up: @scribu4 years ago

  • Keywords close removed

So this is basically what was suggested in #10964 i.e. split the query in two, right?

PS: apply_filters_ref_array() is not necessary there. Yay for PHP5.

comment:14 in reply to: ↑ 13 @ryan4 years ago

Replying to scribu:

So this is basically what was suggested in #10964 i.e. split the query in two, right?

Yes. That ticket is all over the place though.

If there's a hook registered for posts_request, then don't do the expand_ids block. Create two new request filters for the two queries in the expand_ids block. That way plugins have a clear way forward. Sort of like Denis' suggestion in 10964, but I think new filters will handle back compat better than adding a flag to posts_request.

comment:15 @scribu3 years ago

Another patch was submitted in #19608

The problem was again the temporary table size.

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

comment:16 @scribu3 years ago

  • Component changed from Query to Performance
  • Keywords early added

comment:17 @scribu3 years ago

  • Milestone changed from Awaiting Review to 3.4

I think it's pretty obvious that we should give this a try for WP 3.4.

comment:18 @scribu3 years ago

With regards to back-compat, I agree with ryan that it's better to not fire some of the old filters anymore and introduce new filters. That way, if a plugin wants to be compatible with WP < 3.4, it can just keep the old callbacks without any modification and add new code for WP 3.4+.

comment:19 @nacin3 years ago

  • Description modified (diff)

Removing the screenshot (dead link). I do remember the graph looking mighty impressive, though.

Note: See TracTickets for help on using tickets.