Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
See profiling/discussion in #2348729: Convert theme_views_view_field to twig.
Proposed resolution
Something other than drush dis -y contextual
Possibly it could do something in hook_theme_registry_alter() instead?
Remaining tasks
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#19 | blackfire_contextual_preprocess.png | 109.13 KB | Berdir |
#19 | contextual-preprocess-2580263-19.patch | 2.47 KB | Berdir |
| |||
#11 | xhprof-after-static.png | 372.13 KB | nils.destoop |
#11 | xhprof-before-static.png | 380.2 KB | nils.destoop |
Comments
Comment #2
dawehnerWhat about require users of
#contextual_links
to append contextual_preprocess() to the theme registry entries they need it for?Comment #3
star-szrThanks for making the issue @catch!
Can we make an API addition to hook_theme() to allow adding preprocess functions instead of just replacing them? The
'preprocess functions'
key seems like a sledgehammer.'additional preprocess functions' or something…
Either way I guess we would rename contextual_preprocess() to something else so it doesn't get automatically picked up.
Comment #4
catchMost of the things we can contextually edit are entities, so I think we should look at whether it's feasible to move this into hook_entity_view_alter() or somewhere similar.
Comment #11
nils.destoop CreditAttribution: nils.destoop as a volunteer and at Wunder commentedAny plans to pick this up again? As current implementation is causing a mass of calls to hasPermission.
An xhprof screenshot as anonymous user of one of my sites for the contextual_preprocess function. It takes 972,991 microsecs:
When I implement a static variable for the hasPermission. The total time is decreased to 8,872 microsecs:
Comment #14
geek-merlinComment #2348729-75: Convert theme_views_view_field to twig says:
Theme functions are dead now, so this should be a quick win now.
Comment #17
larowlanShould this be a task?
Now we've moved everything to templates instead of theme functions, is this something we should re-profile to assess if it is still problematic? With render caching and DPC it may not be as bad anymore.
We can't assume that its only used for entities unfortunately, there's nothing in the API that enforces that.
Could we optimize this by moving the check for $element['#contextual_links'] before the hasPermission check?
hasPermission ends up loading all of the users roles, but we could bypass that if there's no links.
If that doesn't help, should we postpone this on an issue as mentioned above by @Cottser to add support for theme hooks nominating their additional preprocess functions?
Comment #18
BerdirRe-evaluating this seems like a good idea, as is moving the order of the checks. I guess it also depends where you profile. I guess something like /admin/content with at least 50 nodes is going to have a lot more templates than some other page.
Comment #19
BerdirDid some profiling of this with blackfire. I can confirm that contextual_preprocess() is still slow, note that you must create a user that is not uid 1, because uid 1 has special handling that skips the actual permission check.
On /admin/content with 50 nodes I get around 700 calls to it and that's ~4.3% (12ms) of the total response time. This is with a user with a single admin role, it's likely even more if you have a bunch of roles and don't have the permission.
With the attached patch, which implements the proposal in #17, contextual_preprocess() no longer shows up at all. Due to moving the permission check inside the check for having contextual links, we can also move the cache context inside, only adding to to templates that actually have contextual links. That's another considerable performance improvement, saves about 1400 Drupal\Core\Render\BubbleableMetadata::merge() calls in this scenario. In total, in my comparison, that is > 6% improvement with this patch:
Comment #20
BerdirComment #21
BerdirThe merge part made me wonder and I tested the permission page with 7 roles and just the standard.profile modules and configuration. This patch is a 20% improvement on that page according to blackfire. From 2200 hasPermission() calls to 93. 9900 fewer BubbleableMetadata::merge() calls.
Likely both the 6% and this 20% improvement are overreported as usual, but 20% is massive and I think makes this a major performance bug.
Comment #22
Wim LeersWow! 🤩
So obvious. So simple. Such a big impact.
🚢 👏
Comment #23
alexpottCommitted and pushed cffeb24f20 to 10.0.x and ff78b80431 to 9.5.x and dff5773787 to 9.4.x. Thanks!
Committed 48d1874 and pushed to 9.3.x. Thanks!
Backported to 9.3.x since there is no reason to not as far as I can see.
Comment #28
star-szrI am very happy to see this in! Thanks folks!