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

[Rob Crittenden]

Jan Cholasta wrote:
> 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
>


Just a couple of questions/clarifications:

1. I think more documentation is needed for on_finalize(). The name 
isn't particularly descriptive. If I read it properly you are providing 
an alternate way to override finalize(), right?

2. Changing to a partition in Attribute is not equivalent to the regular 
expression previously used. Why loosen the requirements?

3. I think you should document in-line where you have block calls to 
finalize_attr() so that future developers know to add the stub.

4. What is the purpose of the read-lock in finalize?

It generally looks good and provides impressive performance improvements.

rob