Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 UTC on 18 March 2024, to get $100 off your ticket.
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.
Comment | File | Size | Author |
---|---|---|---|
#7 | system_admin_menu_block.patch | 2.28 KB | catch |
#1 | system_admin_menu_block.patch | 2.47 KB | catch |
Comments
Comment #1
catchPatch would help.
Comment #2
catchAnd 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.
Comment #3
sunWay to go.
Comment #4
Damien Tournoud CreditAttribution: Damien Tournoud commentedNo. 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.
Comment #5
catchThis 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?
Comment #6
Damien Tournoud CreditAttribution: Damien Tournoud commentedOut 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?
Comment #7
catchHere 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).
Comment #8
jrchamp CreditAttribution: jrchamp commented@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.
Comment #9
catchI removed the elseif because that also breaks the static caching - since as noted above, most calls to this function don't have mlid populated.
Comment #10
jrchamp CreditAttribution: jrchamp commentedI 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.
Comment #11
catchI 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.
Comment #12
Dries CreditAttribution: Dries commentedI've committed the patch in #7 but I'll leave this issue open so DamZ can still follow-up with a patch. Incremental improvements. ;-)
Comment #14
catchRetitling for the remaining issue(s).
Comment #15
catchsystem_admin_menu_block_access() was reverted, so nothing to do here.