Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cache performance inefficent #600

Open
garygreen opened this issue Jul 24, 2022 · 3 comments
Open

Cache performance inefficent #600

garygreen opened this issue Jul 24, 2022 · 3 comments

Comments

@garygreen
Copy link

garygreen commented Jul 24, 2022

There are various performance and storage problems with the CachedClipboard as below:

  • Each ability check sets a new key in cache for every single user
  • Each of the checks sets a huge payload in your cache, e.g. Redis, which has a serialized model of every single ability defined
  • The cache is set forever which is not recommended, because there's potential the cache would never get cleared and linger around forever. There should be a reasonable limit, e.g. 1 month.

See below example of bouncer setting cached ability in Redis:

Huge payloads set for every single user

image

With an application of a million users and 200 abilities Bouncer would set a HUGE amount of data in cache, forever.

I'm not sure what the solution is here, but I think reducing the payload stored would be a good start, maybe limit it to just the important ids if possible. Also is there a reason why it needs to cache for each user?

@JosephSilber
Copy link
Owner

  • Each ability check sets a new key in cache for every single user

    Of course it does! How else could it be stored? It's obviously user-dependent, so it has to be stored per user.

  • Each of the checks sets a huge payload in your cache, e.g. Redis, which has a serialized model of every single ability defined

    I actually had a plan to introduce an additional caching strategy which only caches Boolean responses to a given check. It would use the non-caching Clipboard (currently activated by using Bouncer::dontCache()), and simply cache the returned Booleans.

    I had started on it once, but didn't finish it. I said in Optimize the non-cached clipboard to only fetch relevant abilities #276:

    After merging this and having people try it out, I plan to create an additional caching clipboard based on this one, that only caches the results of the checks at the gate.

    If we want to revive that, we would have to figure out a way to make all of this configuration intuitive:

    • Should we cache at all?
    • Should we only cache for the current request?
    • Should we only cache for the current request single abilities at a time?
    • Should we cache for future requests?
    • Should we only cache for future requests single abilities at a time? If so, should we cache for the current request all abilities?

    To complicate matters, we might even need another layer of local caching. As you can see here, here and in other places, rehydrating all models for every check is quite wasteful, so a local cache would be really beneficial.

    This gets really complex. Add in expiration to the mix, and it can get really hairy.

  • The cache is set forever which is not recommended, because there's potential the cache would never get cleared and linger around forever. There should be a reasonable limit, e.g. 1 month.

    I can hear that. It's been requested in Feature Request: Configure ttl Cache instead of forever #552, and I think it makes sense to make it configurable.

    Would only be complex if we would have two layers of caching (see the previous point).

@garygreen
Copy link
Author

I actually had a plan to introduce an additional caching strategy which only caches Boolean responses to a given check. It would use the non-caching Clipboard (currently activated by using Bouncer::dontCache()), and simply cache the returned Booleans.

That's one way of doing it, though would mean a lot more keys being set in Redis: users * abilities = quite lot of keys of bools.

Personally I think permissions is such an integral part of an application it makes sense to just have a field directly on the User model... $user->permissions which is a JSON array of the users roles, and abilities cached.

That JSON array would be super simple which lists roles, and individual granted abilities, e.g:

$user->permissions:

{"roles": ["moderator", "editor"], "granted": ["kick-users"], "forbidden": ["ban-users"]}

....that way there is no need for caching, it's already available on users straight out the box. The amount of data stored is minimal, and the abilities can be loaded once and expanded based on the roles.

Roles Caching by PHP File

In addition to the above the abilities for each role could be cached in a PHP file, a bit like config caching, which would also benefit from OpCache.

It would just be a simple file that returns:

<?php

return [
   'moderator' => ['ban-users', 'delete-accounts'],
   'editor' => ['make-posts'],
];

As I write this, I'm starting to realise maybe it's sensible we create own super simple permission system

@kurucu
Copy link

kurucu commented May 4, 2023

I was about to reply, but now see that this hasn't been active since July '22 - is it still relevant?

Anyhow - rather than caching the results on the users table, why not cache it in the cache? All that's changed in your proposal is that the cache is per user, rather than per user per ability.

I was about to refactor my app to use Bouncer, and this post spooked me. Regarding the assertions of the original post:

  • "Each ability check sets a new key in cache for every single user" - is that a big deal? What's the memory and performance penalty? Does it reach cache implementation boundaries?
  • "Each of the checks sets a huge payload in your cache, e.g. Redis, which has a serialized model of every single ability defined" - Same question as above, does this payload push any boundaries?
  • "The cache is set forever which is not recommended, because there's potential the cache would never get cleared and linger around forever. There should be a reasonable limit, e.g. 1 month." - seems like this is easy to fix with TTLs, and some events/triggers for destruction

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants