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

Tue Nov 22 23:01:29 UTC 2011

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

ACK, pushed to master.

rob