[Patchew-devel] [PATCH] optimize mod

[Paolo Bonzini]

On 21/11/18 14:52, Caio Carrara wrote:
> 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.

Right.

>> -        _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.

You're absolutely right, this is gone in v2 fortunately.  Removing
enabled is much simpler.

>> +        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()

I think we should remove the "try/except" altogether (this patch was
just reindenting it).  Fam?

Paolo