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

[Jan Cholasta]

Dne 9.11.2011 11:59, Alexander Bokovoy napsal(a):
> On Wed, 09 Nov 2011, Jan Cholasta wrote:
>>>> 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.)
>>> A review of API class is a good idea. However, I think it is currently
>>> enough what is in proposed patch as it gets you 2-3x speed increase
>>> already.
>>
>> I've already started experimenting with the API class, hopefully it
>> will result in something useful :)
> Good.
>
>>> This is something I'd like to keep as it has great value for all
>>> end-users and it requires loading all Python files in ipalib/plugins
>>> (and in ipaserver/plugins for server side).
>>
>> I'm not suggesting we should skip importing the modules. The plugin
>> initialization process consists of these 3 steps:
>>
>>   1. import plugin modules
>>   2. create plugin instances
>>   3. finalize plugin instances
>>
>> What I'm suggesting is that we do steps 2 and 3 on-demand. We can't
>> do step 1 on-demand, because we have no way of knowing what plugins
>> are available without importing the modules. (Both your and my patch
>> does only step 3 on-demand, but I think we can do better than that.)
> Agreed.
>
>>>> 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'").
>>> Done.
>>>
>>
>> +        if not self.__dict__['_Plugin__finalized']:
>> +            self.finalize()
>>
>> This doesn't seem right, coding style-wise. If you want to access
>> the property from outside the Plugin class, why did you name-mangle
>> it in the first place?
> It is supposed to be internal detail of a Plugin. I agree it stinks a
> bit here. :)
>
>
>>       def finalize(self):
>>           """
>>           """
>> +        if not self.__finalized:
>> +            self.__finalized = True
>> +        else:
>> +            return
>>           if not is_production_mode(self):
>>               lock(self)
>>
>> Finalize is supposed to be called just once, so IMO it would be
>> better not to do the check and let it throw the exception.
> On contrary, I think sequential finalize() calls should do nothing.
> This is at least what I expect from a finalized object -- no-op.

In my patch, finalize() throws an exception if called more than once, 
but ensure_finalized() does nothing for plugins that are already 
finalized. So there are both kinds of behavior available.

>
>> +        for i in ('Object', 'Property', 'Backend'):
>> +            if i in self:
>> +                namespaces.append(self[i])
>>
>> AFAIK the framework is supposed to be generally usable for other
>> projects in the future. With that in mind, I think we can't afford
>> to hard-code things like this.
> That's true. As you managed to get around without hardcoding, we can
> drop this part.
>
>> Anyway, I've made a patch that uses data descriptors for the trap
>> objects (which is IMHO more elegant than what you do). I've also
>> managed to add on-demand finalization to Object and Attribute
>> subclasses by moving the set-up code from set_api() to finalize()
>> (it doesn't add much performance, though). See attachment.
> I read through the code and I like it! Did you get the whole test
> suite running with it? There are some parts of tests that expect
> non-finalized objects to have None properties while they are now not
> None.

I always run the test suite ;-)

All the unit tests succeed (they do when run with my patch 54 applied, 
which fixes failures of some unrelated unit tests).

>
> If so, preliminary ACK for your patch from reading it if you would
> make sure on_finalize() allows multiple calls and would make a better
> commit message. ;)

on_finalize() shouldn't be called directly (perhaps I should rename it 
to _on_finalize to emphasize that...?)

I'll work on the commit message. As usual, it is the hardest part of the 
patch for me.

>
> I'll try to arrange testing myself today/tomorrow.
>
>> The numbers from my VM:
>>
>>          master      abbra       jcholast
>> $ time ipa
>> real    0m1.288s    0m0.619s    0m0.601s
>> user    0m1.103s    0m0.478s    0m0.451s
>> sys     0m0.161s    0m0.126s    0m0.133s
>>
>> $ time ipa user-find
>> real    0m1.897s    0m1.253s    0m1.235s
>> user    0m1.119s    0m0.486s    0m0.488s
>> sys     0m0.160s    0m0.160s    0m0.136s
>>
>> $ time ipa help
>> real    0m1.299s    0m0.629s    0m0.600s
>> user    0m1.094s    0m0.477s    0m0.446s
>> sys     0m0.183s    0m0.136s    0m0.140s
> Looks good (your VM is supposedly at the same farm as mine so numbers
> are comparable). There is anyway great variation in execution times in
> VMs so 600ms vs 629ms is roughly the same.

Yep.

>
> Thanks a lot! I think it is great advance over the original approach.

Thanks for the kind words :-)

Honza

-- 
Jan Cholasta