WordPress.org

Make WordPress Core

Opened 18 months ago

Last modified 16 months ago

#22189 closed defect (bug)

Saving navigation menus is slow — at Version 5

Reported by: nacin Owned by:
Milestone: 3.5 Priority: normal
Severity: normal Version:
Component: Menus Keywords:
Focuses: Cc:

Description (last modified by nacin)

This is a follow-up to a few tickets, such as #17031. I could not find a duplicate. It is also tangentially related to #14134, which would be menus are affected to max post var settings.

Bottom line: saving navigation menus are slow. I was hoping to see that removing hierarchy ancestor cleaning in #11399 made a big improvement, but a 90-item flat menu still took 24 seconds and a few thousand queries to save. No me gusta.

I started chipping away at this giant list of queries, and came up with a number of changes across a number of areas of core to reduce them. The page now saves (and renders — it doesn't redirect) with only a few hundred queries — it's a lot, but considering a few queries per post being saved, not bad — and just a few short seconds on my little MacBook Air that could.

Since these improvements are largely outside the menus API, I will be spawning individual tickets for most things.

Change History (8)

comment:1 nacin18 months ago

  • Summary changed from Saving navigation menus are slow to Saving navigation menus is slow

nacin18 months ago

Avoid querying for slugs in wp_unique_post_slug(). We may want to introduce a pre_* filter here in the future.

comment:2 DrewAPicture18 months ago

  • Cc xoodrew@… added

nacin18 months ago

When printing nav checklists, we avoid meta and term caches. WP_Post's magic get/set will trigger a metadata_exists() check. Using property_exists() and some cleaner code can prevent the meta cache from firing.

nacin18 months ago

Explanation to follow

comment:3 nacin18 months ago

22189.diff — Avoid useless queries in wp_unique_post_slug() for nav menu items. We may want to introduce a pre_* filter here in the future, so plugins could similarly sidestep these queries.

22189.2.diff — When printing nav checklists, we avoid meta and term caches. WP_Post's magic get/set will trigger a metadata_exists() check. Using property_exists() and some cleaner code can prevent the meta cache from firing.

#22190 — Delete oEmbed caches on pre_post_update so cached meta is used. Patch: attachment:22190.diff.

#22191 — In update_metadata(), compare old/new values before deciding whether to call add_metadata(). Patch attachment:22191.diff.

#22192 — update_metadata() and update_option() strict checks can cause false negatives. No patch, but this is solved for nav menus only in 22189.3.diff by casting integers to strings.

Also in 22189.3.diff:

  • Don't call wp_get_nav_menu_items() in wp_update_nav_menu_item() unless we need to. (Huge.)
  • When saving a menu item, only set the menu term against it if it isn't already set. This prevents a wp_get_object_terms() call that can trigger a meta cache we haven't loaded.
  • When creating a new menu item, we need to create it then add meta (as we need the post ID to do so). But when updating a menu item, update the meta first. This allows for update_metadata() to use the existing meta cache for old/new comparison, rather than forcing a new meta cache fetch.

comment:4 nacin18 months ago

Some of these things, you may notice, are legitimate bugs or less-than-ideal API behavior. But a decent amount of this is also only a problem when you are saving a ton of posts at once. The only similar situation for core is bulk_edit_posts(), which A) could probably be made faster (I haven't checked) and B) isn't unexpected by the user to be super fast. Try running an operation on a few hundred or thousand Gmail messages. It understandably takes time.

We can probably make some adjustments to make some of this work better at scale. The biggest ones are probably to improve the order in which metadata sets, term sets, and term counting is performed from wp_insert_post(). Some of this would mean new API such as a 'meta' array — #20451 — so things can be done all in one go, either before or after the cache clear, depending. I'm also looking very curiously at the actual need to flush an object's metadata when we clear its cache too (aside from object deletions).

With regards to term counting, what I was seeing is that term counting (triggered by post status changes) was getting deferred, but in order to defer them, term IDs were still fetched for each object (to be entered into the defer queue), which triggered a new term cache fetch for each object. It would be cooler if objects themselves were deferred, that way an enterprising individual could defer term counting, update a ton of posts, fetch the term cache for all of those posts at once, then run the count queue.

Of course, neither term counts (especially on post status changes) are even needed for menu items, which is another story.

Overall, while there is continued room for improvement, this is a quick bite at the apple to speed up menus now.

comment:5 nacin18 months ago

  • Description modified (diff)
Note: See TracTickets for help on using tickets.