Make WordPress Core

Opened 10 years ago

Closed 12 months ago

#27120 closed enhancement (invalid)

Add object cache to wp_nav_menu() items

Reported by: clorith's profile Clorith Owned by:
Milestone: Priority: normal
Severity: normal Version: 3.8
Component: Menus Keywords: has-patch 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 10 years ago.
27120.2.patch (1006 bytes) - added by Clorith 10 years ago.
Fixed indentations to follow coding standard
27120.3.diff (2.8 KB) - added by soulseekah 6 years ago.
Implementation + tests
27120.4.diff (3.0 KB) - added by soulseekah 6 years ago.
Added delete invalidation tests

Download all attachments as: .zip

Change History (28)

@Clorith
10 years ago

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


10 years ago

#2 @Clorith
10 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 10 years ago by Clorith (previous) (diff)

#3 @SergeyBiryukov
10 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
10 years ago

  • Version changed from trunk to 3.8

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


10 years ago

@Clorith
10 years ago

Fixed indentations to follow coding standard

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


10 years ago

#7 @SergeyBiryukov
10 years ago

  • Milestone changed from Awaiting Review to 3.9

#8 @kirasong
10 years ago

  • Keywords has-patch dev-feedback added

#9 @nacin
10 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
10 years ago

  • Milestone changed from Awaiting Review to Future Release

#11 @chriscct7
9 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
6 years ago

Implementation + tests

@soulseekah
6 years ago

Added delete invalidation tests

#12 @soulseekah
6 years 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
6 years ago

  • Milestone set to 5.0

#14 @SergeyBiryukov
6 years ago

  • Milestone changed from 5.0 to 5.1

#15 @pento
5 years ago

  • Milestone changed from 5.1 to Future Release

This ticket needs reviewing and a decision.

#16 @mukesh27
20 months ago

@spacedmonkey did a lot of object cache related work for 6.1. Do you have time available for this ticket? Or did you get a chance to work on this ticket? If you have some time to review this ticket, please have a look at it. If you think I can be of any help, please let me know.

cc. @peterwilsoncc.

#17 @johnbillion
20 months ago

This ticket crossed my mind last week because the wp_cache_flush_group() function is now in place. It facilitates caching nav menus separately for each URL in a group that can now be flushed easily when necessary (as long as the cache driver supports it).

However I think the main blocker for doing this in core is handling dynamic menu items and handling filters on the nav menu hooks that can adjust the output. It's fine on a client site where you know there is no dynamism in the nav menus, but it can't be guaranteed otherwise.

#18 @spacedmonkey
20 months ago

A lot of work has been done on caching and caching priming in 6.0/6.1. I am not sure that this ticket is valid anymore. Taxonomy and posts queries are cached in object caching now. This was fixed upstream. @Clorith and @mukesh27 can you do some profiling against trunk see if this ticket is valid?

#20 @spacedmonkey
15 months ago

@Clorith Can you confirm that this ticket is still valid? Caching was added to both WP_Query and WP_Term_Query, so it is likely there lower level caches will cover this.

I believe we need mark this as #invalid, but I want to hear from others on this ticket.

#21 @Clorith
15 months ago

Oh how I wish I was better at making tickets back in the day, I can only apologize for the lack of clarity and information in this one.

I do agree that the underlying cache solutions are probably a better approach now though, and with the improvements done there recently, I can't immediately replicate my original thoughts at least, so I think we could probably close this ticket, and if someone can recall what I was thinking in my days of youth, and it turns out this isn't fully resolved, we can always reopen it later.

#22 @spacedmonkey
13 months ago

This ticket now seems invalid, I am plan to close it out, if anyone disagree, let me know.

#23 @spacedmonkey
13 months ago

#36225 was marked as a duplicate.

#24 @mukesh27
12 months ago

  • Keywords 2nd-opinion removed
  • Milestone Future Release deleted
  • Resolution set to invalid
  • Status changed from reopened to closed

As per the discussion at https://core.trac.wordpress.org/ticket/27120#comment:22, no one disagrees. Therefore, I am going to close this. Thank you!

Note: See TracTickets for help on using tickets.