While investigating performances issues on one of our site, I noticed that enabling the XML sitemap menu module was responsible for a time increase of about 40% for a single drush cc menu command.

Tracing the issue, this is caused by xmlsitemap_menu_xmlsitemap_process_menu_links be called to process (called from xmlsitemap_menu_menu_link_alter) each menu item individually during menu rebuild.

I may be wrong, but there is little gain in immediate process of a menu link from a menu no used in the site map.

Would it make sense to only process menu items form menu actually in the sitemap. On our site, and I guess many other, it would greatly reduce the load during menu rebuild.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pbuyle created an issue. See original summary.

pbuyle’s picture

Status: Active » Needs review
FileSize
747 bytes

The attached patch implement the proposed solution.

pbuyle’s picture

quicksketch’s picture

This looks great @pbuyle! I ran into the same issue with XMLSitemap having a rather devastating effect on menu rebuilds. I applied this patch and it had a tremendous benefit:

Before: 6307 queries in 2053.25 ms Page execution time was 11350.89 ms
After: 2091 queries in 942.86 ms Page execution time was 6986.62 ms

It seems like this could also be applied to xmlsitemap_menu_menu_delete() and xmlsitemap_menu_menu_link_insert(), but those are very minor in comparison to the change made here in xmlsitemap_menu_menu_link_alter(). Hooks for insert/update/delete are not called during rebuilds, but hook_menu_link_alter() is.

So until #1013856: hook_menu_link_update() not invoked for menu links that have other elements form_alter()'d in is fixed, this work-around helps menu rebuilds tremendously. In our case, the inclusion of the "management" menu was about 90% of the links being loaded and saved into the xmlsitemap table, none of which you'd ever want to be displayed in the sitemap.

quicksketch’s picture

I think #2 is a fine patch and could be committed as-is. However if we want to avoid the unnecessary entries in the xmlsitemap table entirely for unused menus then this patch will skip the inserting of these items entirely. They will be added during the menu rebuild if the menu is enable later. For consistency with new sites, I added an update hook that removes all the unnecessary entries from the xmlsitemap table.

quicksketch’s picture

Status: Needs review » Reviewed & tested by the community

RTBC #2. I'll leave it to the maintainers whether they think #2 or #5 is a better approach. Either solves the critical issue that we unnecessarily recreate xmlsitemap entries for disabled menus. #5 just builds on #2 to entirely avoid/remove the entries in the xmlsitemap table.

candelas’s picture

Patch #5 works great. I was having timeouts and after this patch all works perfect. My xmlsitemap table went from 1234 entries to 325. Thanks a lot!

pifagor’s picture

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 5: xmlsitemap-skip-rebuilds-2646152.patch, failed testing. View results

nplowman’s picture

Attaching an updated patch that works with 7.x-2.4.

candelas’s picture

Status: Needs work » Needs review