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

[Jan Cholasta]

Dne 15.11.2011 20:10, Rob Crittenden napsal(a):
> 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?

Yes. I have split finalize() to finalize() (the method to be called to 
finalize the plugin) and on_finalize() (the method to be overridden by 
plugin developer to implement custom finalization), so that I can guard 
the finalizing code from recursively calling itself (which happens when 
you try to read an unfinalized finalize_attr attribute from within a 
finalize() call).

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

Well, I didn't think it through. I have reverted the change.

>
> 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.

OK.

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

I guess it is the usual - to prevent modifying the object after the API 
is finalized. The commit that introduced it is 
<https://fedorahosted.org/freeipa/changeset/0edb22c9ac70c5acfab51318810f693d59fab955>, 
the commit that made it conditional is 
<https://fedorahosted.org/freeipa/changeset/359d54e741877f04b0773fb0955041eee7ec0054>.

>
> It generally looks good and provides impressive performance improvements.
>
> rob

Updated patch attached.

Honza

-- 
Jan Cholasta
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-57.2-lazy-finalize.patch
Type: text/x-patch
Size: 14791 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20111121/ce9afb08/attachment.bin>