WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 11 months ago

#37094 closed enhancement (fixed)

wp_get_nav_menu_items should use tax_query

Reported by: spacedmonkey Owned by: boonebgorges
Milestone: 4.9 Priority: normal
Severity: normal Version: 3.0
Component: Menus Keywords: has-patch 2nd-opinion
Focuses: performance Cc:

Description

When the wp_get_nav_menu_items function was introduced in 3.0, taxonomy query were not in core. However, as taxonomy queries are now in core, it removing the need to call get_objects_in_term. Removing this call to get_objects_in_term, is great, as that function is unfilterable and uncached.

It also means that any improves to the taxonomy queries, such as caching or filters could also be put into play here.

Attachments (8)

37094.patch (1.3 KB) - added by spacedmonkey 2 years ago.
37094.2.patch (1.3 KB) - added by spacedmonkey 12 months ago.
Screen Shot 2017-05-29 at 19.18.49.png (169.2 KB) - added by spacedmonkey 12 months ago.
Screen Shot 2017-05-29 at 18.36.06.png (25.6 KB) - added by spacedmonkey 12 months ago.
Screen Shot 2017-05-29 at 18.24.08.png (216.7 KB) - added by spacedmonkey 12 months ago.
Screen Shot 2017-05-29 at 18.36.06.2.png (25.6 KB) - added by spacedmonkey 12 months ago.
Screen Shot 2017-05-29 at 18.35.56.png (13.3 KB) - added by spacedmonkey 12 months ago.
37094.diff (3.6 KB) - added by boonebgorges 11 months ago.

Download all attachments as: .zip

Change History (17)

@spacedmonkey
2 years ago

#1 @spacedmonkey
2 years ago

  • Keywords has-patch needs-unit-tests added
  • Version set to 3.0

#2 @moonomo
2 years ago

Thanks for the ticket- just found this while digging the Query log. +1 for using tax_query instead of using get_objects_in_term.

#3 @ocean90
20 months ago

Queries before:

SELECT tr.object_id FROM wp_term_relationships AS tr INNER JOIN wp_term_taxonomy AS tt ON tr.term_taxonomy_id = tt.term_taxonomy_id WHERE tt.taxonomy IN ('nav_menu') AND tt.term_id IN ('2') ORDER BY tr.object_id ASC

+----+-------------+-------+-------+-----------------------------------+------------------+---------+-------------+------+----------+--------------------------+
| id | select_type | table | type  | possible_keys                     | key              | key_len | ref         | rows | filtered | Extra                    |
+----+-------------+-------+-------+-----------------------------------+------------------+---------+-------------+------+----------+--------------------------+
|  1 | SIMPLE      | tt    | const | PRIMARY,term_id_taxonomy,taxonomy | term_id_taxonomy | 138     | const,const |    1 |   100.00 | Using index              |
|  1 | SIMPLE      | tr    | ref   | term_taxonomy_id                  | term_taxonomy_id | 8       | const       |    4 |   100.00 | Using where; Using index |
+----+-------------+-------+-------+-----------------------------------+------------------+---------+-------------+------+----------+--------------------------+

SELECT wp_posts.* FROM wp_posts WHERE 1=1 AND wp_posts.ID IN (6,23,160,290) AND wp_posts.post_type = 'nav_menu_item' AND ((wp_posts.post_status = 'publish')) ORDER BY wp_posts.menu_order ASC

+----+-------------+---------------+------+--------------------------+------------------+---------+-------------+------+----------+-----------------------------+
| id | select_type | table         | type | possible_keys            | key              | key_len | ref         | rows | filtered | Extra                       |
+----+-------------+---------------+------+--------------------------+------------------+---------+-------------+------+----------+-----------------------------+
|  1 | SIMPLE      | wp_posts      | ref  | PRIMARY,type_status_date | type_status_date | 164     | const,const |    4 |   100.00 | Using where; Using filesort |
+----+-------------+---------------+------+--------------------------+------------------+---------+-------------+------+----------+-----------------------------+

Queries with 37094.patch:

SELECT term_taxonomy_id FROM wp_term_taxonomy WHERE taxonomy = 'nav_menu' AND term_id IN (2)

+----+-------------+-----------------------+-------+---------------------------+------------------+---------+-------------+------+----------+-------------+
| id | select_type | table                 | type  | possible_keys             | key              | key_len | ref         | rows | filtered | Extra       |
+----+-------------+-----------------------+-------+---------------------------+------------------+---------+-------------+------+----------+-------------+
|  1 | SIMPLE      | wp_term_taxonomy      | const | term_id_taxonomy,taxonomy | term_id_taxonomy | 138     | const,const |    1 |   100.00 | Using index |
+----+-------------+-----------------------+-------+---------------------------+------------------+---------+-------------+------+----------+-------------+

SELECT wp_posts.* FROM wp_posts LEFT JOIN wp_term_relationships ON (wp_posts.ID = wp_term_relationships.object_id) WHERE 1=1 AND ( wp_term_relationships.term_taxonomy_id IN (2) ) AND wp_posts.post_type = 'nav_menu_item' AND ((wp_posts.post_status = 'publish')) GROUP BY wp_posts.ID ORDER BY wp_posts.menu_order ASC

+----+-------------+----------------------------+--------+--------------------------+------------------+---------+-------------------------------------------------+------+----------+-----------------------------------------------------------+
| id | select_type | table                      | type   | possible_keys            | key              | key_len | ref                                             | rows | filtered | Extra                                                     |
+----+-------------+----------------------------+--------+--------------------------+------------------+---------+-------------------------------------------------+------+----------+-----------------------------------------------------------+
|  1 | SIMPLE      | wp_term_relationships      | ref    | PRIMARY,term_taxonomy_id | term_taxonomy_id | 8       | const                                           |    4 |   100.00 | Using where; Using index; Using temporary; Using filesort |
|  1 | SIMPLE      | wp_posts                   | eq_ref | PRIMARY,type_status_date | PRIMARY          | 8       | wp_develop.wp_term_relationships.object_id      |    1 |   100.00 | Using where                                               |
+----+-------------+----------------------------+--------+--------------------------+------------------+---------+-------------------------------------------------+------+----------+-----------------------------------------------------------+

I'm not really a MySQL expert but Using temporary; Using filesort sounds bad.

#4 @spacedmonkey
12 months ago

I have done some profiling and I updated the patch.

@ocean90 I didn't manage to replicate your error message. I don't see how there is a problem, tax_query is a well tested part of core. The only reason it didn't use tax_query when this function was created, was because it didn't exists.

I have uploaded some screenshots from my profiling. It shows the change in queries generated. The new patch, results in one less query per menu call. By passing the term_taxonomy_id, the tax_query doesn't need to look anything up in the database, resulting in one less query.

#5 @boonebgorges
11 months ago

  • Keywords 2nd-opinion added; needs-unit-tests removed
  • Milestone changed from Awaiting Review to 4.9

Replying to spacedmonkey:

@ocean90 I didn't manage to replicate your error message. I don't see how there is a problem, tax_query is a well tested part of core. The only reason it didn't use tax_query when this function was created, was because it didn't exists.

What @ocean90 is seeing is not errors, but the results of EXPLAIN, which indicate that a move to get_posts() forces Using temporary. This is bad, and gets increasingly bad as the site of your posts and term_relationships tables get bigger. The main ID query in get_posts() often has to use temporary tables when tax_query is used, mostly because GROUP BY is needed to ensure uniqueness (the same post could match more than one of the terms). There is no easy way to fix this, and given that this part of WP_Query has no caching at all, I'm not convinced that there's a benefit to switching to get_posts() here; there may in fact be a loss in performance.

That being said, it is a good idea to improve performance here. How about adding caching to get_objects_in_term()? 37094.diff shows how this can be done. This actually has bigger impact on query reduction than the original patch, since get_objects_in_term() is used more broadly, while get_posts() always requires at least one query. Tests are also attached that show invalidation when: (a) term assignment changes, (b) a term is deleted, (c) a post is deleted. (No invalidation exists when deleting a non-post object type, but presumably this ought to be handled by the plugin registering the non-post taxonomy.) Thoughts?

Side note that caching for get_objects_in_term() was suggested and sorta rejected in https://core.trac.wordpress.org/ticket/27120#comment:9. But the "switched to a higher level function" idea there probably corresponds to @spacedmonkey's original idea on this ticket, which is less than ideal for the reasons described above. For these reasons I think that caching get_objects_in_term() is a reasonable way forward.

Last edited 11 months ago by boonebgorges (previous) (diff)

@boonebgorges
11 months ago

#6 follow-up: @spacedmonkey
11 months ago

So the reason I created this ticket is I have a navigation caching plugin. I noticed that when profiling, that get_objects_in_term runs a query. get_objects_in_term has no hooks or caching, so there was nothing I could do about that query.

By using the tax query and tt_id, it results in on extra query. However, if the site has lots of terms and linked, I could understand it might be slow. However, this is a general problem with the tax query.

Personally, I would use the tax query and work on #22176 and #37038 tickets. In short, do this query in the wordpress way (a tax query) and make the rest of the core better around it.

But I am not against get_objects_in_term, it is a quick and simple solution.

#7 in reply to: ↑ 6 @boonebgorges
11 months ago

Replying to spacedmonkey:

By using the tax query and tt_id, it results in on extra query. However, if the site has lots of terms and linked, I could understand it might be slow. However, this is a general problem with the tax query.

Personally, I would use the tax query and work on #22176 and #37038 tickets. In short, do this query in the wordpress way (a tax query) and make the rest of the core better around it.

#37038 eliminates the transform, but the transform is not the problematic query here: it's the wp_posts + wp_term_relationships table join. You're correct that this is a general problem with tax query, but it's also the case that tax queries are generally only fired by core in specific places: taxonomy archives, etc. Nav queries are fired on pretty much every page, sometimes multiple times, so we've got to be much more careful about them.

#8 @spacedmonkey
11 months ago

You make some good points. I think we should go ahead with get_objects_in_term caching found in 37094.diff. get_objects_in_term is only used 2 places in core, one of each is here. It is a very low impact change for the rest of core.

#9 @boonebgorges
11 months ago

  • Owner set to boonebgorges
  • Resolution set to fixed
  • Status changed from new to closed

In 40921:

Cache results in get_objects_in_term().

This helps to reduce database queries when generating nav menus.

Props spacedmonkey.
Fixes #37094.

Note: See TracTickets for help on using tickets.