Try to reduce the number of database queries in MenuTreeStorage::rebuild()

Created on 11 December 2024, 6 months ago
Updated 19 June 2025, 5 days ago

Problem/Motivation

Found running xhprof against drush cr, by mistake when trying to track down something else.

When the menu tree is rebuilt, MenuTreeStorage::rebuild() does this:

    $links = [];
    $children = [];
    $top_links = [];
    // Fetch the list of existing menus, in case some are not longer populated
    // after the rebuild.
    $before_menus = $this->getMenuNames();
    if ($definitions) {
      foreach ($definitions as $id => $link) {
        // Flag this link as discovered, i.e. saved via rebuild().
        $link['discovered'] = 1;
        // Note: The parent we set here might be just stored in the {menu_tree}
        // table, so it will not end up in $top_links. Therefore the later loop
        // on the orphan links, will handle those cases.
        if (!empty($link['parent'])) {
          $children[$link['parent']][$id] = $id;
        }
        else {
          // A top level link - we need them to root our tree.
          $top_links[$id] = $id;
          $link['parent'] = '';
        }
        $links[$id] = $link;
      }
    }
    foreach ($top_links as $id) {
      $this->saveRecursive($id, $children, $links);
    }

This eventually reaches this code:

  protected function doSave(array $link) {
    $affected_menus = [];

    // Get the existing definition if it exists. This does not use
    // self::loadFull() to avoid the unserialization of fields with 'serialize'
    // equal to TRUE as defined in self::schemaDefinition(). The makes $original
    // easier to compare with the return value of self::preSave().
    $query = $this->connection->select($this->table, NULL, $this->options);
    $query->fields($this->table);
    $query->condition('id', $link['id']);
    $original = $this->safeExecuteSelect($query)->fetchAssoc();
    
    if ($original) {
      $link['mlid'] = $original['mlid'];
      $link['has_children'] = $original['has_children'];
      $affected_menus[$original['menu_name']] = $original['menu_name'];
      $fields = $this->preSave($link, $original);
      // If $link matches the $original data then exit early as there are no
      // changes to make. Use array_diff_assoc() to check if they match because:
      // - Some of the data types of the values are not the same. The values
      //   in $original are all strings because they have come from database but
      //   $fields contains typed values.
      // - MenuTreeStorage::preSave() removes the 'mlid' from $fields.
      // - The order of the keys in $original and $fields is different.
      if (array_diff_assoc($fields, $original) == [] && array_diff_assoc($original, $fields) ==   ['mlid' => $link['mlid']]) {
        return $affected_menus;
      }
    }

With the standard profile (but with navigation instead of toolbar) this results in 72 select queries. This is out of 228 queries total. MenuTreeStorage is responsible for more than 122 of the other select queries too, so more than 215 out of 228.

Steps to reproduce

Proposed resolution

I think we could load all the links, either with an IN() query by the IDs or just all the links in the database, then pass that big list around and skip the individual select queries.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

📌 Task
Status

Active

Version

11.0 🔥

Component

menu system

Created by

🇬🇧United Kingdom catch

Live updates comments and jobs are added and updated live.
  • Performance

    It affects performance. It is often combined with the Needs profiling tag.

Sign in to follow issues

Comments & Activities

  • Issue created by @catch
  • First commit to issue fork.
Production build 0.71.5 2024