This is an alternative to #296693: Restrict access to empty top level administration pages which just adds a simple per-user cache cache to the toolbar. It looks like no-one is very keen on reverting system_admin_menu_block_access(), so this means that any module displaying administrative links has to implement their own per-user caching to avoid critical performance issues. admin_menu.module already does this with client-side caching in supported browsers. menu.module should probably use the block cache somehow - maybe we can fix that when #495968: Introduce drupal_render() cache pattern. Start using it for blocks is in, since the toolbar is meant to be used for a finite number of users, a per-user cache is the best option here.
I'm using the block_cache table because this is conveniently cleared on events like saving the permissions pages and menu_link_save().
Hoping for a quick commit here, because both the query logging and profiler noise from this makes it very difficult to evaluate other performance issues in HEAD.
Front page, no content, user 1.
Before patch:
Executed 43 queries in 26.31 milliseconds. Page execution time was 93.31 ms.
Executed 43 queries in 29.88 milliseconds. Page execution time was 97.41 ms.
Executed 43 queries in 24.89 milliseconds. Page execution time was 84.37 ms.
After patch:
Executed 30 queries in 16.55 milliseconds. Page execution time was 71.57 ms.
Executed 30 queries in 16.96 milliseconds. Page execution time was 77.55 ms.
Executed 30 queries in 18.57 milliseconds. Page execution time was 75.16 ms.
Comment | File | Size | Author |
---|---|---|---|
#7 | toolbar_557806.patch | 1.94 KB | drewish |
#1 | toolbar.patch | 1.41 KB | catch |
Comments
Comment #1
catchHelps when including the patch.
Comment #2
Gábor Hojtsy- Is this cache properly updated when permissions change or modules are reconfigured (shortcuts can be from arbitrary menu items)?
- What happens if we have many admins (think a nowpublic.com style community).
Comment #3
Damien Tournoud CreditAttribution: Damien Tournoud commented[whistling innocently]
If the toolbar was a block, we could use block caching instead of adding another level of complexity.
[/whistling innocently]
Comment #4
catchIt's updated in every call to cache_clear_all() or explicit {cache_block} clear - which includes permissions changes, module installation etc. so should be pretty well covered. Note that menu_tree_all_data() is already cached and won't be updated on every possible module configuration change either, cache_clear_all() calls are used more aggressively than menu_rebuild() in most cases though, such as on every node_save().
If you have 500 admins, you have 500 relatively small cache entries - not much compared to the per-page block caching, per-object field caching, per router-item menu caching that we already have.
Comment #5
catchJust to make my own position clear, I'd rather do #296693: Restrict access to empty top level administration pages than this, and would like a decision to be made before freeze as to which way we go, rather than letting it slide, because removing that access callback would be an API change. Unfortunately every patch to remove the access callback conflicts with d7ux IA change patches, so it's a bit of a waste re-rolling.
See also #520364: system_admin_menu_block_access() makes no sense and #519046: Clean up toolbar menu code.
Comment #6
Gábor Hojtsy@Damien: well, it is not a block for other reasons as discussed already elsewhere. I'd say this looks like a simple performance improvement for a limited number of people, who'd otherwise use the site quite actively, so their experience should matter to us a great deal :)
Comment #7
drewish CreditAttribution: drewish commentedcatch, Any reason you're using
$GLOBALS['user']->uid,
rather than$user
? I could understand if it wasn't already brought in one line before the patch ;)I'd set out to try to slim down your monster comment but ended up rephrasing it. I think it's an improvement but would appreciate your feedback.
Comment #8
catch@drewish, habit ;)
And that comment looks much better.
Comment #9
sunIF you'd use cache_menu, you'd get cache clearing for free.
Comment #10
sunBut I should also amend that - with caching - you also need to copy some of the shiny new code in admin_menu 3.x, specifically admin_menu_toolbar.module, to achieve an active menu item highlighting via JavaScript in a menu output that is cached.
Comment #11
catchI used {cache_block} to get cache clearing for free - since the block cache is cleared on menu_link_save() anyway, but also permissions form submit - where menu isn't.
Comment #12
drewish CreditAttribution: drewish commentedSounds like we should amend the comment to mention why we're using cache_block... I'd wondered about that myself but hadn't that there was that much behind the decision.
Comment #13
catch@sun, that sounds like a good reason not to pursue this caching unless we absolutely have to, so postponing on #296693: Restrict access to empty top level administration pages which doesn't require these hacks.
Comment #14
catchsystem_admin_menu_block_access() was rolled back, so we no longer need this.