It caches based on $item['mlid'] but then overwrites the $item variable with whichever the last child of the top level menu item is, so that never gets used.

Additionally, we can cache based on item['path'] here, no reason not to - we never, ever get $item['mlid'] passed to this function, which results in another 20 pointless queries.

HEAD:
Executed 91 queries in 45.8 milliseconds.

Patch:
Executed 58 queries in 27.64 milliseconds.

I don't think this one needs benchmarks.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch’s picture

Patch would help.

catch’s picture

And on the front page:

HEAD:
Executed 56 queries in 53.91 milliseconds.

Patch:
Executed 52 queries in 47.22 milliseconds.

See also #520364: system_admin_menu_block_access() makes no sense.

sun’s picture

Status: Needs review » Reviewed & tested by the community

Way to go.

Damien Tournoud’s picture

Status: Reviewed & tested by the community » Needs work

No. We need to cache by mlid, because that's the only unique key of menu links.

Looking at http://api.drupal.org/api/function/system_main_admin_page/7, which is the standard way of calling system_admin_menu_block(), the mlid should be passed. If it is not, it's probably a bug in the caller, that we need to fix.

catch’s picture

Status: Needs work » Needs review

This function is also called as the access callback for every menu link at that level - which means it's called by menu_get_active_trail() on any item within /admin and also for display of the toolbar, and the management menu block - so it's not just that caller we need to worry about, in fact that's currently the least frequent caller of all the other options.

Can you explain why we need to cache by mlid when we only have between 5-20 possible cache keys on a page, each of which has a unique path?

Damien Tournoud’s picture

Out of the box, sure menu path are unique. But you can add every link you want in the link hierarchy below the "Administer" link.

Ok, basically we got all that thing wrong. We are trying to hide a menu router (ie. system_admin_menu_block_access()), based on considerations on the menu links... That's so wrong it hurts. We need to rethink the whole thing.

Could we concentrate this issue on just fixing the static caching, keeping the cache keyed by mlid?

catch’s picture

Here it is with just the static caching fixed.

Executed 71 queries in 37.36 milliseconds. (so leaving the 20 queries to fetch the mlid with this version).

jrchamp’s picture

@catch, it should probably still be elseif, so that we don't return the cache items whose mlid is mistakenly blank. Plus, it allows us to skip that if when mlid isn't set.

catch’s picture

I removed the elseif because that also breaks the static caching - since as noted above, most calls to this function don't have mlid populated.

jrchamp’s picture

I see what you mean. Is there any way to flag a missing mlid at that point so that people know about this problem? I guess I'm worried about multiple items without mlids causing the same menu link to be display multiple times (the first blank one found). I'm a bit surprised that it doesn't cache on menu_name as well, given that when mlid is 0, you get different results depending on which menu_name you provide.

catch’s picture

I think once #520364: system_admin_menu_block_access() makes no sense is sorted out, we'll have less possible entry points into this function, at which point it'll be easier to track down why no mlid is passed in and fix it at the source. Since the current patch is now a pretty straight bug fix, we could probably go ahead here, wait and see on Damien's patch, and maybe open another issue for the missing mlids when we've got less to worry about.

Dries’s picture

I've committed the patch in #7 but I'll leave this issue open so DamZ can still follow-up with a patch. Incremental improvements. ;-)

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

Title: system_admin_menu_block() static caching is broken » system_admin_menu_block() not passed mlid
Assigned: catch » Unassigned
Status: Needs work » Active

Retitling for the remaining issue(s).

catch’s picture

Title: system_admin_menu_block() not passed mlid » system_admin_menu_block() static caching is broken
Status: Active » Fixed

system_admin_menu_block_access() was reverted, so nothing to do here.

Status: Fixed » Closed (fixed)
Issue tags: -Performance

Automatically closed -- issue fixed for 2 weeks with no activity.