Statically cache Role entities to speed up (User|UserSession)::hasPermission()

Hm, I'd expect the difference to be much smaller with #1885830: Enable static caching for config entities if not completely irrelevant.

What might be more interesting is an internal cache of the permissions array? Will also require more memory, though.

Shouldn't this entire code be moved to e.g. current_user or some other access service?

Neither the UserSession nor the User entity should really have this state information.

Access permissions are bound to authentication + access. IMO, the service that manages the current user is the place where such information can be cached. All other services should retrieve it from there.

So, \Drupal\Core\Session\AccountProxy ?

Here is a reroll.

We have an issue here with tests, they fail because the statically cached roles are not cleared. So we need to figure out how we want to best deal with that.

Also, I do not think the account proxy is the right place for this? It might be, but then we have to duplicate the permission logic again... as well as the account proxy not being the correct place to have that logic. it should be passed on.

How about we clear the static whenever the user is saved?

Also, I do not think the account proxy is the right place for this? It might be, but then we have to duplicate the permission logic again... as well as the account proxy not being the correct place to have that logic. it should be passed on.

Yeah, having logic in several places makes the code more fragile. In theory we could move the fast implementation on the current user service but keep the other ones and invalidate but this really feels like optimization on the wrong place.

I think the user being saved it not so much a problem, as we will know the user being saved at that point.

The main issue arises from adding a user to a role or something during a request like in UserPermissionTest. From this perspective, having a central place for this would pretty much be the only way to go. I think the static caching of config entities could be a solution here. However that will still call out to the storage controller each time too. We could consider a trait or something for this instead, I dunno.

A PermissionableInterface with a PermissionableTrait that both Account and User leverage makes sense. Although I'm still not comfortable with the \Drupal call in there.

Doesn't User have a method to set a new role? If so, it can just update itself in-place in addition to writing through on save. The user object is shared between the various objects that have a reference to it so that should be fine.

Now that config entities are cacheable, this patch turns that flag on for roles.

I also consolidated the logic for both hasPermission calls in RoleStorage. I'd rather put it on a RoleRepository or something, but we don't have that.

It doesn't feel quite right, but I think it's the best option while both object only have role names. Ideally, they'd both return role objects from their getRoles() method, where it would be trivial to just call hasPermission on those.

If everyone hates that we can go back to how it was, but please don't make a trait, that's just putting a bandaid on the larger problem of having to work with roles as strings.

Ignore the patch in #11.

Also, that patch brings it down to around 4-5ms for me from 9-10ms, on a front page with around 10 nodes on it (61 calls to hasPermission).

Having a cache on the UserSession object for the hasPermission calls had it down to around 3-4ms, but I'm not sure that's worth it. It'd be better to figure out why a single call to Drupal\Core\Entity\EntityManager::getStorage('user_roles') takes ~3ms.

How about this approach instead?

That just seems to be putting implementation details of how roles and permissions are stored into two unrelated classes?

Fair enough. How about this? I don't think we need to cache $roleStorage though, since EntityManager::getController() already does.

I dont really care about the caching, but access to dependencies should still be through local accessors.

Why the change from using loadmultiple to efq? Feels like a micro-optimization.

The way config entity query is implemented, this is probably not slower faster (uhm) at all, because that loads all config entities too? So nothing really changes at all with this?

Ok, with the accessor.

Why the change from using loadmultiple to efq?

Using EFQ instead of loadMultiple() means we don't need to load the roles at all, which means we also don't need to enable static entity caching for them. We may still want to for other reasons, but not for the goal of this issue. Also, a contrib implementation of the entity.query.config service (for e.g., MongoDB) could be potentially extremely fast, so I think it's a good coding guideline to always use EFQ where possible, and only load() when necessary.

The way config entity query is implemented, this is probably not faster at all, because that loads all config entities too?

Not from what I can see. Looks to me like the config is only loaded via $configFactory->loadMultiple(), not via the entity pipeline at all, and that that loadMultiple() is statically cached regardless of entity type annotations. Also, when the condition includes id, then only those get loaded, so we're safe on that front as well.

From \Drupal\Core\Config\Entity\Query\Query::loadRecords() (which is always called in execute()):

    // Load the corresponding records.
    $records = array();
    foreach ($this->configFactory->loadMultiple($names) as $config) {
      $records[substr($config->getName(), $prefix_length)] = $config->get();
    }
    return $records;

So yes, config entity EFQ always loads everything.

Also, a contrib implementation of the entity.query.config service (for e.g., MongoDB) could be potentially extremely fast, so I think it's a good coding guideline to always use EFQ where possible, and only load() when necessary.

I strongly disagree with this. We cannot be afraid of using our most basic high level API for perceived performance reasons.

Wow, suddenly a lot of activity here… msonnabaum asked me yesterday to look into #12, to figure out why it's failing. Turns out the root cause for the many test failures is the fact that resetCache() is not called when save()ing a config entity… AFAICT all tests then pass, including the ones that fatal'd in #12.

Attaching the rerolled patch in case we revert back to the #12 approach.

+++ b/core/lib/Drupal/Core/Session/UserSession.php
@@ -250,4 +242,15 @@ public function getHostname() {
+      $this->roleStorage = \Drupal::entityManager()->getStorage('user_role');

The reason for some of the failures of #12/#24 is that $this->roleStorage is undeclared by the class. However, I still don't get the resistance to #21.

So yes, config entity EFQ always loads everything.

There's a difference between ConfigFactory::loadMultiple() and ConfigEntityStorage::loadMultiple(). The latter calls the former plus a whole bunch of other stuff, like instantiating the entity class and invoking hook_entity_load().

We cannot be afraid of using our most basic high level API for perceived performance reasons.

There's a conceptual difference between querying and loading. That's why we have EFQ. It makes no sense to me to load a bunch of entities within a method whose only job is to return the result of something that is conceptually, merely a query.

Alex is totally right in my opinion. The fact that the current implementation loads everything is merely an implementation detail/shortcoming of the core storage. This could easily not be the case when people switch out the config storage.

Instead of arguing about the semantics of what is right here, I'd ask that if another approach is being proposed, it needs some performance info with it.

How many I/O calls with #21 compared to #12?

No time to capture that info at the moment. If someone else can, that would be great. Otherwise, I'll try to soon, or else we can move on with the original approach here, and I can open a follow up to change to EFQ.

Meanwhile, here's trying to get #24 passing, so we can do an apples-to-apples comparison. Interdiff of this patch relative to #24 and relative to #21 are each attached.

Picking this up again. Starting with reverting an unnecessary change to UserSessionTest, and Wim's change from #24 to see what fails with that, because I think that reset may need to move to somewhere else.

I split off DX changes that aren't strictly related to performance to #2302463: Cleanup User::hasPermission() and UserSession::hasPermission() to follow Law of Demeter.

I restored Wim's fix from #24, but accordingly, removed the resetCache() from ContentEntityDatabaseStorage, and did the same for delete()/doDelete(). Which makes sense, since the static cache is independent of the storage implementation.

This one should be green. Also updated title and summary.

Quoting myself from the issue summary:

Xhprof screenshot from a few months ago (possibly outdated): Anon frontpage get comparison of UserSession::hasPermission().

I wrote that because I'm a bit skeptical on what the savings are at this point. Constructing a config entity from a config array should be fast. Role::postLoad() just does a uasort, which I also hope is fast. HEAD has 0 implementations of hook_user_role_load(). HEAD has 2 implementations of hook_entity_load(): comment_entity_load() which exits fast for non-content entities, and content_translation_entity_load() which isn't even enabled in Standard profile, and if it was enabled in that xhprof run and is the culprit, we should add a similar early return that comment module has.

So my concern with this is that either the xhprof is outdated and this patch no longer has much savings, or else if it does, then it's pointing to something we should optimize, not have to cover with a cache.

Meanwhile, I wonder if statically caching config entities is truly safe, given our config override system. ConfigFactory::getCacheKey() deals with that, but our static entity cache doesn't.

I pinged @msonnabaum about #41.

And in the mean time #2302463: Cleanup User::hasPermission() and UserSession::hasPermission() to follow Law of Demeter was committed :)

Re #41 too. If you want more info on statically caching config entities, we have an issue for that. I am on my phone right now so don't have the issue to hand. Sorry.

That's what #1885830: Enable static caching for config entities is about and yes, it was using the cache key at some point.

Fixed both the reset and override problems in #2303881: Config entity static cache doesn't get reset and isn't override aware. Postponing this on that.

#46 landed, so this is a reroll of #40 to not include the hunks that landed as part of that issue.

Well, thats nice and simple and green. Way to go, subissues!

Looks fine to me as well, but should we do some profiling to check how much exactly this helps?

+1 for profiling. We want to make sure that this is still worthwhile.

Doing the profiling, I want to compare to the previously profiled scenario. From #13:

Also, that patch brings it down to around 4-5ms for me from 9-10ms, on a front page with around 10 nodes on it (61 calls to hasPermission).

Having a cache on the UserSession object for the hasPermission calls had it down to around 3-4ms, but I'm not sure that's worth it. It'd be better to figure out why a single call to Drupal\Core\Entity\EntityManager::getStorage('user_roles') takes ~3ms.

So I created a fresh install, created 10 nodes, and profiles the front page. I did this as an authenticated user (UID 2).

Today's HEAD has 63 calls to ::hasPermission(), and it saves 3 ms (from 11.3 ms to 8.3 ms). On the actual permission checking it, 3.5 ms is saved (from 6.1 ms to 2.65 ms — a 56.8% reduction!). ::hasPermission became only 3 ms faster, not 3.5 ms, because the ::getRoleStorage() calls now take 0.5 ms more. That sounds like a PHP language runtime implementation detail, that can (should) be optimized away in future versions of PHP.

Overall, we trade 2.2% less function calls for 1% more memory consumption (in this specific scenario). Considering that memory consumption won't increase further, but the number of ::hasPermission() calls likely will be significantly higher on very complex websites, that looks like a fair trade-off.

Conclusion: yes, this patch is still worth committing.

Thanks for the analysis.

Committed and pushed to 8.x. Thanks!