WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#22189 closed defect (bug) (fixed)

Saving navigation menus is slow

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.

Attachments (5)

22189.diff (680 bytes) - added by nacin 5 years ago.
Avoid querying for slugs in wp_unique_post_slug(). We may want to introduce a pre_* filter here in the future.
22189.2.diff (1.7 KB) - added by nacin 5 years 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.
22189.3.diff (4.3 KB) - added by nacin 5 years ago.
Explanation to follow
22189.term-cache.diff (1.6 KB) - added by nacin 5 years ago.
Prime term caches in wp_get_nav_menu_items() outside of nav-menu-template.php.
22189.tax_input-on-create.diff (791 bytes) - added by duck_ 5 years ago.

Download all attachments as: .zip

Change History (27)

#1 @nacin
5 years ago

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

@nacin
5 years ago

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

#2 @DrewAPicture
5 years ago

  • Cc xoodrew@… added

@nacin
5 years 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.

@nacin
5 years ago

Explanation to follow

#3 @nacin
5 years 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.

#4 @nacin
5 years 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.

#5 @nacin
5 years ago

  • Description modified (diff)

#6 @knutsp
5 years ago

  • Cc knut@… added

#7 @SergeyBiryukov
5 years ago

In 22189.2.diff, the logic in line 255 looks reversed: $title is only used if it's not set.

@nacin
5 years ago

Prime term caches in wp_get_nav_menu_items() outside of nav-menu-template.php.

#8 @nacin
5 years ago

In [22232]:

Avoid queries in wp_unique_post_slug() for nav menu items. see #22189.

#9 @nacin
5 years ago

In [22233]:

Prime post term caches for nav menu items. Avoid doing so in frontend template functions as it remains unneeded there. see #22189.

#10 @nacin
5 years ago

In [22234]:

In nav menus, avoid firing the magic WP_Post getter for potentially non-existent properties, which results in a metadata cache hit. see #22189.

#11 follow-up: @nacin
5 years ago

In [22235]:

Optimize wp_update_nav_menu_item() for high performance. Only query items for a menu when we need them. Update items after updating its meta so the meta cache can be leveraged. see #22189.

#12 @husobj
5 years ago

  • Cc ben@… added

#13 @nacin
5 years ago

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

#14 in reply to: ↑ 11 @duck_
5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Replying to nacin:

In [22235]:

Optimize wp_update_nav_menu_item() for high performance. Only query items for a menu when we need them. Update items after updating its meta so the meta cache can be leveraged. see #22189.

When $menu_item_db_id is zero is_object_in_term() will return a WP_Error and the newly created menu item will not be associated with the specified menu. attachment:22189.tax_input-on-create.diff skips is_object_in_term() if the call is not updating an existing menu item.

#15 @duck_
5 years ago

In 22399:

Correctly associate new menu items with a menu when using wp_update_nav_menu_item()

wp_update_nav_menu_item() must pass tax_input to wp_insert_post() when creating items
otherwise the menu-item relationship isn't made.

See #22189.

#16 @nacin
5 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

#17 @davidsarver
5 years ago

Was this supposed to lower the number of PHP variables as well as the number of queries? I've upgraded the site to 3.5, but because of a limit of 1000 php variables the menu is still limited to 90 items. Were these improvements not actually integrated into 3.5?

#19 @davidsarver
5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Because of bureaucracy and the fact that I work for a large organization that gets attacked every so often, I can't easily get the limit changed. Would it be feasible for me to go through the .diff files from beginning to end and implement the changes manually? or are they already implemented in 3.5 and I need to find a different solution?

I'm going to re-open this ticket without asking hoping that someone who knows how WordPress works can give a quick answer. Sorry that I'm a noob and I don't know how this ticketing system works at all.

#20 @ocean90
5 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

There is no need to reopen a ticket to get an answer.

Also his ticket was closed as fixed in 3.5. Please open a new ticket if you think it's an issue with max_input_vars.

#21 follow-up: @nacin
5 years ago

You are looking for #14134 for max_input_vars and such.

#22 in reply to: ↑ 21 @davidsarver
5 years ago

Replying to nacin:

You are looking for #14134 for max_input_vars and such.

So these changes do not decrease the number of variables used? Yes, I'm aware of #14134, I've posted on there as well.

Note: See TracTickets for help on using tickets.