[RFC/MERGE] Updates to decorators
John Arbash Meinel
john at arbash-meinel.com
Fri Jan 12 00:57:18 GMT 2007
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
I have 2 updates to our current decorators, one follows on the other
one, but is a little more controversial.
Basically, I've been doing lsprof testing, and I've been getting
frustrated with the 'read_locked()' functions. Because in a big test,
you can have 10 or more @needs_read_lock functions. So when you are
tracking up from the bottom, or down from the top, you frequently hit a
barrier which makes it difficult to tell who is calling what. (10
functions call "read_locked()" and read_locked calls 10 other functions,
so which is being called without going to the source?)
The first pass changes the needs_read_lock and needs_write_lock
decorators so that they use 'exec' to create a function with the a
mostly correct name (plus a _read_locked suffix). In testing, it was
enough to make sure that functions could be uniquely determined. In
theory if we have Branch.get() and Repository.get() they would end up
having the same function signatures.
As a small blurb this is a snippet from the old 'bzr st --lsprof':
2 0 0.2330 0.0337 bzrlib.tree:461(_iter_changes)
+528 0 0.0576 0.0168 +bzrlib.workingtree:1307(_comparison_data)
+482 0 0.1104 0.0092 +bzrlib.decorators:35(read_locked)
+1058 0 0.0122 0.0091 +bzrlib.inventory:947(iter_entries_by_dir)
+482 0 0.0076 0.0054 +bzrlib.revisiontree:84(get_file_sha1)
500 488 0.4910 0.0098 bzrlib.decorators:35(read_locked)
+1 0 0.1445 0.0389 +bzrlib.branch:1104(revision_history)
+488 0 0.0316 0.0103 +bzrlib.workingtree:1846(unlock)
+488 0 0.0318 0.0098 +bzrlib.workingtree:1348(lock_read)
+482 0 0.0393 0.0049 +bzrlib.workingtree:588(get_file_sha1)
+5 0 0.0018 0.0002 +bzrlib.lockable_files:181(get_utf8)
+3 0 0.0017 0.0001 +bzrlib.workingtree:1801(_last_revision)
+1 0 0.1069 0.0001 +bzrlib.workingtree:1447(read_working_in...
+1 0 0.0006 0.0001 +bzrlib.workingtree:1833(conflicts)
+4 0 0.0013 0.0001 +bzrlib.lockable_files:175(get)
+9 0 0.0001 0.0001 +bzrlib.lockable_files:247(unlock)
+9 0 0.0001 0.0001 +bzrlib.lockable_files:231(lock_read)
+1 0 0.2337 0.0000 +bzrlib.tree:429(compare)
+1 0 0.0001 0.0000 +bzrlib.branch:1055(unlock)
+1 0 0.0000 0.0000 +bzrlib.branch:1047(lock_read)
+1 0 0.0002 0.0000 +bzrlib.inter:103(unlock)
+1 0 0.0002 0.0000 +bzrlib.repository:902(is_shared)
+1 0 0.0001 0.0000 +bzrlib.inter:82(lock_read)
+1 0 0.0001 0.0000 +bzrlib.repository:236(lock_read)
+1 0 0.0001 0.0000 +bzrlib.repository:295(unlock)
and after the changes:
2 0 0.2365 0.0329 bzrlib.tree:461(_iter_changes)
+528 0 0.0588 0.0165 +bzrlib.workingtree:1307(_comparison_data)
+482 0 0.1119 0.0097 +<<string>>:1(get_file_sha1_read_locked)
+1058 0 0.0120 0.0090 +bzrlib.inventory:947(iter_entries_by_dir)
+482 0 0.0089 0.0065 +bzrlib.revisiontree:84(get_file_sha1)
482 0 0.1119 0.0097 <<string>>:1(get_file_sha1_read_locked)
+482 0 0.0309 0.0098 +bzrlib.workingtree:1348(lock_read)
+482 0 0.0311 0.0096 +bzrlib.workingtree:1846(unlock)
+482 0 0.0402 0.0051 +bzrlib.workingtree:588(get_file_sha1)
I hope that you can see why the new form is easier for me to read and
follow the trace.
The second patch extends the first one by making the wrapping created
function also include the same parameters as the original function. It
uses 'inspect' to do most of the introspection. What this really changes
is the results when doing help(function). Specifically the difference is:
>>> import bzrlib.branch.Branch
>>> help(bzrlib.branch.Branch.fetch)
Help on method fetch in module bzrlib.decorators:
fetch(self, *args, **kwargs) unbound bzrlib.branch.Branch method
Copy revisions from from_branch into this branch.
:param from_branch: Where to copy from.
:param last_revision: What revision to stop at (None for at the end
of the branch.
:param pb: An optional progress bar to use.
Returns the copied revision count and the failed revisions in a tuple:
(copied, failures).
New Version:
Help on method fetch:
fetch(self, from_branch, last_revision=None, pb=None) unbound
bzrlib.branch.Branch method
Copy revisions from from_branch into this branch.
:param from_branch: Where to copy from.
:param last_revision: What revision to stop at (None for at the end
of the branch.
:param pb: An optional progress bar to use.
Returns the copied revision count and the failed revisions in a tuple:
(copied, failures).
So you can see how the new version makes it much more obvious what
parameters a function can take.
This also shows up in the output of 'epydoc'. So for public
documentation it would be nice to use the newest version.
I'm not 100% happy with having to recompile everything on startup, as it
does have some overhead. It does help documentation and lsprof results,
though. So it might be worth having around somehow.
I did some performance timing, and the results are:
bzr.dev patch1 patch2
bzr rocks 162ms 168ms 176ms
--no-plugins 95ms 96ms 96ms
bzr status 443ms 462ms 476ms
--no-plugins 415ms 439ms 447ms
So you can see that this is really the specific overhead of compiling
the new wrapper functions, since 'bzr rocks --no-plugins' has very
little overhead, since it doesn't load as many modules and thus doesn't
have to compile the functions, but otherwise there is a fairly static
overhead of around 20ms for patch1, and 30ms for patch2.
(I realize BB is going to get confused because I'm submitting 2 separate
patches in one email, but I don't want to re-write everything).
Basically, the performance impact is large enough that I'm not sure we
want to do it. But the improvements to documentation and lsprof seem
like it would be worth keeping around *somehow*.
One other possibility, is that we could change the definition of
'needs_read_lock' based on whether --lsprof is supplied (or some other
flag in the case of generating documentation). We could do something
like what we do right now in 'bzr' with 'bzrlib.inspect_for_copy'. And
have the 'bzr' front-end switch the definitions to the faster form while
it is loading bzrlib.
This would mean that users importing bzrlib directly would load a little
slower, but with better documentation (which is just fine from the
interactive interpreter, or when generating epydocs), but 'bzr' the
command line program would not have to pay the penalty.
Comments?
John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.3 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
iD8DBQFFptztJdeBCYSNAAMRAme8AJ9dY2nlva5pw6aIb/Ewe8qYbqdhCACg0Uig
yrVQQBfisFX7FT5NV4Zj22g=
=li9v
-----END PGP SIGNATURE-----
-------------- next part --------------
A non-text attachment was scrubbed...
Name: decorator_names.patch
Type: text/x-patch
Size: 2192 bytes
Desc: not available
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20070111/d131cb1e/attachment.bin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: decorator_parameters.patch
Type: text/x-patch
Size: 4504 bytes
Desc: not available
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20070111/d131cb1e/attachment-0001.bin
More information about the bazaar
mailing list