shortcut_current_displayed_set() issues unnecessary queries / menu_load_links() causes a filesort [#646486]

On every page in HEAD with shortcut module enabled we have the following queries:

SELECT s.set_name AS set_name FROM shortcut_set s INNER JOIN shortcut_set_users u ON s.set_name = u.set_name WHERE (u.uid = :db_condition_placeholder_0)

SELECT ss.* FROM shortcut_set ss WHERE (set_name = :db_condition_placeholder_0)

These could be consolidated into a single query with a join. Here's the relevant snippet from shortcut_current_displayed_set().

Additionally, there's absolutely no reason why this should be using db_select().

  // If none was found, try to find a shortcut set that is explicitly assigned
  // to this user.
  $query = db_select('shortcut_set', 's');
  $query->addField('s', 'set_name');
  $query->join('shortcut_set_users', 'u', 's.set_name = u.set_name');
  $query->condition('u.uid', $account->uid);
  $shortcut_set_name = $query->execute()->fetchField();
  if ($shortcut_set_name) {
    $shortcut_set = shortcut_set_load($shortcut_set_name);
  }

Comments

Comment #1

catch

he/him

English

CreditAttribution: catch commented 30 November 2009 at 08:57

Priority:

Normal

» Critical

This is a bit worse than I originally realised:

shortcut_current_displayed_set() calls menu_load_links()

However shortcut_renderable_links() calls menu_tree($menu_name) - which then calls http://api.drupal.org/api/function/menu_tree_page_data/7 - which gets the links from cache anyway, and also has language support - as far as I can see links from menu_load_links() are queries then thrown away on most requests.

So the menu_load_links() call needs to go too.

Comment #2

David_Rothstein CreditAttribution: David_Rothstein commented 1 December 2009 at 02:31

Is this really critical? I'm pretty sure these queries only run for users who have appropriate permissions. And also, only on pages where the shortcuts will actually be displayed (except perhaps for http://api.drupal.org/api/function/shortcut_page_build/7 which has some sketchy stuff going on in it that could probably be improved).

I think having two versions of shortcut_set_load() (one of which would be a lightweight version that did not load the menu links) was discussed in the original issue and decided against in the name of a cleaner API..... We could obviously revisit that if necessary, but is it really a big performance hit? Seems like we are talking about a single database query that should be relatively fast.

Comment #3

catch

he/him

English

CreditAttribution: catch commented 1 December 2009 at 02:46

Priority:

Critical

» Normal

Originally I'd thought the menu_load_links() was being used to render the shortcuts, which would've meant they bypassed the translatable query tag, which would've been critical, but forgot to change the status back as I followed the code flow, back to normal.

However it's still three queries run on every page for no good reason, whereas it could be one. And executing a page callback now takes a baseline of one database query if using a non-database caching backend, so a couple of queries can easily be 10/20% of those required to execute a request. I also don't see why a separate round trip to the database to load something into memory when there's a 99% chance it won't be used should be considered 'clean'.

Comment #4

catch

he/him

English

CreditAttribution: catch commented 10 January 2010 at 14:42

Title:	shortcut_current_displayed_set() issues unnecessary queries	» shortcut_current_displayed_set() issues unnecessary queries / menu_load_links() causes a filesort
Priority:	Normal	» Critical

mysql> EXPLAIN SELECT ml.* FROM menu_links ml WHERE (ml.menu_name = 'management') ORDER BY weight ASC;
+----+-------------+-------+------+-------------------------------------+------------------------+---------+-------+------+-----------------------------+
| id | select_type | table | type | possible_keys                       | key                    | key_len | ref   | rows | Extra                       |
+----+-------------+-------+------+-------------------------------------+------------------------+---------+-------+------+-----------------------------+
|  1 | SIMPLE      | ml    | ref  | menu_plid_expand_child,menu_parents | menu_plid_expand_child | 98      | const |  173 | Using where; Using filesort | 
+----+-------------+-------+------+-------------------------------------+------------------------+---------+-------+------+-----------------------------+
1 row in set (0.00 sec)