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.
- Every call to hasPermission() does
$roles = \Drupal::entityManager()->getStorageController('user_role')->loadMultiple($this->getRoles());
for both logged in users and anonymous users. - The config records themselves are statically cached within ConfigFactory, so successive loads of the same role don't require additional IO.
- However, each load requires invoking the entity constructor, Role::postLoad(), hook_entity_load(), and hook_user_role_load().
- The entity system includes a static cache feature (enabled via the
static_cache
annotation key) that allows the above to be bypassed for static cache hits. - Currently in HEAD, @ContentEntityType defaults static_cache to TRUE (since for content entities, there's a significant IO savings) and @ConfigEntityType defaults it to FALSE (since for config entities, it only saves on a few function calls, not on IO). There are no individual entity types in HEAD that override this default.
- Because hasPermission() is called so frequently, this issue makes Role the first config entity type to use static_cache.
- Xhprof screenshot from a few months ago (possibly outdated): Anon frontpage get comparison of UserSession::hasPermission().
Comment | File | Size | Author |
---|---|---|---|
#47 | 2202185-47.patch | 1.99 KB | effulgentsia |
#38 | 2202185-38.patch | 3.05 KB | effulgentsia |
#33 | interdiff.txt | 1.56 KB | effulgentsia |
#33 | 2202185-33.patch | 3.96 KB | effulgentsia |
#31 | interdiff.txt | 507 bytes | effulgentsia |
Comments
Comment #1
BerdirHm, 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.
Comment #2
alexpottMeant to get a test in
Comment #4
sunShouldn't this entire code be moved to e.g. current_user or some other access service?
Neither the
UserSession
nor theUser
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.
Comment #5
tim.plunkettSo, \Drupal\Core\Session\AccountProxy ?
Comment #6
damiankloip CreditAttribution: damiankloip commentedHere 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.
Comment #7
moshe weitzman CreditAttribution: moshe weitzman commentedHow about we clear the static whenever the user is saved?
Comment #8
dawehnerYeah, 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.
Comment #9
damiankloip CreditAttribution: damiankloip commentedI 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.
Comment #10
Crell CreditAttribution: Crell commentedA 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.
Comment #11
msonnabaum CreditAttribution: msonnabaum commentedNow 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.
Comment #12
msonnabaum CreditAttribution: msonnabaum commentedIgnore the patch in #11.
Comment #13
msonnabaum CreditAttribution: msonnabaum commentedAlso, 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.
Comment #15
effulgentsia CreditAttribution: effulgentsia commentedHow about this approach instead?
Comment #17
msonnabaum CreditAttribution: msonnabaum commentedThat just seems to be putting implementation details of how roles and permissions are stored into two unrelated classes?
Comment #18
effulgentsia CreditAttribution: effulgentsia commentedFair enough. How about this? I don't think we need to cache $roleStorage though, since EntityManager::getController() already does.
Comment #19
msonnabaum CreditAttribution: msonnabaum commentedI 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.
Comment #20
BerdirThe way config entity query is implemented, this is probably not
slowerfaster (uhm) at all, because that loads all config entities too? So nothing really changes at all with this?Comment #21
effulgentsia CreditAttribution: effulgentsia commentedOk, with the accessor.
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.
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.Comment #22
tim.plunkettFrom \Drupal\Core\Config\Entity\Query\Query::loadRecords() (which is always called in execute()):
So yes, config entity EFQ always loads everything.
Comment #23
msonnabaum CreditAttribution: msonnabaum commentedI strongly disagree with this. We cannot be afraid of using our most basic high level API for perceived performance reasons.
Comment #24
Wim LeersWow, 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 whensave()
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.
Comment #26
effulgentsia CreditAttribution: effulgentsia commentedThe 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.
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().
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.
Comment #27
damiankloip CreditAttribution: damiankloip commentedAlex 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.
Comment #28
msonnabaum CreditAttribution: msonnabaum commentedInstead 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?
Comment #29
effulgentsia CreditAttribution: effulgentsia commentedNo 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.
Comment #31
effulgentsia CreditAttribution: effulgentsia commentedComment #33
effulgentsia CreditAttribution: effulgentsia commentedPicking 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.
Comment #34
effulgentsia CreditAttribution: effulgentsia commentedComment #38
effulgentsia CreditAttribution: effulgentsia commentedI 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.
Comment #40
effulgentsia CreditAttribution: effulgentsia commentedThis one should be green. Also updated title and summary.
Comment #41
effulgentsia CreditAttribution: effulgentsia commentedQuoting myself from the issue summary:
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.
Comment #42
Wim LeersI pinged @msonnabaum about #41.
And in the mean time #2302463: Cleanup User::hasPermission() and UserSession::hasPermission() to follow Law of Demeter was committed :)
Comment #43
damiankloip CreditAttribution: damiankloip commentedRe #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.
Comment #44
alexpottI've had this at the back of my mind since the friday discussions at Portland.
Comment #45
BerdirThat's what #1885830: Enable static caching for config entities is about and yes, it was using the cache key at some point.
Comment #46
effulgentsia CreditAttribution: effulgentsia commentedFixed 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.
Comment #47
effulgentsia CreditAttribution: effulgentsia commented#46 landed, so this is a reroll of #40 to not include the hunks that landed as part of that issue.
Comment #48
moshe weitzman CreditAttribution: moshe weitzman commentedWell, thats nice and simple and green. Way to go, subissues!
Comment #49
BerdirLooks fine to me as well, but should we do some profiling to check how much exactly this helps?
Comment #50
Wim Leers+1 for profiling. We want to make sure that this is still worthwhile.
Comment #51
Wim LeersDoing the profiling, I want to compare to the previously profiled scenario. From #13:
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.
Comment #52
webchickThanks for the analysis.
Committed and pushed to 8.x. Thanks!