[merge] Registry

Wed Oct 11 01:03:43 BST 2006

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Richard Wilbur wrote:
> On Wed, 2006-09-27 at 17:47 -0500, John Arbash Meinel wrote:
>> Robert Collins wrote:
>>> On Thu, 2006-09-21 at 16:28 -0500, John Arbash Meinel wrote:
>>>
>>> John and I talked on IRC and we are mostly in agreement now... sometime
>>> soon should see a new patch :)
>>>
>>> -Rob
>> Attached is an updated Registry, which defers to helper classes for
>> handling whether to lazy import or not.
>>
>> And it lessens how much Registry tries to look like a dictionary.
>>
>> John
>> =:->
>>
> 
> John,
> 
> I'd say it looks pretty good.  I do have a few questions and comments:
> 
> First, my lack of context is showing:
>   i.  Is this replacing some other concept of "Registry"?

We already have several registries in bzr. We have the
{WorkingTree,Branch,Repository}Format registries, the Transport
registries, plugin command registries, etc. They are all more ad-hoc,
where we have a dictionary or list that gets populated as you add more
items.

This is partially an attempt to:
1) Unify some of the interfaces
2) Allow some of them to support lazy registration, so we can break more
functionality out into separate files that only get loaded when needed.

>   ii.  I'm wondering how "it lessens how much Registry tries to look
> like a dictionary"?

You missed the first versions of this patch, when I implemented a lot
more of the dict interface. __iter__, __setitem__, etc. The changes were
in response to Robert's comments.

> +
> +    def _do_import(self):
> +        obj = __import__(self._module_name, globals(), locals(),
> +                         [self._member_name])
> +        if member_name:
> +            obj = getattr(obj, member_name)
> +        self._obj = obj
> +        self._imported = True
> 
> ^-- Did you intend 'member_name' as a keyword argument as in
> '_do_import(self, member_name=None):'?  Otherwise it is out of scope as
> far as I can tell.  The only other reference to that name is the final
> parameter of __init__() which makes it out of scope here.

No, it was supposed to be 'self._member_name'. I refactored it from
other code, and missed that. It would seem I also missed some testing,
since that should have been caught by the tests.

> 
> It looks like you were thinking of being able to satisfy a lazy import
> of the form 'from <module_name> import <member_name>', although it
> doesn't seem to be supported by the rest of the framework, yet.
> 

No, it is already supported. If you pass:

register_lazy(key, module, member) <= if 'member' is not None, then it
will be returned by getattr().

> [...]
> 
> === modified file 'NEWS'
> --- NEWS        2006-09-27 20:44:31 +0000
> +++ NEWS        2006-09-27 22:47:02 +0000
> @@ -3,6 +3,12 @@
>    IMPROVEMENTS:
>      * ``bzr help commands`` output is now shorter (Aaron Bentley)
>  
> +    * New Registry class to provide name-to-object registry-like
> support,
> +      for example for schemes where plugins can register new classes to
> +      do certain tasks (e.g. log formatters). Also provides lazy
> registration
> +      to allow modules to be oladed on request. (John Arbash Meinel,
> Adeodato
> +      Simó)
> +
>    INTERNALS:
> 
> ^--Spelling: 'oladed' --> 'loaded'

Thanks.

> 
> [...]
> 
> My guess is that this code is intended to help with scalability issues.
> Speeds start up in the presence of a large number of plug-ins by
> registering their existence and only loading/importing the actual code
> when deemed necessary.  Preserves access to the help text for all
> modules on more immediate recall.
> 
> In light of this, I appreciate your usage of __slots__ to minimize
> overhead.  I also like the use of callbacks for validation of
> default_key assignment.
> 
> Do we have any benchmarks that could show the benefits of strict versus
> lazy import?  They would most likely make a strong case for using lazy
> imports--especially as such things as plug-ins multiply.
> 
> Richard
> 

Yeah, I just added a bunch of benchmarks for my other lazy_import stuff.
You can see it when running 'bzr selftest --benchmark bench_startup'.
This is only in the last few versions of bzr.dev, but it is in
preparation for my 2 lazy_* branches (lazy_regex just landed, and
lazy_import is on its way). And further, Registy should also be able to
help some aspects, though not at first.

As an example of how the new code should help, this is bzr.dev without
my lazy changes (these are run on my Mac laptop, so are much slower than
numbers I typically quote):

...bench_startup.StartupBenchmark.test___version   OK  1368ms/ 1623ms
...ks.bench_startup.StartupBenchmark.test_branch   OK  1901ms/ 2375ms
...ks.bench_startup.StartupBenchmark.test_commit   OK  1815ms/ 2070ms
...arks.bench_startup.StartupBenchmark.test_diff   OK  1169ms/ 1578ms
...arks.bench_startup.StartupBenchmark.test_help   OK  1397ms/ 1411ms
...h_startup.StartupBenchmark.test_help_commands   OK  1356ms/ 1369ms
...marks.bench_startup.StartupBenchmark.test_log   OK  1102ms/ 1348ms
...s.bench_startup.StartupBenchmark.test_missing   OK  1043ms/ 1540ms
...arks.bench_startup.StartupBenchmark.test_pull   OK  1179ms/ 1590ms
...rks.bench_startup.StartupBenchmark.test_rocks   OK  1188ms/ 1200ms
...ks.bench_startup.StartupBenchmark.test_status   OK  1111ms/ 1402ms

And then in a test branch where I have both lazy_* branches merged:
...bench_startup.StartupBenchmark.test___version   OK   860ms/ 1044ms
...ks.bench_startup.StartupBenchmark.test_branch   OK  1187ms/ 1730ms
...ks.bench_startup.StartupBenchmark.test_commit   OK  1127ms/ 1300ms
...arks.bench_startup.StartupBenchmark.test_diff   OK   925ms/ 1155ms
...arks.bench_startup.StartupBenchmark.test_help   OK   423ms/  434ms
...h_startup.StartupBenchmark.test_help_commands   OK   564ms/  576ms
...marks.bench_startup.StartupBenchmark.test_log   OK   852ms/ 1300ms
...s.bench_startup.StartupBenchmark.test_missing   OK   855ms/ 1379ms
...arks.bench_startup.StartupBenchmark.test_pull   OK  1041ms/ 1510ms
...rks.bench_startup.StartupBenchmark.test_rocks   OK   353ms/  364ms
...ks.bench_startup.StartupBenchmark.test_status   OK   938ms/ 1185ms

I've focused on 'bzr rocks' for the moment, because that is the
quintessential *do nothing* command, that should be super cheap to
startup. As you can see, the startup time dropped from 1.2s down to
0.35s. With benefits across the board.

The reason I want to get Registry in, is because I want to start trying
to refactor the Commands into a registry, which lazily imports the
commands from other files. That way you don't have to load all 40+
commands when you just want 1.

And similar effects for the different branch/repository/working tree
formats. It should be possible to only load the formats in use, rather
than always loading all formats.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.5 (Darwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFFLDTfJdeBCYSNAAMRAhoLAKDKo2mDovBjc5RrJ3Mse2alIns0igCgmV1y
tFbYBPlfMBviz4mFaxwMg+0=
=XxP1
-----END PGP SIGNATURE-----