[Freeipa-devel] [PATCH] 1 Do lazy initializiation ipalib

[Jan Cholasta]

Dne 2.11.2011 17:38, Alexander Bokovoy napsal(a):
> (actual patch attached!)
>
> On Wed, 02 Nov 2011, Alexander Bokovoy wrote:
>> 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 at vm-114 freeipa-2-1-speedup]# time ipa dnsrecord-find ipa.local>/dev/null
>>
>> real    0m0.883s
>> user    0m0.504s
>> sys     0m0.136s
>> [root at vm-114 freeipa-2-1-speedup]# time ipa user-find>/dev/null
>>
>> real    0m0.887s
>> user    0m0.486s
>> sys     0m0.141s
>> [root at vm-114 freeipa-2-1-speedup]# time ipa group-find>/dev/null
>>
>> real    0m0.933s
>> user    0m0.446s
>> sys     0m0.169s
>> [root at vm-114 freeipa-2-1-speedup]# time ipa help>/dev/null
>>
>> real    0m0.624s
>> user    0m0.479s
>> sys     0m0.133s
>> [root at vm-114 freeipa-2-1-speedup]# time ipa group>/dev/null
>>
>> real    0m0.612s
>> user    0m0.475s
>> sys     0m0.126s
>> [root at vm-114 freeipa-2-1-speedup]# time ipa user>/dev/null
>>
>> real    0m0.617s
>> user    0m0.472s
>> sys     0m0.131s
>>
>> --
>> / Alexander Bokovoy
>

The problem with the API class is that it creates all the plugin 
instances every time. Both mine and your patch don't change that, they 
deal only with finalization, which is the easier part of the lazy 
initialization problem. Additionally, in your patch finalize() is called 
only on Command subclasses, so non-Command plugins are not 
ReadOnly-locked like they should (currently this is done only when 
production mode is off).

We could change API so that the plugins are initialized on-demand. That 
way we could probably get rid off finalize() on plugins altogether and 
move the initialization code into __init__() (because all the required 
information would be available on-demand) and the locking into API. The 
flaw of this approach is that it would require a number of fundamental 
changes, namely changing the way API class handles plugin initialization 
and moving the initialization of "static" Plugin properties (name, 
module, fullname, bases, label, ...) from instance level to class level. 
(Nonetheless, I think API would't suffer from a facelift - currently it 
is messy, hard to read code, with bits nobody ever used or uses anymore, 
some of the docstrings and comments aren't correct, etc.)

Anyway, if we decide to go with your solution, please make sure *all* 
the plugins that are used are finalized (because of the locking) and add 
a new api.env attribute which controls the lazy finalization, so that 
the behavior can be overriden (actually I have already done that - see 
attached patch - just use "api.env.plugins_on_demand" instead of 
"api.env.context == 'cli'").

Honza

-- 
Jan Cholasta
-------------- next part --------------
A non-text attachment was scrubbed...
Name: lazycontrol.patch
Type: text/x-patch
Size: 1384 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20111103/328bff38/attachment.bin>