Code review comment for lp:~gagern/bzr/bug396819-lazy_import-threadsafe

Am 23.11.11 15:19, schrieb Martin Packman:
> Sorry we've not managed to get this reviewed MvG. Your analysis here and in the bug about the previous change seem correct to me, but I'm unconvinced about making further changes to lazy_import in this regard.

Currently, lazy_import is not merely ill designed, but actually breaking
stuff. So I believe that this proposal still should receive a proper
review, and the fix be committed. Unless there actually is code ready to
replace it, fixing it is better than not fixing it.

> I can't offer useful suggestions for improving the branch, only complain about the state of the pitch:
> * There's no way to disable lazy imports for things like loggerhead that care about threading not startup time.

Would be easy, but a different issue, to be implemented in a different
branch, preferrably on top of this one.

> * Telling whether a given lazy import is actually doing anything useful is far too hard.
> * It's not uncommon to see an object get imported then unconditionally used further down in the module.
> * Many parts of bzrlib are lazily imported somewhere, but end up always being required anyway.
> * The import tariff tests are nice, but very heavy compared to say, a lint style check.

tariff tests can deal with only a very limited number of scenarios to
check, but will by design also catch obscure paths leading to some
import. Lint-style checks could process a lot more code paths, detecting
lots of the stuff you describe above, but would be likely to miss some
more obscure constructs. So I'd consider them complementary.

> What I'd like to see would be a replacement with fewer features bzrlib could move to instead. I'm imagining it would:
>
> * Use the import statement directly.

I know that strange things are possible, but how would this work?
"from bzrlib.lazyly import foobar", with "lazily" not being a module but
instead an object working as a proxy factory? I guess that should be
possible, although I have some concerns about nested modules.

> * Only works on module objects.

That would surely make things a lot easier and cleaner.

> * Use a proxy that remains in place but gains attributes.

Not sure about performance here. I can imagine why a self-replacing
proxy would at least feel faster. It should be possible to do some
benchmarks on this.