Cleanup Trac Imports

[cg@…]

Since I'm using Trac very much I have a big problem with it. It just leaks memory, a gc.collect() collects more than 10.000 objects after some requests.

I have just created a patch file that cleans up unused imports, star-imports are also deleted ('cause they clutter the namespace). Maybe that's a startingpoint to cleanup some things and let it leak less memory.

[cg@…]

The patch file that fixes some import-errors

[Christopher Grebs <cg@…>]

Oh, by the way. The patch was created with the latest changeset included ([7455])

I'm still not sure if it really helps to minimize memleak but it cleans it up anyway :D

[Christopher Grebs <cg@…>]

Replying to Christopher Grebs <cg@…>:

Oh, by the way. The patch was created with the latest changeset included ([7455])

I'm still not sure if it really helps to minimize memleak but it cleans it up anyway :D

argh, wrong link. Anyway the latest trunk (not 0.11 stable branch) is the base.

[Matthew Good]

Thanks for looking at the unused imports. Getting rid of some of those helps keep the code clean, but is not going to have a significant impact on memory usage. Imported modules are always referred to by sys.modules, so they will never get garbage collected (this is intentional to make sure modules are only executed the first time you import them).

However, I think most of the "import *"s are intentional. Trac uses __all__ extensively to ensure that "import *" only imports names intentionally exposed by that module and does not pollute the namespace. This is also convenient for commonly used modules like "trac.core" so that you don't have to remember to add each of these as you start to use them.

Can you re-submit a patch that does not include the "import *" expansions?

[Christopher Grebs <cg@…>]

New patch (without * imports replaced)

[Christopher Grebs <cg@…>]

I just added the new patch – this should work… thank's for answer my ticket. Regarding memory-leaking: I learned that python2.4 can't free reserved memory and I think that's the most depressing thingy…

[Remy Blank]

Some cleanup would indeed be needed.

[Christian Boos]

Christopher, any chance you could update the patch to current trunk?

[Christopher Grebs <cg@…>]

Hey,

I think this should be no problem. I think I can find some minutes tomorrow or so. Stay tuned :-)

[Remy Blank]

What would really be useful is a script that collects all imports and finds which symbols don't appear anywhere else in the module. Maybe I'll give it a shot.

[Christian Boos]

​google:pylint or ​google:pychecker?

[Remy Blank]

Replying to cboos:

​google:pylint or ​google:pychecker?

Sure, more efficient, but not as fun :-) I knew about pylint, but haven't used it so far. Good opportunity to find out!

[Christian Boos]

Another one: ​http://divmod.org/trac/wiki/DivmodPyflakes

[Remy Blank]

I have managed to get pylint working with PyDev, and that thing is pretty amazing! Configuration takes a bit of time, but it does catch many "cosmetic" errors (and even less cosmetic ones). Patch coming shortly (and hopefully it won't break all the patches in your queue).

[Christian Boos]

What would you think about an even more drastic clean-up, which would be to import modules only, not the objects defined in those modules.

This would get rid of (most) of the import maintenance trouble, make the import lists much smaller, and clearly show which names are coming from which module. This is the style followed by Mercurial, Django, ​at Google…

[Remy Blank]

Replying to cboos:

What would you think about an even more drastic clean-up, which would be to import modules only, not the objects defined in those modules.

My first reaction was: no, please no! Having to prefix every class reference and every function call with a module name sounds like a pain in the backside, and IMO hurts readability.

But thinking about it, there are probably not that many class references, and function calls are also relatively rare compared to method calls. So it's probably not as bad as I initially thought.

Now, if we want to stay on-schedule for 0.12, I wouldn't do that now. Moreover, this is a purely cosmetic issue, so it will take time for no added value to the user.

So, -1 for 0.12, and a small -0 for next-major-0.1X.

[Remy Blank]

A thorough cleanup (using pylint) has been committed in [8734].

[Remy Blank]

And some additional cleanup on multirepos in [8736].

… my… head… hurts…