bouncer icon indicating copy to clipboard operation
bouncer copied to clipboard

Cache performance inefficent

Open garygreen opened this issue 1 year ago • 3 comments

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?

garygreen avatar Jul 24 '22 21:07 garygreen

  • 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 #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 #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).

JosephSilber avatar Jul 25 '22 17:07 JosephSilber

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

garygreen avatar Jul 25 '22 19:07 garygreen

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

kurucu avatar May 04 '23 05:05 kurucu