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
> I've already started experimenting with the API class, hopefully it
> will result in something useful :)
> >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.)
> >>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'").
> + 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):
> 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
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
Freeipa-devel mailing list