mod.py | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-)
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@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
- _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)]
+ for f in memoized_hooks[hook_name]:
+ try:
+ f(**params)
+ except:
+ print("Cannot invoke module hook: %s.%s" % (type(f.__self__), hook_name))
+ traceback.print_exc()
def get_module(name):
return _loaded_modules.get(name)
--
2.19.1
_______________________________________________
Patchew-devel mailing list
Patchew-devel@redhat.com
https://www.redhat.com/mailman/listinfo/patchew-devel
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@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@redhat.com
_______________________________________________
Patchew-devel mailing list
Patchew-devel@redhat.com
https://www.redhat.com/mailman/listinfo/patchew-devel
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@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
_______________________________________________
Patchew-devel mailing list
Patchew-devel@redhat.com
https://www.redhat.com/mailman/listinfo/patchew-devel
On 19/11/18 14:38, 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@redhat.com>
Hmm, this undoes thw "Always read from DB to accept configuration
updates" part, because .enabled() also comes from the DB.
One idea could be to cache enabled() every time a module is saved.
By the way, the enabled() check already doesn't work for the
www_url_hook, which is run only ones. More precisely, it only works
because modules are default-enabled.
Paolo
> ---
> 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
> - _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)]
> + for f in memoized_hooks[hook_name]:
> + try:
> + f(**params)
> + except:
> + print("Cannot invoke module hook: %s.%s" % (type(f.__self__), hook_name))
> + traceback.print_exc()
>
> def get_module(name):
> return _loaded_modules.get(name)
>
_______________________________________________
Patchew-devel mailing list
Patchew-devel@redhat.com
https://www.redhat.com/mailman/listinfo/patchew-devel
On Mon, 11/19 17:01, Paolo Bonzini wrote:
> On 19/11/18 14:38, 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@redhat.com>
>
> Hmm, this undoes thw "Always read from DB to accept configuration
> updates" part, because .enabled() also comes from the DB.
>
> One idea could be to cache enabled() every time a module is saved.
>
> By the way, the enabled() check already doesn't work for the
> www_url_hook, which is run only ones. More precisely, it only works
> because modules are default-enabled.
I'm in favor of dropping the enabled field from the model if it has a
performance advantage.
>
> Paolo
>
> > ---
> > 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
> > - _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:
Nit: I think PEP wants 'hook_name not in ...'?
> > + # 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)]
> > + for f in memoized_hooks[hook_name]:
> > + try:
> > + f(**params)
> > + except:
> > + print("Cannot invoke module hook: %s.%s" % (type(f.__self__), hook_name))
> > + traceback.print_exc()
> >
> > def get_module(name):
> > return _loaded_modules.get(name)
> >
>
> _______________________________________________
> Patchew-devel mailing list
> Patchew-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/patchew-devel
Fam
_______________________________________________
Patchew-devel mailing list
Patchew-devel@redhat.com
https://www.redhat.com/mailman/listinfo/patchew-devel
On 20/11/18 02:33, Fam Zheng wrote:
> On Mon, 11/19 17:01, Paolo Bonzini wrote:
>> On 19/11/18 14:38, 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@redhat.com>
>>
>> Hmm, this undoes thw "Always read from DB to accept configuration
>> updates" part, because .enabled() also comes from the DB.
>>
>> One idea could be to cache enabled() every time a module is saved.
>>
>> By the way, the enabled() check already doesn't work for the
>> www_url_hook, which is run only ones. More precisely, it only works
>> because modules are default-enabled.
>
> I'm in favor of dropping the enabled field from the model if it has a
> performance advantage.
Ok.
>>> 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
>>> - _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:
>
> Nit: I think PEP wants 'hook_name not in ...'?
Yep.
Paolo
_______________________________________________
Patchew-devel mailing list
Patchew-devel@redhat.com
https://www.redhat.com/mailman/listinfo/patchew-devel
© 2016 - 2024 Red Hat, Inc.