WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 3 months ago

#27120 reopened enhancement

Add object cache to wp_nav_menu() items

Reported by: Clorith Owned by:
Milestone: 5.0 Priority: normal
Severity: normal Version: 3.8
Component: Menus Keywords: has-patch 2nd-opinion has-unit-tests
Focuses: template, performance Cc:

Description

Leveraging $menu_items in the wp_nav_menu() function call with the Object Cache shows a drop in queries by 2 for each consecutive call for a specific theme_location

Might need a bit more intense testing to ensure it doesn't negatively impact anything else using the function, but from a front facing side of things it has s ofar not showed any negative effect in my tests.

Attachments (4)

27120.patch (1.0 KB) - added by Clorith 4 years ago.
27120.2.patch (1006 bytes) - added by Clorith 4 years ago.
Fixed indentations to follow coding standard
27120.3.diff (2.8 KB) - added by soulseekah 3 months ago.
Implementation + tests
27120.4.diff (3.0 KB) - added by soulseekah 3 months ago.
Added delete invalidation tests

Download all attachments as: .zip

Change History (17)

@Clorith
4 years ago

This ticket was mentioned in IRC in #wordpress-dev by Clorith. View the logs.


4 years ago

#2 @Clorith
4 years ago

There seems to be an object cache for the default fallback, so if your theme_location isn't found, when nobody picked a menu location in the admin-ui, caching occurs as it should.

If someone has defined the menu location and it matches theme_location, there is no caching done pre-patch.

The two queries being ran with each iteration of wp_nav_menu() as mentioned;

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

and

SELECT 
	wp_posts.* 
FROM 
	wp_posts 
WHERE 
	1=1 
AND 
	wp_posts.ID IN (35,36,38,39,50,51,118,130,131,132,133,134) 
AND 
	wp_posts.post_type = 'nav_menu_item' 
AND 
	((wp_posts.post_status = 'publish')) 
ORDER BY 
	wp_posts.menu_order ASC 
Last edited 4 years ago by Clorith (previous) (diff)

#3 @SergeyBiryukov
4 years ago

Please use tabs rather than spaces for indentation (per WordPress Coding Standards).

Do we need to clear this cache somewhere in case of using a persistent object cache?

#4 @SergeyBiryukov
4 years ago

  • Version changed from trunk to 3.8

This ticket was mentioned in IRC in #wordpress-dev by Clorith. View the logs.


4 years ago

@Clorith
4 years ago

Fixed indentations to follow coding standard

This ticket was mentioned in IRC in #wordpress-dev by Clorith. View the logs.


4 years ago

#7 @SergeyBiryukov
4 years ago

  • Milestone changed from Awaiting Review to 3.9

#8 @mikeschroder
4 years ago

  • Keywords has-patch dev-feedback added

#9 @nacin
4 years ago

  • Keywords 2nd-opinion added; dev-feedback removed
  • Milestone changed from 3.9 to Awaiting Review

I'm not entirely sure I see the bug here. Or rather, I'm not sure the solution is the best way to reduce queries.

The queries I see are in wp_get_nav_menu_object(), which uses get_term() or get_term_by() (one of which is cached, the other will be in #21760); and in wp_get_nav_menu_items(), which uses get_objects_in_term(), which could be switched to a higher-level function that performs caching first.

get_posts() and get_terms() have their own caches to some extent. The get_posts() call could be moved to a _prime_post_caches() call instead.

I don't think we need to blindly cache menus; deeper improvements could go a long way. (As Sergey said, these would changes would require cache invalidation.)

#10 @nacin
4 years ago

  • Milestone changed from Awaiting Review to Future Release

#11 @chriscct7
3 years ago

  • Milestone Future Release deleted
  • Resolution set to maybelater
  • Status changed from new to closed

Closing as maybelater given the lack of interest over the past 2 years

@soulseekah
3 months ago

Implementation + tests

@soulseekah
3 months ago

Added delete invalidation tests

#12 @soulseekah
3 months ago

  • Keywords has-unit-tests added
  • Resolution maybelater deleted
  • Status changed from closed to reopened

Maybe now? :)

@campusboy1987 pointed out that the same wp_nav_menu() calls (for example in twentysixteen header.php and footer.php) generate a duplicate query.

With help from @versusbassz @campusboy1987 @denisco this patch was put together.

  1. Added an item cache around the get_posts() call inside wp_get_nav_menu_items().
  2. Invalidation is inside wp_update_nav_menu_item()
  3. Some tests, including invalidation.

Thanks :)

#13 @SergeyBiryukov
3 months ago

  • Milestone set to 5.0
Note: See TracTickets for help on using tickets.