Problem/Motivation

We call admin_toolbar_links_access_filter_user_has_admin_role() a lot. It's doing this in a loop each time: Role::load($role_id)->isAdmin(). That's expensive, even if all the other caching layers are working properly and it's not actually resulting in duplicate DB queries and such.

Proposed resolution

Since this function is only trying to remember a bool for each integer uid, and generally the function is only ever run for a single uid on any given page load, a static cache adds almost nothing to the total memory footprint, but could significantly reduce the number of function calls that have to run for this module to work.

Remaining tasks

  1. Confirm test results.
  2. Review.
  3. Commit.

User interface changes

None.

API changes

None.
Only an internal optimization to an existing function.

Data model changes

None.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dww created an issue. See original summary.

dww’s picture

Assigned: dww » Unassigned
Issue summary: View changes
Status: Active » Needs review
FileSize
1.06 KB
dww’s picture

Further optimization, while we're at it. ;)

Since it's recommended to order roles from least permissive to most, we'll probably have to load all the roles before we find the one marked admin. Better to use ::loadMultiple() instead of loading each individually.

adriancid’s picture

Hi @dww thanks for the issue and for the patch, I tested and it works fine, but I'm trying to figure out if we can achieve the same behavior with a cache context or other cache strategy. What do you think?

dww’s picture

Hi @adriancid.

Sorry, no, a 'cache context' in the Drupal 8 sense is entirely about render arrays. This issue has nothing to do with render arrays. It's entirely about reducing the number of pointless function calls this particular (heavily used) function is performing. Cache context and tags won't help in this case. A static cache is the appropriate cache strategy. It's an incredibly small cost (letting an array of boolean values and integer keys hang out in RAM during the course of a page load, where N is usually only ever 1) to reduce dozens (or more) of function calls that we don't need to duplicate.

Does that make sense? Do you want to read some more docs about D8's render caching to understand what 'cache context' means in D8?

Thanks,
-Derek

adriancid’s picture

Thanks, yesterday I thought a little about this and as you say I don't see the way to do this with context or tags cache, we need to use the static cache.

Do you think that we need to use the drupal_static function for this?

dww’s picture

Glad you see why a static variable in RAM is a good solution, and why render caching doesn't help.

I also thought of drupal_static(). However, that generates additional function calls (which this patch is trying to avoid) and I can see no reason to ever clear this cache on a given page load. Anything that's dynamically altering roles or something should have happened before the first time we call $role->isAdmin().

So no, I don't think we want drupal_static() here, either. ;)

I'd RTBC, but it's my own patch. Maybe someone else will review/test.

Thanks,
-Derek

  • adriancid committed 212128b on 8.x-1.x
    Issue #2972553 by dww, adriancid: Add static cache to speed up...
adriancid’s picture

Status: Needs review » Fixed

Thanks Derek for the issue, the patch, and the patience. ;-)

Status: Fixed » Closed (fixed)

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