Jan Cholasta wrote:
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
ACK, pushed to master.
rob
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
