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.

CommentFileSizeAuthor
#7 toolbar_557806.patch1.94 KBdrewish
#1 toolbar.patch1.41 KBcatch
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch’s picture

FileSize
1.41 KB

Helps when including the patch.

Gábor Hojtsy’s picture

- 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).

Damien Tournoud’s picture

[whistling innocently]
If the toolbar was a block, we could use block caching instead of adding another level of complexity.
[/whistling innocently]

catch’s picture

It'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.

catch’s picture

Just 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.

Gábor Hojtsy’s picture

@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 :)

drewish’s picture

FileSize
1.94 KB

catch, 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.

When building the toolbar we need to fetch the entire admin menu tree and
check the user's access to each menu link, an expensive process. To avoid
this the toolbar is cached on a per-user basis because the access callbacks
can use an arbitrary criteria unrelated to roles or permissions. Since the
toolbar is generally only accessible to a limited number of users, and is
rendered on every page, it should still be more efficient than building it
each time.

catch’s picture

@drewish, habit ;)

And that comment looks much better.

sun’s picture

IF you'd use cache_menu, you'd get cache clearing for free.

sun’s picture

But 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.

catch’s picture

I 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.

drewish’s picture

Sounds 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.

catch’s picture

Status: Needs review » Postponed

@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.

catch’s picture

Status: Postponed » Closed (duplicate)

system_admin_menu_block_access() was rolled back, so we no longer need this.