This looks like a decent step in the right direction. One way it could be improved further, I think, is to separate different profilers into different objects and give them different implementations of the runWhatever method, instead of doing flag checking and having a bunch of different methods on one class.

I don't think it would go wrong to clean up the twistd command line, either. twistd --profiler=hotshot, etc, would be a lot nicer than what we have now.

Clearly this isn't a thorough review. I wanted to get your thoughts before proceeding in any direction.

This looks like a decent step in the right direction. One way it could be improved further, I think, is to separate different profilers into different objects and give them different implementations of the runWhatever method, instead of doing flag checking and having a bunch of different methods on one class.

I don't think it would go wrong to clean up the twistd command line, either. twistd --profiler=hotshot, etc, would be a lot nicer than what we have now.

I think both points are linked. If we allow ourselves to change the command line options, we can have something smarter than flag checking. But if we're trying to have a backward compatible mechanism, I'm not sure it will ease things :). But having separate objects and options would definitely be cleaner.

trial has profiler command line options too. They should probably be the same as twistd's.

Some of the new tests fail if profile isn't importable. They should skip, like the hotshot tests skip, I guess. Here's the error output I get from trial twisted.test.test_twistd:

===============================================================================
[ERROR]: twisted.test.test_twistd.AppProfilingTestCase.test_hotshot
Traceback (most recent call last):
File "/home/exarkun/Projects/Twisted/trunk/twisted/test/test_twistd.py", line 327, in test_hotshot
profiler.run(reactor)
File "/home/exarkun/Projects/Twisted/trunk/twisted/application/app.py", line 91, in run
self._reportImportError("hotshot", e)
File "/home/exarkun/Projects/Twisted/trunk/twisted/application/app.py", line 49, in _reportImportError
raise SystemExit(s)
exceptions.SystemExit: Failed to import module hotshot: No module named profile; please install the python-profiler package
This is most likely caused by your operating system not including
the module to it being non-free. Either do not use the option
--profile, or install the module; your operating system vendor
may provide it in a separate package.
===============================================================================
[ERROR]: twisted.test.test_twistd.AppProfilingTestCase.test_hotshot
Traceback (most recent call last):
Failure: exceptions.ImportError: No module named profile; please install the python-profiler package
===============================================================================
[ERROR]: twisted.test.test_twistd.AppProfilingTestCase.test_hotshotSaveStats
Traceback (most recent call last):
File "/home/exarkun/Projects/Twisted/trunk/twisted/test/test_twistd.py", line 351, in test_hotshotSaveStats
profiler.run(reactor)
File "/home/exarkun/Projects/Twisted/trunk/twisted/application/app.py", line 91, in run
self._reportImportError("hotshot", e)
File "/home/exarkun/Projects/Twisted/trunk/twisted/application/app.py", line 49, in _reportImportError
raise SystemExit(s)
exceptions.SystemExit: Failed to import module hotshot: No module named profile; please install the python-profiler package
This is most likely caused by your operating system not including
the module to it being non-free. Either do not use the option
--profile, or install the module; your operating system vendor
may provide it in a separate package.
===============================================================================
[ERROR]: twisted.test.test_twistd.AppProfilingTestCase.test_hotshotSaveStats
Traceback (most recent call last):
Failure: exceptions.ImportError: No module named profile; please install the python-profiler package
-------------------------------------------------------------------------------

In the twistd man page (thanks for updating it!), I guess the copyright statement should match the one in most of the source files. I think the man pages were just missed when the source files were updated (r11450).

_reportImportError says the module to it, should say the module due to it. Thanks for updating the log.msg/log.deferr in that method, too. I think _why shouldn't be passed by keyword though (it's private, after all). It should be safe to just pass it as the second positional argument (if it isn't, we should fix it so it is).

ProfilerRunner.run should probably use try/finally for sys.stdout manipulation. An exception raised by print_stats will mess stuff up seriously as it is, I think. Not sure how to test this, not sure when print_stats raises an exception.

Same for HotshotRunner.run. Also, hasattr is evil. getattr with a default is better, or you could just try to get the stream and catch the AttributeError.

Alright, I think I've fixed all the reported problems. The only thing I left over is trial modifications: for now it has its one thing, so we should open another ticket to use the same funtions (hopefully, it'll be easier once this in).

runWithProfiler and runWithHotshot deprecations would probably be better if they pointed at ProfileRunner and HotshotRunner, respectively. AppProfiler seems like it is something that can probably go away after we drop support for all the deprecated ways to invoke this functionality. Also, aren't the assertions about the file in the deprecation warning wrong? It should be the file of the caller of runWith* which the deprecation warning points to, not app.py.

If a profiler is unimportable, the ImportError gets logged four times (it does in trunk too, I think). Maybe it would be appropriate to improve this just a bit in this branch. Deleting the traceback.print_exc call in _reportImportError gets rid of one of them pretty easily.

If I pass an invalid name to --profiler, I get an UnboundLocalError.

If I pass --savestats and don't specify a profiler, the profiler switches to hotshot, which is a bit surprising.

Ah, the incompatibility I thought I saw was due to the man page being corrected to document that --profile takes an argument. No actual behavior changed though, great.

In _BasicProfiler._reportImportError docstring, distribution should be distributions or Linux distributions or packagers.

In ProfileRunner.run docstring, under standard should be under the standard.

In HotshotRunner.run docstring, under hotshot should be under the hotshot.

In AppProfiler docstring, I'd say something like "Class which selects a specific profile runner based on configuration options." to make it explicit and clear what is being "managed".

ApplicationRunner should document its profiler instance attribute.

The comma in the docstring of AppProfilingTestCase.test_profile is extraneous.

AppProfilingTestCase.test_withoutProfile lets a lot of code run in between putting None into sys.modules and setting up the try/finally block.

The comma in the docstring of AppProfilingTestCase.test_hotshot is extraneous.

The epytext (or lack thereof) for the nothotshot option in the docstring of AppProfilingTestCase.test_nothotshotDeprecation is inconsistent with the way options are marked up in other docstrings. Also, produce in that docstring should be produces. The sentence would also read a bit more easily if "switching" and "on" were next to each other instead of spread out so much.

In the docstring for AppProfilingTestCase.test_hotshotPrintStatsError, "during the print of the stats" is awkward. I'd say "while printing the stats".

I changed the comment near the exception handling code a bit. Don't read it too closely if you don't want to learn more about why this happened. :P

Since handling the exception from importing hotshot.stats was the only change, I think this can probably be re-merged now. One thing I noticed though is that a couple of the builders have a large number of conch failures for this branch. Was it created when some bad conch code was merged into trunk?