Dne 2.11.2011 17:38, Alexander Bokovoy napsal(a):
(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@vm-114 freeipa-2-1-speedup]# time ipa dnsrecord-find ipa.local>/dev/null
real 0m0.883s
user 0m0.504s
sys 0m0.136s
[root@vm-114 freeipa-2-1-speedup]# time ipa user-find>/dev/null
real 0m0.887s
user 0m0.486s
sys 0m0.141s
[root@vm-114 freeipa-2-1-speedup]# time ipa group-find>/dev/null
real 0m0.933s
user 0m0.446s
sys 0m0.169s
[root@vm-114 freeipa-2-1-speedup]# time ipa help>/dev/null
real 0m0.624s
user 0m0.479s
sys 0m0.133s
[root@vm-114 freeipa-2-1-speedup]# time ipa group>/dev/null
real 0m0.612s
user 0m0.475s
sys 0m0.126s
[root@vm-114 freeipa-2-1-speedup]# time ipa user>/dev/null
real 0m0.617s
user 0m0.472s
sys 0m0.131s
--
/ Alexander Bokovoy
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).
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.)
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'").
Honza
--
Jan Cholasta
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/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)
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
