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

[Jan Cholasta]

Dne 3.11.2011 12:43, Alexander Bokovoy napsal(a):
> On Thu, 03 Nov 2011, Jan Cholasta wrote:
>> The problem with the API class is that it creates all the plugin
>> instances every time. Both mine and your patch don't change that,
>> they deal only with finalization, which is the easier part of the
>> lazy initialization problem. Additionally, in your patch finalize()
>> is called only on Command subclasses, so non-Command plugins are not
>> ReadOnly-locked like they should (currently this is done only when
>> production mode is off).
> I ended up with explicitly finalizing Object, Property, and Backend
> objects. This, by the way, proved that major slowdown is brought by
> the Command and Method instances as I've got statistically negligible
> variations of execution time, within less than 1%.
>
>> We could change API so that the plugins are initialized on-demand.
>> That way we could probably get rid off finalize() on plugins
>> altogether and move the initialization code into __init__() (because
>> all the required information would be available on-demand) and the
>> locking into API. The flaw of this approach is that it would require
>> a number of fundamental changes, namely changing the way API class
>> handles plugin initialization and moving the initialization of
>> "static" Plugin properties (name, module, fullname, bases, label,
>> ...) from instance level to class level. (Nonetheless, I think API
>> 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 :)

>
> One important feature I'd like to still keep in FreeIPA is ability to
> extend any object and action during plugin import phase regardless of
> the python source file it comes from. So far our split between 'user
> methods' and 'dnsrecord methods' is mostly utilitarian as they are
> implemented in their own source code files. However, nobody prevents
> you from implementing all needed modifications to all classes in the
> single source file and I expect this to be fairly typical to
> site-specific cases.
>
> 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'").
> 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?

      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.

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

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.

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

Honza

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