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. 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). > 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. -- / Alexander Bokovoy
>From 44ebebc2fede6f001a826fa4047abfeb02195cac Mon Sep 17 00:00:00 2001 From: Alexander Bokovoy <aboko...@redhat.com> Date: Wed, 2 Nov 2011 12:24:20 +0200 Subject: [PATCH] Perform late initialization of FreeIPA plugins https://fedorahosted.org/freeipa/ticket/1336 When plugins are loaded, instances of the provided objects and commands are registered in the API instance. The patch changes finalization process to apply on first access to the Command instance that would cause either access to properties (args, options, params) or execution of the command itself. The patch gives 2x boost for client-side commands like help and 3x boost for commands that go to the server side. All performance numbers are approximate and may vary a lot. --- ipalib/config.py | 4 ++ ipalib/constants.py | 1 + ipalib/frontend.py | 57 ++++++++++++++++++++++++++++++++++++ ipalib/plugable.py | 28 +++++++++++++++-- makeapi | 1 + tests/test_ipalib/test_frontend.py | 6 ++-- 6 files changed, 90 insertions(+), 7 deletions(-) diff --git a/ipalib/config.py b/ipalib/config.py index 410e5f0b252fc423e9c673e4f5b62e50eaf3602e..5e3ef8d9bc529dea59890bc8974b1d455113b14c 100644 --- a/ipalib/config.py +++ b/ipalib/config.py @@ -492,6 +492,10 @@ class Env(object): if 'conf_default' not in self: self.conf_default = self._join('confdir', 'default.conf') + # Set plugins_on_demand: + if 'plugins_on_demand' not in self: + self.plugins_on_demand = (self.context == 'cli') + def _finalize_core(self, **defaults): """ Complete initialization of standard IPA environment. diff --git a/ipalib/constants.py b/ipalib/constants.py index 6d246288b0bb6ad3509fdb62616a03d678312319..7ec897b58786b5d7047d5fbdafb655b7d71a5400 100644 --- a/ipalib/constants.py +++ b/ipalib/constants.py @@ -188,6 +188,7 @@ DEFAULT_CONFIG = ( ('confdir', object), # Directory containing config files ('conf', object), # File containing context specific config ('conf_default', object), # File containing context independent config + ('plugins_on_demand', object), # Whether to finalize plugins on-demand (bool) # Set in Env._finalize_core(): ('in_server', object), # Whether or not running in-server (bool) diff --git a/ipalib/frontend.py b/ipalib/frontend.py index 61e7f493f8a8e30a1a189d06cd6a69893319deaf..92380b0b0ce59b6888b394dd2595faa0b1107e33 100644 --- a/ipalib/frontend.py +++ b/ipalib/frontend.py @@ -23,6 +23,7 @@ Base classes for all front-end plugins. import re import inspect +import types from base import lock, check_name, NameSpace from plugable import Plugin, is_production_mode from parameters import create_param, parse_param_spec, Param, Str, Flag, Password @@ -64,6 +65,46 @@ def entry_count(entry): return num_entries +class _MirrorTrap(object): + """ + _MirrorTrap is a special class to support late initialization in FreeIPA. + _MirrorTrap instances can be used instead of None for properties that + change their state due to Plugin#finalize() method called. + + As Plugin#finalize() might be computationally expensive, objects that could not + act without such properties, should initialize them to _MirrorTrap() instance + in their __init__() method so that _MirrorTrap instance would force the object + to finalize() before returning the value of the property. + + Usage pattern is following: + self.args = _MirrorTrap(self, 'args') + + Pass the reference to the object holding the attribute and the name of the attribute. + On first access to the attribute, _MirrorTrip will try to call object's finalize() method + and expects that the attribute will be re-assigned with new (proper) value. + + In many places in FreeIPA, an attribute gets assigned with NameSpace() instance during + finalize() call. + """ + def __init__(self, master, attr): + self.master = master + self.attr = attr + + def __call__(self, **args): + self.master.finalize() + # At this point master.attr points to proper object + return getattr(self.master, self.attr)(**args) + + def __iter__(self): + self.master.finalize() + return getattr(self.master, self.attr).__iter__() + + def __len__(self): + self.master.finalize() + return getattr(self.master, self.attr).__len__() + +def is_not_initialized(attr): + return isinstance(attr, (types.NoneType, _MirrorTrap)) class HasParam(Plugin): """ @@ -404,6 +445,14 @@ class Command(HasParam): msg_summary = None msg_truncated = _('Results are truncated, try a more specific search') + def __init__(self): + self.args = _MirrorTrap(self, 'args') + self.options = _MirrorTrap(self, 'options') + self.output_params = _MirrorTrap(self, 'output_params') + self.output = _MirrorTrap(self, 'output') + + super(Command, self).__init__() + def __call__(self, *args, **options): """ Perform validation and then execute the command. @@ -411,6 +460,10 @@ class Command(HasParam): If not in a server context, the call will be forwarded over XML-RPC and the executed an the nearest IPA server. """ + # Plugin instance must be finalized before we get to execution + if not self.__dict__['_Plugin__finalized']: + self.finalize() + params = self.args_options_2_params(*args, **options) self.debug( 'raw: %s(%s)', self.name, ', '.join(self._repr_iter(**params)) @@ -769,6 +822,10 @@ class Command(HasParam): loaded in self.api to determine what their custom `Command.get_args` and `Command.get_options` methods should yield. """ + if self.__dict__['_Plugin__finalized']: + # Already finalized, instance is locked and modifications will + # cause exceptions. + return self._create_param_namespace('args') if len(self.args) == 0 or not self.args[-1].multivalue: self.max_args = len(self.args) diff --git a/ipalib/plugable.py b/ipalib/plugable.py index b0e415656e0428eb164c35a2862fcfbf50883381..3e7305a9c5d2919e8c7a3f928871b0698c4f3af0 100644 --- a/ipalib/plugable.py +++ b/ipalib/plugable.py @@ -207,6 +207,7 @@ class Plugin(ReadOnly): self.label ) ) + self.__finalized = False def __get_api(self): """ @@ -220,6 +221,10 @@ class Plugin(ReadOnly): def finalize(self): """ """ + if not self.__finalized: + self.__finalized = True + else: + return if not is_production_mode(self): lock(self) @@ -637,10 +642,25 @@ class API(DictProxy): if not production_mode: assert p.instance.api is self - for p in plugins.itervalues(): - p.instance.finalize() - if not production_mode: - assert islocked(p.instance) is True + # Always finalize objects, their properties, and backends + namespaces = [] + for i in ('Object', 'Property', 'Backend'): + if i in self: + namespaces.append(self[i]) + for n in namespaces: + for p in n: + n[p].finalize() + + # Finalize remaining plugins only if forced to do so + # Otherwise they will get finalized on first access to + # properties that are affected by finalization + if not self.env.plugins_on_demand: + for p in plugins.itervalues(): + if not p.instance.__dict__['_Plugin__finalized']: + p.instance.finalize() + if not production_mode: + assert islocked(p.instance) + object.__setattr__(self, '_API__finalized', True) tuple(PluginInfo(p) for p in plugins.itervalues()) object.__setattr__(self, 'plugins', diff --git a/makeapi b/makeapi index af3b0f96bbb6cc1a705c407561a44d8d25d26485..f68cd6254f278d945e22e092d7f3610e5638c02b 100755 --- a/makeapi +++ b/makeapi @@ -357,6 +357,7 @@ def main(): validate_api=True, enable_ra=True, mode='developer', + plugins_on_demand=False, ) api.bootstrap(**cfg) diff --git a/tests/test_ipalib/test_frontend.py b/tests/test_ipalib/test_frontend.py index 0f6aecb3d137872fd89e89f9db64e1d9aab7d8c1..206e1f1f2ed759893019b9679393ded78d1e0452 100644 --- a/tests/test_ipalib/test_frontend.py +++ b/tests/test_ipalib/test_frontend.py @@ -252,7 +252,7 @@ class test_Command(ClassChecker): """ Test the ``ipalib.frontend.Command.args`` instance attribute. """ - assert self.cls().args is None + assert frontend.is_not_initialized(self.cls().args) is True o = self.cls() o.finalize() assert type(o.args) is plugable.NameSpace @@ -301,7 +301,7 @@ class test_Command(ClassChecker): """ Test the ``ipalib.frontend.Command.options`` instance attribute. """ - assert self.cls().options is None + assert frontend.is_not_initialized(self.cls().options) is True o = self.cls() o.finalize() assert type(o.options) is plugable.NameSpace @@ -323,7 +323,7 @@ class test_Command(ClassChecker): Test the ``ipalib.frontend.Command.output`` instance attribute. """ inst = self.cls() - assert inst.output is None + assert frontend.is_not_initialized(inst.output) is True inst.finalize() assert type(inst.output) is plugable.NameSpace assert list(inst.output) == ['result'] -- 1.7.7
_______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
