This helps prevent unnecessary multiple rebuilds on the same request.

CommentFileSizeAuthor
#1 2415363-menu-rebuild-variable.patch1.2 KBdave reid

Comments

dave reid’s picture

Status: Active » Needs review
StatusFileSize
new1.2 KB

Status: Needs review » Needs work

The last submitted patch, 1: 2415363-menu-rebuild-variable.patch, failed testing.

dave reid’s picture

Status: Needs work » Needs review

Looks like the 7.x-1.x tests are currently failing, so nothing new introduced in this patch.

quicksketch’s picture

Status: Needs review » Reviewed & tested by the community

I confirmed this issue. On our site, I was seeing menu_rebuild() being called *five* times when flushing caches, because we had 4 enabled themes (plus the menu_rebuild() that happens normally in the first place).

Unfortunately this patch doesn't 100% fix the problem. menu_rebuild() is still called twice, once on the current page load when flushing caches and then a second time on the next page load. This is because of the order of drupal_flush_all_caches():

function drupal_flush_all_caches() {
  // Change query-strings on css/js files to enforce reload for all users.
  _drupal_flush_css_js();

  registry_rebuild();
  drupal_clear_css_cache();
  drupal_clear_js_cache();

  // Rebuild the theme data. Note that the module data is rebuilt above, as
  // part of registry_rebuild().
  system_rebuild_theme_data();
  drupal_theme_rebuild();

  entity_info_cache_clear();
  node_types_rebuild();
  // node_menu() defines menu items based on node types so it needs to come
  // after node types are rebuilt.
  menu_rebuild();

  // Synchronize to catch any actions that were added or removed.
  actions_synchronize();

  // Don't clear cache_form - in-progress form submissions may break.
  // Ordered so clearing the page cache will always be the last action.
  $core = array('cache', 'cache_path', 'cache_filter', 'cache_bootstrap', 'cache_page');
  $cache_tables = array_merge(module_invoke_all('flush_caches'), $core);
  foreach ($cache_tables as $table) {
    cache_clear_all('*', $table, TRUE);
  }

  // Rebuild the bootstrap module list. We do this here so that developers
  // can get new hook_boot() implementations registered without having to
  // write a hook_update_N() function.
  _system_update_bootstrap_status();
}

So menu_rebuild() is called once by the function directly, then the call to module_invoke_all('flush_caches') will set the variable and the menu_rebuild() will be called again on the next page request.

I think this patch is a good fix and solves most of the problem. However, I think that this patch should be applied *in addition* to #2378449: Performance issue with Breakpoints doubling time for drush cc all. Combined, they'll reduced the menu_rebuild() calls down to a single one on cache clears. So marking this RTBC.

quicksketch’s picture

I filed a patch that combines the two issues over at #2378449-8: Performance issue with Breakpoints doubling time for drush cc all. However, if we want to commit this patch first, this one is still RTBC by itself.

dave reid’s picture

Status: Reviewed & tested by the community » Closed (duplicate)