[merge] Registry

[Richard Wilbur]

On Wed, 2006-10-11 at 10:03 +1000, John Arbash Meinel wrote:
> 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
> =:->
John,

Thanks for filling in the holes in my context so graciously.  I look
forward to your repaired patch, including the tests you mentioned.

Richard
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 191 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20061011/1add5051/attachment-0001.pgp