Optimize menu tree building and use it for toolbar

cache_clear_all('links:' . $menu_name . ':', 'cache_menu', TRUE);
in function
menu_cache_clear()

either remove the ':', or edit your cache cid.

This is a very interesting alpha-quality patch, which actually maps nicely to what I outlined in #618530: System module's "listing" pages (and blocks) should use menu_build_tree().

+++ modules/toolbar/toolbar.module
@@ -134,12 +134,13 @@ function toolbar_get_menu_tree() {
   $admin_link = db_query("SELECT * FROM {menu_links} WHERE menu_name = 'management' AND module = 'system' AND link_path = 'admin'")->fetchAssoc();
...
+    $data = menu_build_tree('management', array(
+      'parents' => array($admin_link['mlid']),
+      'min_depth' => 2,
+      'max_depth' => 2,
+    ));

Instead of a hard-coded 2, we want to use $admin_link['depth'] here. :)

+++ modules/toolbar/toolbar.module
@@ -134,12 +134,13 @@ function toolbar_get_menu_tree() {
+    menu_tree_check_access($data['tree'], $data['node_links']);
+    $tree = $data['tree'];

Do we really want to put this into the caller's responsibility?

How about going with this menu_build_tree(), but perhaps make it private, i.e. _menu_build_tree(), and make menu_build_tree() invoke _menu_build_tree() + additionally invoke menu_tree_check_access() and return the resulting tree data? :)

This review is powered by Dreditor.

now the cid is too long...

This should work.

Sorry, left a stale parameter in there.

So what's left here?

This works beautifully. Aside from that:

1) A bit wonky is the "parents" tree parameter (list). When would I want to pass more than one plid? (Though we can as well ignore that, because it equals the current code.)

2) uhm, nothing. I'll check whether we could use this cool thingy also for contextual links now ;) (separate issue)

That, I don't get. How can the tree be different for the same parameters?

Access checking happens later only, so I really don't see how this would be possible.

I also don't see a ->random() in there. ;)

It would make me really worry about this function. The result of a function should be the same under the same conditions. I'd consider it a critical bug if a function doesn't return the same for the same arguments.

So can you clarify?

Tagging (and updating the other issue accordingly)

Can we proceed here?

In the meantime, I looked into whether this function could be used for contextual links as well. It probably could be, if the menu system would store a proper plid, depth, and pX columns for dynamic paths/local tasks... which it is currently not. That's surely a separate issue (perhaps related to the existing #550254: Menu links are sometimes not properly re-parented), but this patch would be a major step towards a better solution.

Fixed inline comments.

In $tree_parameters,

- renamed 'parents' to 'expanded'

- renamed 'active_link' to 'active_trail'

because I think that's much more understandable for people who want to use this tree builder.

Test slave #44 produces non-reliable results.

I RTBC this on the condition that in a followup patch we will review how menu build tree and menu tree data could be merged.

I think this is a great clean-up that should be committed.

One comment though: in the original description, DamZ complains about unnecessary duplication but this patch adds more lines than it removes ... Also, in the original description, DamZ complains that the original code is inefficient (both in term of processing power and in term of memory usage), but I don't see performance numbers on the new code? Do we want to test whether it is actually faster? (It should be.)

That sounds like a 'needs review' to me. :)

Wanted to review this patch but it is outdated. However, its purpose is promissing.

#20: drupal.menu-build-tree.20.patch queued for re-testing.

subscribing

patch in #20 no longer applied. Re-rolled.

in the original description, DamZ complains about unnecessary duplication but this patch adds more lines than it removes

Actually, the number of pluses and minuses in the patch is misleading. There are technically more pluses, but there's a lot of added white space, doc blocks, and comments. If you compare just lines of meaningful code. There are 105 minuses of meaningful code versus 89 pluses of meaningful code. So the patch does reduce duplication.

Also, for testers, make sure you clear the cache after applying, otherwise you'll get Fatal errors since the old cache data doesn't match the new cache data structure.

Performance testing it beyond me, or I'd do that and RTBC this again. :-(

Haven't looked too closely at the patch, but it does seem like a nice cleanup for the menu system.

In terms of the toolbar, the changes are probably good for the purposes of this patch - but ideally we want to have the toolbar stop doing weird things altogether, rather than improving the way it does weird things :) In particular, I assume/hope that the fix for #596010: Move "Administration" link into Account menu and make Toolbar output first level of 'admin' menu will involve getting rid of the custom database query in the toolbar module (the one that ties it to paths under 'admin') and allowing it to call something like menu_tree_page_data('management', 1) directly instead...

@David: That's this patch then. The changes for Toolbar in this patch make it use the ideal way.

Re-rolled patch. Also changed tree cid hash from md5 to sha256 to reflect recent changes.

In preparation for the new "major" issue priority, I'm tagging this (not actually critical, but still important) issue as priority-major.
See: http://drupal.org/node/669048#comment-3019824 (#669048: We should have four priorities not three (introduce a new 'major' priority)) for more information about the coming major issue priority.

Re-titling this so it's more obvious why it's critical, I will try to profile it soonish.

There's a separate patch for menu_tree_page_data() here #327918: Optimize menu_tree_page_data() performance, one or the other of these patches are duplicate for that hunk, don't have time to look further this morning though.

#32: drupal.menu-build-tree.patch queued for re-testing.

This takes ~5% off request time on a newly installed HEAD (with access to toolbar) according to xhprof.

There's no tests with the patch, do we have some already?

Attached screenshots, 004 is head, 005 with patch.

This patch changed only marginally since Damien and me touched it last. What else is left to get this in? Nothing?

#32: drupal.menu-build-tree.patch queued for re-testing.

#32: drupal.menu-build-tree.patch queued for re-testing.

Reviewed and committed to CVS HEAD. :)

The new static cache introduced here needs to be cleared by menu_reset_static_cache() - otherwise it leads to some very odd bugs. The patch is trivial, but could ideally use some tests, so I created a separate issue for it here:

#843278: Tests for Menu system static cache clearing