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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch created an issue. See original summary.

dawehner’s picture

What about require users of #contextual_links to append contextual_preprocess() to the theme registry entries they need it for?

star-szr’s picture

Issue tags: +Twig

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

catch’s picture

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

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

nils.destoop’s picture

Any 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:
before

When I implement a static variable for the hasPermission. The total time is decreased to 8,872 microsecs:
after-static

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

geek-merlin’s picture

Comment #2348729-75: Convert theme_views_view_field to twig says:

Hmm I see contextual_preprocess() is getting called every time, that's unfortunate. I think @joelpittet and I talked briefly about it. It's because template_preprocess() is not called for theme functions. I wonder if there's something we can do to optimize these types of cases, at least for contextual.

Theme functions are dead now, so this should be a quick win now.

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev
larowlan’s picture

Should 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?

Berdir’s picture

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

Berdir’s picture

Version: 9.3.x-dev » 9.4.x-dev
Status: Active » Needs review
FileSize
2.47 KB
109.13 KB

Did 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:

Berdir’s picture

Issue tags: -needs profiling
Berdir’s picture

Priority: Normal » Major

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

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Wow! 🤩

So obvious. So simple. Such a big impact.

🚢 👏

alexpott’s picture

Version: 9.4.x-dev » 9.3.x-dev
Status: Reviewed & tested by the community » Fixed

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

  • alexpott committed cffeb24 on 10.0.x
    Issue #2580263 by Berdir, nils.destoop, catch, Cottser, larowlan: Find a...

  • alexpott committed ff78b80 on 9.5.x
    Issue #2580263 by Berdir, nils.destoop, catch, Cottser, larowlan: Find a...

  • alexpott committed dff5773 on 9.4.x
    Issue #2580263 by Berdir, nils.destoop, catch, Cottser, larowlan: Find a...

  • alexpott committed 470f318 on 9.3.x
    Issue #2580263 by Berdir, nils.destoop, catch, Cottser, larowlan: Find a...
star-szr’s picture

I am very happy to see this in! Thanks folks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.