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
>From 49910cb09d60c8b8c728234f5b6eedb24ab0583d Mon Sep 17 00:00:00 2001
From: Jan Cholasta <jchol...@redhat.com>
Date: Thu, 3 Nov 2011 06:42:17 -0400
Subject: [PATCH] Add on-demand finalization of plugins.
Command, Object and Attribute subclasses are finalized on-demand
(only in CLI by default). This is done by finalizing plugins when
access to attributes that are available only after finalization
is detected.
ticket 1336
---
ipalib/cli.py | 4 +-
ipalib/config.py | 4 ++
ipalib/constants.py | 1 +
ipalib/frontend.py | 57 +++++++++++++++------------
ipalib/plugable.py | 75 +++++++++++++++++++++++++++++++++---
ipaserver/rpcserver.py | 12 +++---
makeapi | 1 +
tests/test_ipalib/test_frontend.py | 3 +-
8 files changed, 116 insertions(+), 41 deletions(-)
diff --git a/ipalib/cli.py b/ipalib/cli.py
index 7fe8087..68d0914 100644
--- a/ipalib/cli.py
+++ b/ipalib/cli.py
@@ -684,7 +684,7 @@ class help(frontend.Local):
mcl = max((self._topics[topic_name][1], len(mod_name)))
self._topics[topic_name][1] = mcl
- def finalize(self):
+ def on_finalize(self):
# {topic: ["description", mcl, {"subtopic": ["description", mcl, [commands]]}]}
# {topic: ["description", mcl, [commands]]}
self._topics = {}
@@ -736,7 +736,7 @@ class help(frontend.Local):
len(s) for s in (self._topics.keys() + [c.name for c in self._builtins])
)
- super(help, self).finalize()
+ super(help, self).on_finalize()
def run(self, key):
name = from_cli(key)
diff --git a/ipalib/config.py b/ipalib/config.py
index 410e5f0..5e3ef8d 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 6d24628..7ec897b 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 61e7f49..47714e7 100644
--- a/ipalib/frontend.py
+++ b/ipalib/frontend.py
@@ -388,17 +388,19 @@ class Command(HasParam):
ipalib.frontend.my_command()
"""
+ finalize_early = False
+
takes_options = tuple()
takes_args = tuple()
- args = None
- options = None
- params = None
+ args = Plugin.finalize_attr('args')
+ options = Plugin.finalize_attr('options')
+ params = Plugin.finalize_attr('params')
obj = None
use_output_validation = True
- output = None
+ output = Plugin.finalize_attr('output')
has_output = ('result',)
- output_params = None
+ output_params = Plugin.finalize_attr('output_params')
has_output_params = tuple()
msg_summary = None
@@ -411,6 +413,7 @@ 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.
"""
+ self.ensure_finalized()
params = self.args_options_2_params(*args, **options)
self.debug(
'raw: %s(%s)', self.name, ', '.join(self._repr_iter(**params))
@@ -759,7 +762,7 @@ class Command(HasParam):
"""
return self.Backend.xmlclient.forward(self.name, *args, **kw)
- def finalize(self):
+ def on_finalize(self):
"""
Finalize plugin initialization.
@@ -789,7 +792,7 @@ class Command(HasParam):
)
self.output = NameSpace(self._iter_output(), sort=False)
self._create_param_namespace('output_params')
- super(Command, self).finalize()
+ super(Command, self).on_finalize()
def _iter_output(self):
if type(self.has_output) is not tuple:
@@ -1030,19 +1033,20 @@ class Local(Command):
class Object(HasParam):
- backend = None
- methods = None
- properties = None
- params = None
- primary_key = None
- params_minus_pk = None
+ finalize_early = False
+
+ backend = Plugin.finalize_attr('backend')
+ methods = Plugin.finalize_attr('methods')
+ properties = Plugin.finalize_attr('properties')
+ params = Plugin.finalize_attr('params')
+ primary_key = Plugin.finalize_attr('primary_key')
+ params_minus_pk = Plugin.finalize_attr('params_minus_pk')
# Can override in subclasses:
backend_name = None
takes_params = tuple()
- def set_api(self, api):
- super(Object, self).set_api(api)
+ def on_finalize(self):
self.methods = NameSpace(
self.__get_attrs('Method'), sort=False, name_attr='attr_name'
)
@@ -1064,11 +1068,14 @@ class Object(HasParam):
filter(lambda p: not p.primary_key, self.params()), sort=False #pylint: disable=E1102
)
else:
+ self.primary_key = None
self.params_minus_pk = self.params
if 'Backend' in self.api and self.backend_name in self.api.Backend:
self.backend = self.api.Backend[self.backend_name]
+ super(Object, self).on_finalize()
+
def params_minus(self, *names):
"""
Yield all Param whose name is not in ``names``.
@@ -1156,16 +1163,14 @@ class Attribute(Plugin):
only the base class for the `Method` and `Property` classes. Also see
the `Object` class.
"""
- __obj = None
+ finalize_early = False
+
+ __obj = Plugin.finalize_attr('_Attribute__obj')
def __init__(self):
- m = re.match(
- '^([a-z][a-z0-9]+)_([a-z][a-z0-9]+(?:_[a-z][a-z0-9]+)*)$',
- self.__class__.__name__
- )
- assert m
- self.__obj_name = m.group(1)
- self.__attr_name = m.group(2)
+ (self.__obj_name, _, self.__attr_name) = type(self).__name__.partition('_')
+ assert self.__obj_name
+ assert self.__attr_name
super(Attribute, self).__init__()
def __get_obj_name(self):
@@ -1184,9 +1189,9 @@ class Attribute(Plugin):
return self.__obj
obj = property(__get_obj)
- def set_api(self, api):
- self.__obj = api.Object[self.obj_name]
- super(Attribute, self).set_api(api)
+ def on_finalize(self):
+ self.__obj = self.api.Object[self.obj_name]
+ super(Attribute, self).on_finalize()
class Method(Attribute, Command):
diff --git a/ipalib/plugable.py b/ipalib/plugable.py
index b0e4156..ce04815 100644
--- a/ipalib/plugable.py
+++ b/ipalib/plugable.py
@@ -172,10 +172,15 @@ class Plugin(ReadOnly):
Base class for all plugins.
"""
+ finalize_early = True
+
label = None
def __init__(self):
self.__api = None
+ self.__finalize_called = False
+ self.__finalized = False
+ self.__finalize_lock = threading.RLock()
cls = self.__class__
self.name = cls.__name__
self.module = cls.__module__
@@ -210,18 +215,73 @@ class Plugin(ReadOnly):
def __get_api(self):
"""
- Return `API` instance passed to `finalize()`.
+ Return `API` instance passed to `set_api()`.
- If `finalize()` has not yet been called, None is returned.
+ If `set_api()` has not yet been called, None is returned.
"""
return self.__api
api = property(__get_api)
def finalize(self):
"""
+ Finalize the plugin instance.
"""
- if not is_production_mode(self):
- lock(self)
+ with self.__finalize_lock:
+ assert self.__finalized is False
+ if self.__finalize_called:
+ # No recursive calls!
+ return
+ self.__finalize_called = True
+ self.on_finalize()
+ self.__finalized = True
+ if not is_production_mode(self):
+ lock(self)
+
+ def on_finalize(self):
+ """
+ Do custom finalization.
+
+ This method is called from `finalize()`. Subclassess can override this
+ method to implement custom finalization behavior.
+ """
+ pass
+
+ def ensure_finalized(self):
+ """
+ Finalize the plugin instance if it has not yet been finalized.
+ """
+ with self.__finalize_lock:
+ if not self.__finalized:
+ self.finalize()
+
+ class finalize_attr(object):
+ """
+ Create a stub object for attribute that requires finalization.
+
+ When the stub object is accessed, the plugin is finalized and the
+ stub object gets replaced with the actual attribute value.
+ """
+ __slots__ = ('name', 'value')
+
+ def __init__(self, name, value=None):
+ self.name = name
+ self.value = value
+
+ def __get__(self, obj, cls):
+ if obj is None or obj.api is None:
+ return self.value
+ obj.ensure_finalized()
+ try:
+ return getattr(obj, self.name)
+ except RuntimeError:
+ # If the actual attribute value is not set in finalize(),
+ # getattr() calls __get__() again, which leads to infinite
+ # recursion. This can happen only if the plugin is written
+ # badly, so advise the developer about that instead of giving
+ # them a generic "maximum recursion depth exceeded" error.
+ raise AttributeError(
+ "attribute '%s' of plugin '%s' was not set in finalize()" % (self.name, obj.name)
+ )
def set_api(self, api):
"""
@@ -607,11 +667,14 @@ class API(DictProxy):
lock(self)
plugins = {}
+ tofinalize = set()
def plugin_iter(base, subclasses):
for klass in subclasses:
assert issubclass(klass, base)
if klass not in plugins:
plugins[klass] = PluginInstance(klass)
+ if klass.finalize_early or not self.env.plugins_on_demand:
+ tofinalize.add(plugins[klass])
p = plugins[klass]
if not is_production_mode(self):
assert base not in p.bases
@@ -637,8 +700,8 @@ class API(DictProxy):
if not production_mode:
assert p.instance.api is self
- for p in plugins.itervalues():
- p.instance.finalize()
+ for p in tofinalize:
+ p.instance.ensure_finalized()
if not production_mode:
assert islocked(p.instance) is True
object.__setattr__(self, '_API__finalized', True)
diff --git a/ipaserver/rpcserver.py b/ipaserver/rpcserver.py
index 35a1092..d0838a6 100644
--- a/ipaserver/rpcserver.py
+++ b/ipaserver/rpcserver.py
@@ -143,9 +143,9 @@ class session(Executioner):
finally:
destroy_context()
- def finalize(self):
+ def on_finalize(self):
self.url = self.env['mount_ipa']
- super(session, self).finalize()
+ super(session, self).on_finalize()
def route(self, environ, start_response):
key = shift_path_info(environ)
@@ -186,9 +186,9 @@ class WSGIExecutioner(Executioner):
if 'session' in self.api.Backend:
self.api.Backend.session.mount(self, self.key)
- def finalize(self):
+ def on_finalize(self):
self.url = self.env.mount_ipa + self.key
- super(WSGIExecutioner, self).finalize()
+ super(WSGIExecutioner, self).on_finalize()
def wsgi_execute(self, environ):
result = None
@@ -285,13 +285,13 @@ class xmlserver(WSGIExecutioner):
content_type = 'text/xml'
key = 'xml'
- def finalize(self):
+ def on_finalize(self):
self.__system = {
'system.listMethods': self.listMethods,
'system.methodSignature': self.methodSignature,
'system.methodHelp': self.methodHelp,
}
- super(xmlserver, self).finalize()
+ super(xmlserver, self).on_finalize()
def listMethods(self, *params):
return tuple(name.decode('UTF-8') for name in self.Command)
diff --git a/makeapi b/makeapi
index af3b0f9..f68cd62 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 0f6aecb..b717a43 100644
--- a/tests/test_ipalib/test_frontend.py
+++ b/tests/test_ipalib/test_frontend.py
@@ -942,7 +942,8 @@ class test_Object(ClassChecker):
parameters.Str('four', primary_key=True),
)
o = example3()
- e = raises(ValueError, o.set_api, api)
+ o.set_api(api)
+ e = raises(ValueError, o.finalize)
assert str(e) == \
'example3 (Object) has multiple primary keys: one, two, four'
--
1.7.6.4
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
