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

[Alexander Bokovoy]

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.

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

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

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.

Thanks a lot! I think it is great advance over the original approach.
-- 
/ Alexander Bokovoy