[Patchew-devel] [PATCH] optimize mod

[Caio Carrara]

Hello, Paolo.

Sorry taking so long to review it.

On Mon, Nov 19, 2018 at 02:38:35PM +0100, Paolo Bonzini wrote:
> Avoid looking up the module table repeatedly every time a hook is invoked,
> by memoizing the list of hooks.  As a small additional optimization reuse
> the result of get_or_create instead of querying the table again.
> 
> Signed-off-by: Paolo Bonzini <pbonzini at redhat.com>
> ---
>  mod.py | 23 ++++++++++++++---------
>  1 file changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/mod.py b/mod.py
> index e92377a..1da12ec 100644
> --- a/mod.py
> +++ b/mod.py
> @@ -26,8 +26,7 @@ class PatchewModule(object):
>      def get_model(self):
>          # ALways read from DB to accept configuration update in-flight
>          from api.models import Module as PC

Since you're not using PC anymore I think you can drop this import.

> -        _module_init_config(self.__class__)
> -        return PC.objects.get(name=self.name)
> +        return _module_init_config(self.__class__)
>  
>      def enabled(self):
>          try:
> @@ -186,14 +185,20 @@ def load_modules():
>              _loaded_modules[cls.name] = cls()
>              print("Loaded module:", cls.name)
>  
> +memoized_hooks = {}
>  def dispatch_module_hook(hook_name, **params):
> -    for i in [x for x in list(_loaded_modules.values()) if x.enabled()]:
> -        if hasattr(i, hook_name):
> -            try:
> -                getattr(i, hook_name)(**params)
> -            except:
> -                print("Cannot invoke module hook: %s.%s" % (i, hook_name))
> -                traceback.print_exc()
> +    if not hook_name in memoized_hooks:
> +        # The dictionary stores "bound method" objects
> +        memoized_hooks[hook_name] = [getattr(x, hook_name)
> +            for x in list(_loaded_modules.values())
> +            if x.enabled()
> +            and hasattr(x, hook_name)]

s/x/module/

Also, I'm pretty sure you don't need to convert dict.values() return to
a list object. The values() return is already a iterable.

Other than that, there are some people (I'm included) that think list
comprehension should be avoided when it takes more than 2 lines of code.
Mostly because of readability. What you think about this proposal:

# create a function to return the enabled modules:
def _enabled_modules():
    return [module for module in _loaded_modules.values() if module.enabled()]

# In dispatch_module() use a simple for loop that makes things more
explicit:
```
if not hook_name in memoized_hooks:
    for module in _enabled_modules():
        if not hasattr(module, hook_name):
            continue
        memoized_hooks[hook_name] = getattr(module, hook_name)
```

> +    for f in memoized_hooks[hook_name]:

s/f/hook/

> +        try:
> +            f(**params)
> +        except:

We should avoid using bare except. If you want to get any exception use
at least `except Exception`. But actually the proper way to do it is to
specify the exception that should be handled by except block.

> +            print("Cannot invoke module hook: %s.%s" % (type(f.__self__), hook_name))
> +            traceback.print_exc()
>  
>  def get_module(name):
>      return _loaded_modules.get(name)
{...}

-- 
Caio Carrara
Software Engineer, Virt Team - Red Hat
ccarrara at redhat.com