Dne 15.11.2011 20:10, Rob Crittenden napsal(a):
Jan Cholasta wrote:
Dne 9.11.2011 11:59, Alexander Bokovoy napsal(a):
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.
In my patch, finalize() throws an exception if called more than once,
but ensure_finalized() does nothing for plugins that are already
finalized. So there are both kinds of behavior available.
+ 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.
I always run the test suite ;-)
All the unit tests succeed (they do when run with my patch 54 applied,
which fixes failures of some unrelated unit tests).
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. ;)
on_finalize() shouldn't be called directly (perhaps I should rename it
to _on_finalize to emphasize that...?)
I'll work on the commit message. As usual, it is the hardest part of the
patch for me.
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.
Yep.
Thanks a lot! I think it is great advance over the original approach.
Thanks for the kind words :-)
Honza
Just a couple of questions/clarifications:
1. I think more documentation is needed for on_finalize(). The name
isn't particularly descriptive. If I read it properly you are providing
an alternate way to override finalize(), right?
Yes. I have split finalize() to finalize() (the method to be called to
finalize the plugin) and on_finalize() (the method to be overridden by
plugin developer to implement custom finalization), so that I can guard
the finalizing code from recursively calling itself (which happens when
you try to read an unfinalized finalize_attr attribute from within a
finalize() call).
2. Changing to a partition in Attribute is not equivalent to the regular
expression previously used. Why loosen the requirements?
Well, I didn't think it through. I have reverted the change.
3. I think you should document in-line where you have block calls to
finalize_attr() so that future developers know to add the stub.
OK.
4. What is the purpose of the read-lock in finalize?
I guess it is the usual - to prevent modifying the object after the API
is finalized. The commit that introduced it is
<https://fedorahosted.org/freeipa/changeset/0edb22c9ac70c5acfab51318810f693d59fab955>,
the commit that made it conditional is
<https://fedorahosted.org/freeipa/changeset/359d54e741877f04b0773fb0955041eee7ec0054>.
It generally looks good and provides impressive performance improvements.
rob
Updated patch attached.
Honza
--
Jan Cholasta
>From 0fc2e349ade20f71a8f8f4ee637afd9ea160727d Mon Sep 17 00:00:00 2001
From: Jan Cholasta <jchol...@redhat.com>
Date: Thu, 3 Nov 2011 06:42:17 -0400
Subject: [PATCH] Finalize plugin initialization on demand.
This patch changes the way plugins are initialized. Instead of
finalizing all the plugins at once, plugins are finalized only after
they are accessed (currently applies to Command, Object and
Attribute subclasses, only in CLI by default).
This change provides significant performance boost, as only the
plugins that are actually used are finalized.
ticket 1336
---
ipalib/cli.py | 4 +-
ipalib/config.py | 4 ++
ipalib/constants.py | 1 +
ipalib/frontend.py | 63 ++++++++++++++++----------
ipalib/plugable.py | 87 +++++++++++++++++++++++++++++++++---
ipaserver/rpcserver.py | 12 +++---
makeapi | 1 +
tests/test_ipalib/test_frontend.py | 3 +-
8 files changed, 135 insertions(+), 40 deletions(-)
diff --git a/ipalib/cli.py b/ipalib/cli.py
index 7fe8087..7d79775 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 851de43..3dc30da 100644
--- a/ipalib/frontend.py
+++ b/ipalib/frontend.py
@@ -388,17 +388,20 @@ class Command(HasParam):
ipalib.frontend.my_command()
"""
+ finalize_early = False
+
takes_options = tuple()
takes_args = tuple()
- args = None
- options = None
- params = None
+ # Create stubs for attributes that are set in _on_finalize()
+ 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 +414,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))
@@ -769,7 +773,7 @@ class Command(HasParam):
"""
return self.Backend.xmlclient.forward(self.name, *args, **kw)
- def finalize(self):
+ def _on_finalize(self):
"""
Finalize plugin initialization.
@@ -799,7 +803,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:
@@ -1040,19 +1044,21 @@ class Local(Command):
class Object(HasParam):
- backend = None
- methods = None
- properties = None
- params = None
- primary_key = None
- params_minus_pk = None
+ finalize_early = False
+
+ # Create stubs for attributes that are set in _on_finalize()
+ 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'
)
@@ -1074,11 +1080,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``.
@@ -1166,16 +1175,20 @@ class Attribute(Plugin):
only the base class for the `Method` and `Property` classes. Also see
the `Object` class.
"""
- __obj = None
+ finalize_early = False
+
+ NAME_REGEX = re.compile(
+ '^(?P<obj>[a-z][a-z0-9]+)_(?P<attr>[a-z][a-z0-9]+(?:_[a-z][a-z0-9]+)*)$'
+ )
+
+ # Create stubs for attributes that are set in _on_finalize()
+ __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__
- )
+ m = self.NAME_REGEX.match(type(self).__name__)
assert m
- self.__obj_name = m.group(1)
- self.__attr_name = m.group(2)
+ self.__obj_name = m.group('obj')
+ self.__attr_name = m.group('attr')
super(Attribute, self).__init__()
def __get_obj_name(self):
@@ -1194,9 +1207,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..a76f884 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,85 @@ 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 plugin initialization.
+
+ This method calls `_on_finalize()` and locks the plugin object.
+
+ Subclasses should not override this method. Custom finalization is done
+ in `_on_finalize()`.
"""
- 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()`. Subclasses can override this
+ method in order to add custom finalization.
+ """
+ pass
+
+ def ensure_finalized(self):
+ """
+ Finalize plugin initialization 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 plugin attribute that isn't set until the
+ finalization of the plugin initialization.
+
+ When the stub object is accessed, it calls `ensure_finalized()` to make
+ sure the plugin initialization is finalized. The stub object is expected
+ to be replaced with the actual attribute value during the finalization
+ (preferably in `_on_finalize()`), otherwise an `AttributeError` is
+ raised.
+
+ This is used to implement on-demand finalization of plugin
+ initialization.
+ """
+ __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 _on_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,6 +679,7 @@ class API(DictProxy):
lock(self)
plugins = {}
+ tofinalize = set()
def plugin_iter(base, subclasses):
for klass in subclasses:
assert issubclass(klass, base)
@@ -616,6 +689,8 @@ class API(DictProxy):
if not is_production_mode(self):
assert base not in p.bases
p.bases.append(base)
+ if klass.finalize_early or not self.env.plugins_on_demand:
+ tofinalize.add(p)
yield p.instance
production_mode = is_production_mode(self)
@@ -637,8 +712,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..68d4379 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 755849f..007531a 100755
--- a/makeapi
+++ b/makeapi
@@ -397,6 +397,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
