[Freeipa-devel] [PATCH] 1 Do lazy initializiation ipalib
Alexander Bokovoy
abokovoy at redhat.com
Wed Nov 2 16:38:49 UTC 2011
(actual patch attached!)
On Wed, 02 Nov 2011, Alexander Bokovoy wrote:
> On Wed, 02 Nov 2011, Jan Cholasta wrote:
> > >Callable instances are a consequence of the above --
> > >Command.__call__() does use properties that are changed due to
> > >finalize() being run. In fact, there are so many places in FreeIPA
> > >where we simply expect that foo.args/params/output_params/output is
> > >both iterable and callable instance that it is not even funny.
> > >
> > >Now, the process established by the proposed patch is actually
> > >dependent only on the fact that bound instance has finalize() method.
> > >This is quite generic and can be used for all other types of objects.
> > >
> >
> > The real problem is in the API class so it should be fixed there,
> > not worked around in Plugin subclasses.
> Please explain why you do think real problem is in API class.
> Finalize() is needed purely to being able to split plugins' property
> initialization into stages before loading plugins and after due to the
> fact that each and every plugin could contribute commands and
> additions to the same objects. While these stages are separate,
> plugins can access properties and commands they want and it is duty of
> a plugin to get its property working right. We have finalize() method
> exactly for this purpose.
>
> API class' attempt to call finalize() on each plugin's instance at
> once is a poor replacement to detecting access to
> not-yet-initialized properties. We can implement that access like you
> have proposed with __getattribute__ but it has additional cost for all
> other properties that don't need post-initialization (finalization)
> and, what's more important, they incur these costs irrespective
> whether initialization has happened or not. That's bad and my patch
> shows it is actually noticeable as the performance difference, for
> example, of 'ipa dnsrecord-find example.com' between the two patches
> is about 2x in favor of my patch (1.624s vs 0.912s).
>
> Regarding other objects, here is the list of places where we define
> specific finalizers:
> $ git grep 'def finalize'
> ipalib/cli.py: def finalize(self):
> ipalib/frontend.py: def finalize(self):
> ipalib/plugable.py: def finalize(self):
> ipalib/plugable.py: def finalize(self):
> ipaserver/rpcserver.py: def finalize(self):
> ipaserver/rpcserver.py: def finalize(self):
> ipaserver/rpcserver.py: def finalize(self):
> tests/test_ipalib/test_cli.py: def finalize(self):
> tests/test_ipalib/test_config.py: def finalize_core(self, ctx, **defaults):
> tests/util.py: def finalize(self, *plugins, **kw):
>
> As you can see, there NO plugins that define their own finalizers,
> thus, all of them rely on API.finalize(), help.finalize(), and
> Plugin.finalize()/Command.finalize(). ipaserver/rpcserver.py
> finalizers are fine as well, I have checked them.
>
> Updated patch is attached. It addresses all comments from Martin.
>
> [root at vm-114 freeipa-2-1-speedup]# time ipa dnsrecord-find ipa.local >/dev/null
>
> real 0m0.883s
> user 0m0.504s
> sys 0m0.136s
> [root at vm-114 freeipa-2-1-speedup]# time ipa user-find >/dev/null
>
> real 0m0.887s
> user 0m0.486s
> sys 0m0.141s
> [root at vm-114 freeipa-2-1-speedup]# time ipa group-find >/dev/null
>
> real 0m0.933s
> user 0m0.446s
> sys 0m0.169s
> [root at vm-114 freeipa-2-1-speedup]# time ipa help >/dev/null
>
> real 0m0.624s
> user 0m0.479s
> sys 0m0.133s
> [root at vm-114 freeipa-2-1-speedup]# time ipa group >/dev/null
>
> real 0m0.612s
> user 0m0.475s
> sys 0m0.126s
> [root at vm-114 freeipa-2-1-speedup]# time ipa user >/dev/null
>
> real 0m0.617s
> user 0m0.472s
> sys 0m0.131s
>
> --
> / Alexander Bokovoy
--
/ Alexander Bokovoy
-------------- next part --------------
>From be4fc63a16e9fdd5fa1c58aa5a5267b8e06e44ce Mon Sep 17 00:00:00 2001
From: Alexander Bokovoy <abokovoy at 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/frontend.py | 50 ++++++++++++++++++++++++++++++++++++
ipalib/plugable.py | 12 ++++++---
tests/test_ipalib/test_frontend.py | 7 +++--
3 files changed, 62 insertions(+), 7 deletions(-)
diff --git a/ipalib/frontend.py b/ipalib/frontend.py
index 61e7f493f8a8e30a1a189d06cd6a69893319deaf..22d77372b8cb58bb2e922b3fb158ee1025b0d242 100644
--- a/ipalib/frontend.py
+++ b/ipalib/frontend.py
@@ -64,6 +64,44 @@ 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()
+
class HasParam(Plugin):
"""
@@ -404,6 +442,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 +457,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))
diff --git a/ipalib/plugable.py b/ipalib/plugable.py
index b0e415656e0428eb164c35a2862fcfbf50883381..e0e1eacbb14a0d8278832bcbda358264bf233100 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,7 @@ class Plugin(ReadOnly):
def finalize(self):
"""
"""
+ self.__finalized = True
if not is_production_mode(self):
lock(self)
@@ -637,10 +639,12 @@ 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
+ if self.env.context != 'cli':
+ for p in plugins.itervalues():
+ 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/tests/test_ipalib/test_frontend.py b/tests/test_ipalib/test_frontend.py
index 0f6aecb3d137872fd89e89f9db64e1d9aab7d8c1..261823c4ff90b05f4c24fa0bfc71fb3e139b6a4b 100644
--- a/tests/test_ipalib/test_frontend.py
+++ b/tests/test_ipalib/test_frontend.py
@@ -30,6 +30,7 @@ from ipalib import frontend, backend, plugable, errors, parameters, config
from ipalib import output
from ipalib.parameters import Str
from ipapython.version import API_VERSION
+import types
def test_RULE_FLAG():
assert frontend.RULE_FLAG == 'validation_rule'
@@ -252,7 +253,7 @@ class test_Command(ClassChecker):
"""
Test the ``ipalib.frontend.Command.args`` instance attribute.
"""
- assert self.cls().args is None
+ assert isinstance(self.cls().args, (types.NoneType, frontend._MirrorTrap))
o = self.cls()
o.finalize()
assert type(o.args) is plugable.NameSpace
@@ -301,7 +302,7 @@ class test_Command(ClassChecker):
"""
Test the ``ipalib.frontend.Command.options`` instance attribute.
"""
- assert self.cls().options is None
+ assert isinstance(self.cls().options, (types.NoneType, frontend._MirrorTrap))
o = self.cls()
o.finalize()
assert type(o.options) is plugable.NameSpace
@@ -323,7 +324,7 @@ class test_Command(ClassChecker):
Test the ``ipalib.frontend.Command.output`` instance attribute.
"""
inst = self.cls()
- assert inst.output is None
+ assert isinstance(inst.output, (types.NoneType, frontend._MirrorTrap))
inst.finalize()
assert type(inst.output) is plugable.NameSpace
assert list(inst.output) == ['result']
--
1.7.7
More information about the Freeipa-devel
mailing list