[V3/permissions] Performance improvements (#1885)

* basic caching layer

* bit more work, now with an upper size to the cache

* cache fix

* smarter cache invalidation

* One more cache case

* Put in a bare skeleton of something else still needed

* more logic handling improvements

* more work, still not finished

* mass-resolve is done in theory, but needs testing

* small bugfixin + comments

* add note about before/after hooks

* LRU-dict fix

* when making comments about optimizations, provide historical context

* fmt pass

.github/CODEOWNERS

@@ -24,6 +24,7 @@ redbot/core/utils/mod.py                 @palmtree5
 redbot/core/utils/data_converter.py      @mikeshardmind
 redbot/core/utils/antispam.py            @mikeshardmind
 redbot/core/utils/tunnel.py              @mikeshardmind
+redbot/core/utils/caching.py             @mikeshardmind
 
 # Cogs
 redbot/cogs/admin/*                 @tekulvw

redbot/cogs/permissions/mass_resolution.py

@@ -0,0 +1,102 @@
+from redbot.core import commands
+from redbot.core.config import Config
+from .resolvers import entries_from_ctx, resolve_lists
+
+# This has optimizations in it that may not hold True if other parts of the permission
+# model are changed from the state they are in currently.
+# (commit hash ~ 3bcf375204c22271ad3ed1fc059b598b751aa03f)
+#
+# This is primarily to help with the performance of the help formatter
+
+# This is less efficient if only checking one command,
+# but is much faster for checking all of them.
+
+
+async def mass_resolve(*, ctx: commands.Context, config: Config):
+    """
+    Get's all the permission cog interactions for all loaded commands
+    in the given context.
+    """
+
+    owner_settings = await config.owner_models()
+    guild_owner_settings = await config.guild(ctx.guild).owner_models() if ctx.guild else None
+
+    ret = {"allowed": [], "denied": [], "default": []}
+
+    for cogname, cog in ctx.bot.cogs.items():
+
+        cog_setting = resolve_cog_or_command(
+            objname=cogname, models=owner_settings, ctx=ctx, typ="cogs"
+        )
+        if cog_setting is None and guild_owner_settings:
+            cog_setting = resolve_cog_or_command(
+                objname=cogname, models=guild_owner_settings, ctx=ctx, typ="cogs"
+            )
+
+        for command in [c for c in ctx.bot.all_commands.values() if c.instance is cog]:
+            resolution = recursively_resolve(
+                com_or_group=command,
+                o_models=owner_settings,
+                g_models=guild_owner_settings,
+                ctx=ctx,
+            )
+
+            for com, resolved in resolution:
+                if resolved is None:
+                    resolved = cog_setting
+                if resolved is True:
+                    ret["allowed"].append(com)
+                elif resolved is False:
+                    ret["denied"].append(com)
+                else:
+                    ret["default"].append(com)
+
+    ret = {k: set(v) for k, v in ret.items()}
+
+    return ret
+
+
+def recursively_resolve(*, com_or_group, o_models, g_models, ctx, override=False):
+    ret = []
+    if override:
+        current = False
+    else:
+        current = resolve_cog_or_command(
+            typ="commands", objname=com_or_group.qualified_name, ctx=ctx, models=o_models
+        )
+        if current is None and g_models:
+            current = resolve_cog_or_command(
+                typ="commands", objname=com_or_group.qualified_name, ctx=ctx, models=o_models
+            )
+    ret.append((com_or_group, current))
+    if isinstance(com_or_group, commands.Group):
+        for com in com_or_group.commands:
+            ret.extend(
+                recursively_resolve(
+                    com_or_group=com,
+                    o_models=o_models,
+                    g_models=g_models,
+                    ctx=ctx,
+                    override=(current is False),
+                )
+            )
+    return ret
+
+
+def resolve_cog_or_command(*, typ, ctx, objname, models: dict) -> bool:
+    """
+    Resolves models in order.
+    """
+
+    resolved = None
+
+    if objname in models.get(typ, {}):
+        blacklist = models[typ][objname].get("deny", [])
+        whitelist = models[typ][objname].get("allow", [])
+        resolved = resolve_lists(ctx=ctx, whitelist=whitelist, blacklist=blacklist)
+        if resolved is not None:
+            return resolved
+        resolved = models[typ][objname].get("default", None)
+        if resolved is not None:
+            return resolved
+    return None

redbot/cogs/permissions/permissions.py

@@ -7,10 +7,12 @@ from redbot.core.bot import Red
 from redbot.core import checks
 from redbot.core.config import Config
 from redbot.core.i18n import Translator, cog_i18n
+from redbot.core.utils.caching import LRUDict
 
-from .resolvers import val_if_check_is_valid, resolve_models
+from .resolvers import val_if_check_is_valid, resolve_models, entries_from_ctx
 from .yaml_handler import yamlset_acl, yamlget_acl
 from .converters import CogOrCommand, RuleType
+from .mass_resolution import mass_resolve
 
 _models = ["owner", "guildowner", "admin", "mod", "all"]
 
@@ -35,8 +37,32 @@ class Permissions:
         self.config = Config.get_conf(self, identifier=78631113035100160, force_registration=True)
         self.config.register_global(owner_models={})
         self.config.register_guild(owner_models={})
+        self.cache = LRUDict(size=25000)  # This can be tuned later
 
-    async def __global_check(self, ctx):
+    async def get_user_ctx_overrides(self, ctx: commands.Context) -> dict:
+        """
+        This takes a context object, and returns a dict of
+
+        allowed: list of commands
+        denied: list of commands
+        default: list of commands
+
+        representing how permissions interacts with the
+        user, channel, guild, and (possibly) voice channel
+        for all commands on the bot (not just the one in the context object)
+
+        This mainly exists for use by the help formatter,
+        but others may find it useful
+
+        Unlike the rest of the permission system, if other models are added later,
+        due to optimizations made for this, this needs to be adjusted accordingly
+
+        This does not account for before and after permission hooks,
+        these need to be checked seperately
+        """
+        return await mass_resolve(ctx=ctx, config=self.config)
+
+    async def __global_check(self, ctx: commands.Context) -> bool:
         """
         Yes, this is needed on top of hooking into checks.py
         to ensure that unchecked commands can still be managed by permissions
@@ -69,12 +95,6 @@ class Permissions:
         """
         if await ctx.bot.is_owner(ctx.author):
             return True
-        voice_channel = None
-        with contextlib.suppress(Exception):
-            voice_channel = ctx.author.voice.voice_channel
-        entries = [x for x in (ctx.author, voice_channel, ctx.channel) if x]
-        roles = sorted(ctx.author.roles, reverse=True) if ctx.guild else []
-        entries.extend([x.id for x in roles])
 
         before = [
             getattr(cog, "_{0.__class__.__name__}__red_permissions_before".format(cog), None)
@@ -87,11 +107,26 @@ class Permissions:
             if override is not None:
                 return override
 
-        for model in self.resolution_order[level]:
+        # checked ids + configureable to be checked against
-            override_model = getattr(self, model + "_model", None)
+        cache_tup = entries_from_ctx(ctx) + (
-            override = await override_model(ctx) if override_model else None
+            ctx.cog.__class__.__name__,
+            ctx.command.qualified_name,
+        )
+        if cache_tup in self.cache:
+            override = self.cache[cache_tup]
             if override is not None:
                 return override
+        else:
+            for model in self.resolution_order[level]:
+                if ctx.guild is None and model != "owner":
+                    break
+                override_model = getattr(self, model + "_model", None)
+                override = await override_model(ctx) if override_model else None
+                if override is not None:
+                    self.cache[cache_tup] = override
+                    return override
+            # This is intentional not being in an else block
+            self.cache[cache_tup] = None
 
         after = [
             getattr(cog, "_{0.__class__.__name__}__red_permissions_after".format(cog), None)
@@ -116,7 +151,8 @@ class Permissions:
         """
         Handles guild level overrides
         """
-
+        if ctx.guild is None:
+            return None
         async with self.config.guild(ctx.guild).owner_models() as models:
             return resolve_models(ctx=ctx, models=models)
 
@@ -224,6 +260,7 @@ class Permissions:
             return await ctx.send(_("Invalid syntax."))
         else:
             await ctx.send(_("Rules set."))
+            self.invalidate_cache()
 
     @checks.is_owner()
     @permissions.command(name="getglobalacl")
@@ -250,6 +287,7 @@ class Permissions:
             return await ctx.send(_("Invalid syntax."))
         else:
             await ctx.send(_("Rules set."))
+            self.invalidate_cache(ctx.guild.id)
 
     @commands.guild_only()
     @checks.guildowner_or_permissions(administrator=True)
@@ -279,6 +317,7 @@ class Permissions:
             return await ctx.send(_("Invalid syntax."))
         else:
             await ctx.send(_("Rules set."))
+            self.invalidate_cache(ctx.guild.id)
 
     @checks.is_owner()
     @permissions.command(name="updateglobalacl")
@@ -298,6 +337,7 @@ class Permissions:
             return await ctx.send(_("Invalid syntax."))
         else:
             await ctx.send(_("Rules set."))
+            self.invalidate_cache()
 
     @checks.is_owner()
     @permissions.command(name="addglobalrule")
@@ -341,6 +381,7 @@ class Permissions:
             data[model_type][type_name][allow_or_deny].append(obj)
             models.update(data)
         await ctx.send(_("Rule added."))
+        self.invalidate_cache(type_name, obj)
 
     @commands.guild_only()
     @checks.guildowner_or_permissions(administrator=True)
@@ -385,6 +426,7 @@ class Permissions:
             data[model_type][type_name][allow_or_deny].append(obj)
             models.update(data)
         await ctx.send(_("Rule added."))
+        self.invalidate_cache(type_name, obj)
 
     @checks.is_owner()
     @permissions.command(name="removeglobalrule")
@@ -428,6 +470,7 @@ class Permissions:
             data[model_type][type_name][allow_or_deny].remove(obj)
             models.update(data)
         await ctx.send(_("Rule removed."))
+        self.invalidate_cache(obj, type_name)
 
     @commands.guild_only()
     @checks.guildowner_or_permissions(administrator=True)
@@ -472,6 +515,7 @@ class Permissions:
             data[model_type][type_name][allow_or_deny].remove(obj)
             models.update(data)
         await ctx.send(_("Rule removed."))
+        self.invalidate_cache(obj, type_name)
 
     @commands.guild_only()
     @checks.guildowner_or_permissions(administrator=True)
@@ -502,6 +546,7 @@ class Permissions:
 
             models.update(data)
         await ctx.send(_("Default set."))
+        self.invalidate_cache(type_name)
 
     @checks.is_owner()
     @permissions.command(name="setdefaultglobalrule")
@@ -532,6 +577,7 @@ class Permissions:
 
             models.update(data)
         await ctx.send(_("Default set."))
+        self.invalidate_cache(type_name)
 
     @checks.is_owner()
     @permissions.command(name="clearglobalsettings")
@@ -540,6 +586,7 @@ class Permissions:
         Clears all global rules.
         """
         await self._confirm_then_clear_rules(ctx, is_guild=False)
+        self.invalidate_cache()
 
     @commands.guild_only()
     @checks.guildowner_or_permissions(administrator=True)
@@ -549,6 +596,7 @@ class Permissions:
         Clears all guild rules.
         """
         await self._confirm_then_clear_rules(ctx, is_guild=True)
+        self.invalidate_cache(ctx.guild.id)
 
     async def _confirm_then_clear_rules(self, ctx: commands.Context, is_guild: bool):
         if ctx.guild.me.permissions_in(ctx.channel).add_reactions:
@@ -588,6 +636,20 @@ class Permissions:
         else:
             await ctx.send(_("Okay."))
 
+    def invalidate_cache(self, *to_invalidate):
+        """
+        Either invalidates the entire cache (if given no objects)
+        or does a partial invalidation based on passed objects
+        """
+        if len(to_invalidate) == 0:
+            self.cache.clear()
+            return
+        # LRUDict inherits from ordered dict, hence the syntax below
+        stil_valid = [
+            (k, v) for k, v in self.cache.items() if not any(obj in k for obj in to_invalidate)
+        ]
+        self.cache = LRUDict(*stil_valid, size=self.cache.size)
+
     def find_object_uniquely(self, info: str) -> int:
         """
         Finds an object uniquely, returns it's id or returns None

redbot/cogs/permissions/resolvers.py

@@ -7,6 +7,23 @@ from redbot.core import commands
 log = logging.getLogger("redbot.cogs.permissions.resolvers")
 
 
+def entries_from_ctx(ctx: commands.Context) -> tuple:
+    voice_channel = None
+    with contextlib.suppress(Exception):
+        voice_channel = ctx.author.voice.voice_channel
+    entries = [x.id for x in (ctx.author, voice_channel, ctx.channel) if x]
+    roles = sorted(ctx.author.roles, reverse=True) if ctx.guild else []
+    entries.extend([x.id for x in roles])
+    # entries now contains the following (in order) (if applicable)
+    # author.id
+    # author.voice.voice_channel.id
+    # channel.id
+    # role.id for each role (highest to lowest)
+    # (implicitly) guild.id because
+    #     the @everyone role shares an id with the guild
+    return tuple(entries)
+
+
 async def val_if_check_is_valid(*, ctx: commands.Context, check: object, level: str) -> bool:
     """
     Returns the value from a check if it is valid
@@ -56,23 +73,7 @@ def resolve_lists(*, ctx: commands.Context, whitelist: list, blacklist: list) ->
     """
     resolves specific lists
     """
-
+    for entry in entries_from_ctx(ctx):
-    voice_channel = None
-    with contextlib.suppress(Exception):
-        voice_channel = ctx.author.voice.voice_channel
-
-    entries = [x.id for x in (ctx.author, voice_channel, ctx.channel) if x]
-    roles = sorted(ctx.author.roles, reverse=True) if ctx.guild else []
-    entries.extend([x.id for x in roles])
-    # entries now contains the following (in order) (if applicable)
-    # author.id
-    # author.voice.voice_channel.id
-    # channel.id
-    # role.id for each role (highest to lowest)
-    # (implicitly) guild.id because
-    #     the @everyone role shares an id with the guild
-
-    for entry in entries:
         if entry in whitelist:
             return True
         if entry in blacklist:

redbot/core/utils/caching.py

@@ -0,0 +1,53 @@
+import collections
+
+
+class LRUDict:
+    """
+    dict with LRU-eviction and max-size
+
+    This is intended for caching, it may not behave how you want otherwise
+
+    This uses collections.OrderedDict under the hood, but does not directly expose
+    all of it's methods (intentional)
+    """
+
+    def __init__(self, *keyval_pairs, size):
+        self.size = size
+        self._dict = collections.OrderedDict(*keyval_pairs)
+
+    def __contains__(self, key):
+        if key in self._dict:
+            self._dict.move_to_end(key, last=True)
+            return True
+        return False
+
+    def __getitem__(self, key):
+        ret = self._dict.__getitem__(key)
+        self._dict.move_to_end(key, last=True)
+        return ret
+
+    def __setitem__(self, key, value):
+        if key in self._dict:
+            self._dict.move_to_end(key, last=True)
+        self._dict[key] = value
+        if len(self._dict) > self.size:
+            self._dict.popitem(last=False)
+
+    def __delitem__(self, key):
+        return self._dict.__delitem__(key)
+
+    def clear(self):
+        return self._dict.clear()
+
+    def pop(self, key):
+        return self._dict.pop(key)
+
+    # all of the below access all of the items, and therefore shouldnt modify the ordering for eviction
+    def keys(self):
+        return self._dict.keys()
+
+    def items(self):
+        return self._dict.items()
+
+    def values(self):
+        return self._dict.values()