twistd profiling options needs to be tested

[therve]

This is first step to accomplish #2571. The goal is to refactor the free functions in app.py into one object, and to deprecated the old functions.

[therve]

(In [21925]) Branching to 'twistd-profiler-tests-2921'

[therve]

(In [21926])

• Refactor the functions into one AppProfiler object
• Add tests for this object
• Deprecate old free functions
• Spot a bug in hotshot with Python >= 2.5, and correct it

Refs #2921

[therve]

(In [21927]) Add tests for deprecation.

Refs #2921

[therve]

This is ready to review. The nice thing is that the branch corrects a bug with hotshot and Python 2.5. I think this is a move in the good direction.

[exarkun]

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.

[therve]

Replying to exarkun:

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.

[therve]

(In [22267]) Break the profiling into different options, refactor tests.

Refs #2921

[therve]

(In [22269]) Update man pages and copyrights.

Refs #2921

[therve]

Alright, back to review.

[exarkun]

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.

runReactorWithLogging is missing a docstring.

[therve]

(In [22286]) Process review comments.

Refs #2921

[therve]

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).

Back to review.

[exarkun]

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.

[therve]

Erm, sorry for the crappy job, I probably hit the review button a bit too quick. So thanks for the review :).

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

This is the current behavior on trunk (I think). It's also documented as the default value of the profiler option. Do you think it should change?

[exarkun]

It's surprising, I think. It's also undocumented, and there are incompatible changes to the command line already in the branch, so maybe another such change is a good idea.

I think that really everyone just wants to use cProfile/lsprof and profile/hotshot are silly historical accidents, so it may not really matter much either way.

[exarkun]

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".

The rest looks great. Merge when the above is taken care of.

[therve]

(In [22449]) Process review comments.

Refs #2921

[therve]

(In [22450]) Merge twistd-profiler-tests-2921

Author: therve Reviewer: exarkun Fixes #2921

Refactor twistd profiling options to be more easily testable, and add tests for them. This refactor leads to a bug correction of hotshot under Python 2.5 and to some cleanups.

[therve]

(In [22451]) Revert r22450: failing tests of debian system.

Refs #2921

[therve]

Debian slaves fail with this error:

  File "/srv/bb-slave/Run/slave/full2.4/Twisted/twisted/test/test_twistd.py", line 21, in ?
    import hotshot.stats
  File "/usr/lib/python2.4/hotshot/stats.py", line 11, in ?
    sys.exit(1)
exceptions.SystemExit: 1

[therve]

(In [22453]) Try to fix the problem.

Refs #2921

[therve]

I don't want to know what happened, that would make me sick.

[exarkun]

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?

[therve]

(In [22499]) Branching to 'twistd-profiler-tests-2921-2'

[therve]

(In [22502]) Merge twistd-profiler-tests-2921

Author: therve Reviewer: exarkun Fixes #2921

Refactor twistd profiling options to be more easily testable, and add tests for them. This refactor leads to a bug correction of hotshot under Python 2.5 and to some cleanups.

This remerge is due to some insanity of the profiler being non-free under debian.