Steps to reproduce, install toolbar.module, stick a debug_backtrace() in system_admin_menu_block(), watch the entirety of /admin being built on every page.

I think the proper fix for this is just to use a custom menu for the main toolbar links - this will be needed by the latest proposal at #508458: Config and modules page anyway.

#12 menu.png344.63 KBcatch
#6 remove_access.patch3.92 KBcatch
Failed: 11686 passes, 5 fails, 872 exceptions View
#3 _menu_link_translate.png317.91 KBcatch
Members fund testing for the Drupal project. Drupal Association Learn more


jrchamp’s picture


catch’s picture

So there has to be around this, but in the meantime I found some bugs in system_admin_menu_block() which were causing duplicate rendering and queries: #521798: system_admin_menu_block() static caching is broken .

catch’s picture

317.91 KB

This is taking up approximately 30% of page execution time at the moment.

catch’s picture

Title: toolbar_get_menu_tree() causes system_admin_menu_block() to be called on every page » system_admin_menu_block_access() is very, very, slow
sun’s picture

admin_menu, with client-side caching being disabled, suffers from this, too.

catch’s picture

Status: Active » Needs review
3.92 KB
Failed: 11686 passes, 5 fails, 872 exceptions View

We should just roll this back. You might get a link which doesn't go anywhere, but you'll get there 35% faster.

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

Status: Needs work » Needs review

This is now a straight rollback of #296693: Hide empty admin categories I may re-open that one at least temporarily since while the original bug is annoying, the fix is worse.

sun’s picture

Status: Needs review » Active

uhm, no. Re-introducing MENU_DYNAMIC_ITEM would be an option, but reverting the code that ensures that admin categories are hidden if there are not any items below them does not do any good.

Damien Tournoud’s picture

Title: system_admin_menu_block_access() is very, very, slow » system_admin_menu_block_access() makes no sense
Component: toolbar.module » system.module
Assigned: Unassigned » Damien Tournoud

... and I believe I know the guy that introduced that.

On it.

moshe weitzman’s picture

catch’s picture

344.63 KB

webgrind screenshot, looks like Moshe's issue found some paths that rightly shouldn't be affected by this, but fixing this one would narrow that down a lot.

Damien Tournoud’s picture

Assigned: Damien Tournoud » Unassigned

I'm not giving up hope of finding time to work on this, but I leave the floor open.

catch’s picture

I've not been able to think of a good solution to this, but posted a patch over at #519046: Clean up toolbar menu code which fixes most of the symptoms without hopefully adding too much extra cruft.

Gábor Hojtsy’s picture

Another reason system_admin_menu_block_access() does not make total sense is that there might be underlying tabs in the IA on D7. #545952: D7UX IA: move modules to config/modules is not viable as it is because ATM there are no default enabled modules exposing configuration to the configuration tab (admin/config path), so system_admin_menu_block_access() decides people should not have access, even though there is a tab underneath, which has content (but that is not rendered by system_admin_menu_block()).

Of course once we have core config items moved there, this will not be visible anymore, but it is still an issue for cases when the default tab on the item does not have accessible content but there are subtabs otherwise which have content.

catch’s picture

Status: Active » Closed (duplicate)

#296693: Hide empty admin categories was rolled back, and is still open for fixes, marking this as duplicate.