On Wed, 02 Nov 2011, Jan Cholasta wrote:
> >Callable instances are a consequence of the above --
> >Command.__call__() does use properties that are changed due to
> >finalize() being run. In fact, there are so many places in FreeIPA
> >where we simply expect that foo.args/params/output_params/output is
> >both iterable and callable instance that it is not even funny.
> >
> >Now, the process established by the proposed patch is actually
> >dependent only on the fact that bound instance has finalize() method.
> >This is quite generic and can be used for all other types of objects.
> >
> The real problem is in the API class so it should be fixed there,
> not worked around in Plugin subclasses.
Please explain why you do think real problem is in API class. 
Finalize() is needed purely to being able to split plugins' property 
initialization into stages before loading plugins and after due to the 
fact that each and every plugin could contribute commands and 
additions to the same objects. While these stages are separate, 
plugins can access properties and commands they want and it is duty of 
a plugin to get its property working right. We have finalize() method 
exactly for this purpose.

API class' attempt to call finalize() on each plugin's instance at 
once is a poor replacement to detecting access to 
not-yet-initialized properties. We can implement that access like you 
have proposed with __getattribute__ but it has additional cost for all 
other properties that don't need post-initialization (finalization) 
and, what's more important, they incur these costs irrespective 
whether initialization has happened or not. That's bad and my patch 
shows it is actually noticeable as the performance difference, for 
example, of 'ipa dnsrecord-find example.com' between the two patches 
is about 2x in favor of my patch (1.624s vs 0.912s).

Regarding other objects, here is the list of places where we define 
specific finalizers:
$ git grep 'def finalize'
ipalib/cli.py:    def finalize(self):
ipalib/frontend.py:    def finalize(self):
ipalib/plugable.py:    def finalize(self):
ipalib/plugable.py:    def finalize(self):
ipaserver/rpcserver.py:    def finalize(self):
ipaserver/rpcserver.py:    def finalize(self):
ipaserver/rpcserver.py:    def finalize(self):
tests/test_ipalib/test_cli.py:    def finalize(self):
tests/test_ipalib/test_config.py:    def finalize_core(self, ctx, **defaults):
tests/util.py:    def finalize(self, *plugins, **kw):

As you can see, there NO plugins that define their own finalizers, 
thus, all of them rely on API.finalize(), help.finalize(), and 
Plugin.finalize()/Command.finalize(). ipaserver/rpcserver.py 
finalizers are fine as well, I have checked them.

Updated patch is attached. It addresses all comments from Martin.

[root@vm-114 freeipa-2-1-speedup]# time ipa dnsrecord-find ipa.local >/dev/null

real    0m0.883s
user    0m0.504s
sys     0m0.136s
[root@vm-114 freeipa-2-1-speedup]# time ipa user-find >/dev/null

real    0m0.887s
user    0m0.486s
sys     0m0.141s
[root@vm-114 freeipa-2-1-speedup]# time ipa group-find >/dev/null

real    0m0.933s
user    0m0.446s
sys     0m0.169s
[root@vm-114 freeipa-2-1-speedup]# time ipa help >/dev/null

real    0m0.624s
user    0m0.479s
sys     0m0.133s
[root@vm-114 freeipa-2-1-speedup]# time ipa group >/dev/null

real    0m0.612s
user    0m0.475s
sys     0m0.126s
[root@vm-114 freeipa-2-1-speedup]# time ipa user >/dev/null

real    0m0.617s
user    0m0.472s
sys     0m0.131s

/ Alexander Bokovoy

Freeipa-devel mailing list

Reply via email to