← Back to team overview

launchpad-reviewers team mailing list archive

[Merge] lp:~gary/launchpad/bug821531 into lp:launchpad

 

Gary Poster has proposed merging lp:~gary/launchpad/bug821531 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #821531 in Launchpad itself: "++profile++ does not allow saving standard Python profiler format"
  https://bugs.launchpad.net/launchpad/+bug/821531

For more details, see:
https://code.launchpad.net/~gary/launchpad/bug821531/+merge/70738

This change adds the ability to get a stdlib-friendly profile output to ++profile++.  This lets you dig in farther than KCacheGrind does, which has a maximum of the top 499 calls displayed for a given sorting.

Lint is happy except for being confused with the start of profile.pt.

Thanks
-- 
https://code.launchpad.net/~gary/launchpad/bug821531/+merge/70738
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~gary/launchpad/bug821531 into lp:launchpad.
=== modified file 'lib/lp/services/profile/profile.pt'
--- lib/lp/services/profile/profile.pt	2011-08-05 16:24:32 +0000
+++ lib/lp/services/profile/profile.pt	2011-08-08 13:10:47 +0000
@@ -7,6 +7,7 @@
      tal:define="actions options/actions;
                  help actions/help|nothing;
                  log actions/log|nothing;
+                 stdlib actions/stdlib|nothing;
                  show actions/show|nothing;
                  always_log options/always_log;
                  dump_path options/dump_path;
@@ -47,6 +48,12 @@
     <code>++profile++log,show</code> to see both the log information and
     immediate results).  Profiles are logged to
     <tal:block replace="dump_path" />.</p>
+    <p>KCacheGrind is great for visualizing a profile, but it only allows
+    analyzing a limited depth of calls.  If you want a greater profile depth,
+    or if for any other reason you want to use the standard Python library's
+    pstats analysis instead, you can. To do this, include "stdlib" as part of
+    your ++profile++ request, as in <code>++profile++log,stdlib</code> or
+    <code>++profile++log,show,stdlib</code>.</p>
   </tal:block>
   <tal:block condition="log">
     <p tal:condition="always_log"><strong>You have configured every
@@ -54,11 +61,17 @@
     <code>profile_all_requests: True</code> in the
     <code>[profiling]</code> section of your launchpad-lazr.conf.</p>
     <p>Profile was logged to <tal:block replace="dump_path" />.</p>
-    <p>You should be able to use this simply by entering
-    <code>kcachegrind <tal:block replace="dump_path" /></code> in the
-    console for a great visualization of the profile.  The <a
+    <p tal:condition="not:stdlib">You should be able to use this
+    simply by entering <code>kcachegrind <tal:block replace="dump_path"
+    /></code> in the console for a great visualization of the profile.  The <a
     href="https://dev.launchpad.net/Debugging#Profiling%%20page%%20requests";
     >dev wiki</a> may have more information on how to use this.</p>
+    <p tal:condition="stdlib">You should be able to use this
+    simply by entering <code>python -m pstats <tal:block replace="dump_path"
+    /></code> in the console to use the normal Python profile statistics
+    interface.  Type &quot;help&quot; at the prompt, or see the <a
+    href="http://docs.python.org/library/profile.html#instant-user-s-manual";
+    >Python documentation</a>.</p>
   </tal:block>
   <h2>Profile quick view</h2>
   <tal:block condition="not:show">

=== modified file 'lib/lp/services/profile/profile.py'
--- lib/lp/services/profile/profile.py	2011-08-05 16:24:32 +0000
+++ lib/lp/services/profile/profile.py	2011-08-08 13:10:47 +0000
@@ -7,11 +7,14 @@
 
 __metaclass__ = type
 
+from cProfile import Profile
 from datetime import datetime
 import os
+import pstats
 import threading
 import StringIO
 
+from bzrlib import errors
 from bzrlib.lsprof import BzrProfiler
 from zope.pagetemplate.pagetemplatefile import PageTemplateFile
 from zope.app.publication.interfaces import IEndRequestEvent
@@ -40,6 +43,85 @@
     """Fake exception used to log OOPS information when profiling pages."""
 
 
+class StdLibProfiler(BzrProfiler):
+    """This provides a wrapper around the standard library's profiler.
+
+    It makes the BzrProfiler and the StdLibProfiler follow a similar API.
+    It also makes them both honor the BzrProfiler's thread lock.
+    """
+
+    def start(self):
+        """Start profiling.
+
+        This will record all calls made until stop() is called.
+
+        Unlike the BzrProfiler, we do not try to get profiles of sub-threads.
+        """
+        self.p = Profile()
+        permitted = self.__class__.profiler_lock.acquire(
+            self.__class__.profiler_block)
+        if not permitted:
+            raise errors.InternalBzrError(msg="Already profiling something")
+        try:
+            self.p.enable(subcalls=True)
+        except:
+            self.__class__.profiler_lock.release()
+            raise
+
+    def stop(self):
+        """Stop profiling.
+
+        This unhooks from threading and cleans up the profiler, returning
+        the gathered Stats object.
+
+        :return: A bzrlib.lsprof.Stats object.
+        """
+        try:
+            self.p.disable()
+            p = self.p
+            self.p = None
+            return StdLibStats(p)
+        finally:
+            self.__class__.profiler_lock.release()
+
+
+class StdLibStats:
+    """This emulates enough of the Bzr stats class to make it interoperable
+    for our purposes."""
+
+    _stats = None
+
+    def __init__(self, profiler):
+        self.p = profiler
+
+    @property
+    def stats(self):
+        if self._stats is None:
+            self._stats = pstats.Stats(self.p).strip_dirs()
+        return self._stats
+
+    def save(self, filename):
+        self.p.dump_stats(filename)
+
+    def sort(self, name):
+        mapping = {
+            'inlinetime': 'time',
+            'totaltime': 'cumulative',
+            'callcount': 'calls'
+            }
+        self.stats.sort_stats(mapping[name])
+
+    def pprint(self, file):
+        stats = self.stats
+        stream = stats.stream
+        stats.stream = file
+        try:
+            stats.print_stats()
+        finally:
+            stats.stream = stream
+
+
+# Profilers may only run one at a time, but block and serialize.
 _profilers = threading.local()
 
 
@@ -90,12 +172,15 @@
     _profilers.profiler = None
     _profilers.profiling = True
     if actions:
-        if actions.difference(('help', )):
+        if actions.difference(('help', 'stdlib')):
             # If this assertion has reason to fail, we'll need to add code
             # to try and stop the profiler before we delete it, in case it is
             # still running.
             assert getattr(_profilers, 'profiler', None) is None
-            _profilers.profiler = BzrProfiler()
+            if 'stdlib' in actions:
+                _profilers.profiler = StdLibProfiler()
+            else:
+                _profilers.profiler = BzrProfiler()
             _profilers.profiler.start()
     if config.profiling.memory_profile_log:
         _profilers.memory_profile_start = (memory(), resident())
@@ -138,7 +223,7 @@
     dump_path = config.profiling.profile_dir
     if _profilers.profiler is not None:
         prof_stats = _profilers.profiler.stop()
-        # Free some memory.
+        # Free some memory (at least for the BzrProfiler).
         del _profilers.profiler
         if oopsid is None:
             # Log an OOPS to get a log of the SQL queries, and other
@@ -151,11 +236,18 @@
         else:
             oops = request.oops
         if 'log' in actions:
-            filename = '%s-%s-%s-%s.prof' % (
+            filename = '%s-%s-%s-%s' % (
                 timestamp, pageid, oopsid,
                 threading.currentThread().getName())
+            if 'stdlib' in actions:
+                filename += '.prof'
+            else:
+                # The Bzr stats class looks at the filename to know to use
+                # callgrind syntax.
+                filename = 'callgrind.out.' + filename
             dump_path = os.path.join(dump_path, filename)
-            prof_stats.save(dump_path, format="callgrind")
+            prof_stats.save(dump_path)
+            template_context['dump_path'] = os.path.abspath(dump_path)
         if is_html and 'show' in actions:
             # Generate raw OOPS results.
             f = StringIO.StringIO()
@@ -167,6 +259,8 @@
                 f = StringIO.StringIO()
                 prof_stats.pprint(file=f)
                 template_context[name] = f.getvalue()
+        # Try to free some more memory.
+        del prof_stats
     template_context['dump_path'] = os.path.abspath(dump_path)
     if actions and is_html:
         # Hack the new HTML in at the end of the page.
@@ -219,8 +313,8 @@
                 action for action in (
                     item.strip().lower() for item in actions.split(','))
                 if action)
-            result.intersection_update(('log', 'show'))
-            if not result:
+            result.intersection_update(('log', 'show', 'stdlib'))
+            if not result.difference(('stdlib',)):
                 result.add('help')
     return result
 

=== modified file 'lib/lp/services/profile/tests.py'
--- lib/lp/services/profile/tests.py	2011-02-24 06:33:25 +0000
+++ lib/lp/services/profile/tests.py	2011-08-08 13:10:47 +0000
@@ -12,7 +12,6 @@
 import glob
 import os
 import time
-import unittest
 
 from lp.testing import TestCase
 from bzrlib.lsprof import BzrProfiler
@@ -161,6 +160,20 @@
             None)
         self.assertEquals(profile._profilers.actions, set(('log', 'show')))
 
+    def test_optional_profiling_with_stdliblog_request_starts_profiling(self):
+        """If profiling is allowed and a request with the "log" marker
+        URL segment is made with "stdlib", profiling starts with the stdlib
+        profiler."""
+        self.pushProfilingConfig(profiling_allowed='True')
+        profile.start_request(
+            self._get_start_event('/++profile++log,stdlib/'))
+        self.assertIsInstance(profile._profilers.profiler,
+                              profile.StdLibProfiler)
+        self.assertIs(
+            getattr(profile._profilers, 'memory_profile_start', None),
+            None)
+        self.assertEquals(profile._profilers.actions, set(('log', 'stdlib')))
+
     def test_forced_profiling_registers_action(self):
         """profile_all_requests should register as a log action"""
         self.pushProfilingConfig(
@@ -184,6 +197,18 @@
             None)
         self.assertEquals(profile._profilers.actions, set(('help', )))
 
+    def test_optional_profiling_with_stdlib_only_helps(self):
+        """If profiling is allowed and a request with the marker URL segment
+        is made incorrectly, profiling does not start and help is an action.
+        """
+        self.pushProfilingConfig(profiling_allowed='True')
+        profile.start_request(self._get_start_event('/++profile++stdlib/'))
+        self.assertIs(getattr(profile._profilers, 'profiler', None), None)
+        self.assertIs(
+            getattr(profile._profilers, 'memory_profile_start', None),
+            None)
+        self.assertEquals(profile._profilers.actions, set(('help', 'stdlib')))
+
     def test_forced_profiling_with_wrong_request_helps(self):
         """If profiling is forced and a request with the marker URL segment
         is made incorrectly, profiling starts and help is an action.
@@ -216,15 +241,9 @@
         self.assertEquals(profile._profilers.actions, set(('show', )))
 
 
-class TestRequestEndHandler(BaseTest):
-    """Tests for the end-request handler.
-
-    If the start-request handler is broken, these tests will fail too, so fix
-    the tests in the above test case first.
-
-    See lib/canonical/doc/profiling.txt for an end-user description
-    of the functionality.
-    """
+class BaseRequestEndHandlerTest(BaseTest):
+    # These are shared by tests of the bzr profiler, the stdlib profiler,
+    # and the memory analysis.
 
     def setUp(self):
         TestCase.setUp(self)
@@ -271,10 +290,19 @@
         return result
 
     def getProfilePaths(self):
-        return glob.glob(os.path.join(self.profile_dir, '*.prof'))
-
-    #########################################################################
-    # Tests
+        return glob.glob(os.path.join(self.profile_dir, 'callgrind.out.*'))
+
+
+class TestBasicRequestEndHandler(BaseRequestEndHandlerTest):
+    """Tests for the end-request handler.
+
+    If the start-request handler is broken, these tests will fail too, so fix
+    the tests in the above test case first.
+
+    See lib/canonical/doc/profiling.txt for an end-user description
+    of the functionality.
+    """
+
     def test_config_stops_profiling(self):
         """The ``profiling_allowed`` configuration should disable all
         profiling, even if it is requested"""
@@ -299,6 +327,69 @@
         self.assertEqual(self.getProfilePaths(), [])
         self.assertCleanProfilerState()
 
+    def test_forced_profiling_logs(self):
+        """profile_all_requests should register as a log action"""
+        self.pushProfilingConfig(
+            profiling_allowed='True', profile_all_requests='True')
+        request = self.endRequest('/')
+        self.assertIsInstance(request.oops, ErrorReport)
+        response = self.getAddedResponse(request)
+        self.assertIn('Profile was logged to', response)
+        self.assertIn('profile_all_requests: True', response)
+        self.assertNotIn('Top Inline Time', response)
+        self.assertEqual(self.getMemoryLog(), [])
+        paths = self.getProfilePaths()
+        self.assertEqual(len(paths), 1)
+        self.assertIn(paths[0], response)
+        self.assertCleanProfilerState()
+
+    def test_optional_profiling_with_wrong_request_helps(self):
+        """If profiling is allowed and a request with the marker URL segment
+        is made incorrectly, profiling does not start and help is an action.
+        """
+        self.pushProfilingConfig(profiling_allowed='True')
+        request = self.endRequest('/++profile++')
+        self.assertIs(getattr(request, 'oops', None), None)
+        response = self.getAddedResponse(request)
+        self.assertIn('<h2>Help</h2>', response)
+        self.assertNotIn('Top Inline Time', response)
+        self.assertEqual(self.getMemoryLog(), [])
+        self.assertEqual(self.getProfilePaths(), [])
+        self.assertCleanProfilerState()
+
+    def test_forced_profiling_with_wrong_request_helps(self):
+        """If profiling is forced and a request with the marker URL segment
+        is made incorrectly, profiling starts and help is an action.
+        """
+        self.pushProfilingConfig(
+            profiling_allowed='True', profile_all_requests='True')
+        request = self.endRequest('/++profile++')
+        self.assertIsInstance(request.oops, ErrorReport)
+        response = self.getAddedResponse(request)
+        self.assertIn('<h2>Help</h2>', response)
+        self.assertIn('Profile was logged to', response)
+        self.assertIn('profile_all_requests: True', response)
+        self.assertNotIn('Top Inline Time', response)
+        self.assertEqual(self.getMemoryLog(), [])
+        paths = self.getProfilePaths()
+        self.assertEqual(len(paths), 1)
+        self.assertIn(paths[0], response)
+        self.assertCleanProfilerState()
+
+
+class TestBzrProfilerRequestEndHandler(BaseRequestEndHandlerTest):
+    """Tests for the end-request handler of the BzrProfiler.
+
+    If the start-request handler is broken, these tests will fail too, so fix
+    the tests in the above test case first.
+
+    See lib/canonical/doc/profiling.txt for an end-user description
+    of the functionality.
+    """
+
+    # Note that these tests are re-used by TestStdLibProfilerRequestEndHandler
+    # below.
+
     def test_optional_profiling_with_show_request_profiles(self):
         """If profiling is allowed and a request with the "show" marker
         URL segment is made, profiling starts."""
@@ -340,54 +431,35 @@
         self.assertIn(paths[0], response)
         self.assertCleanProfilerState()
 
-    def test_forced_profiling_logs(self):
-        """profile_all_requests should register as a log action"""
-        self.pushProfilingConfig(
-            profiling_allowed='True', profile_all_requests='True')
-        request = self.endRequest('/')
-        self.assertIsInstance(request.oops, ErrorReport)
-        response = self.getAddedResponse(request)
-        self.assertIn('Profile was logged to', response)
-        self.assertIn('profile_all_requests: True', response)
-        self.assertNotIn('Top Inline Time', response)
-        self.assertEqual(self.getMemoryLog(), [])
-        paths = self.getProfilePaths()
-        self.assertEqual(len(paths), 1)
-        self.assertIn(paths[0], response)
-        self.assertCleanProfilerState()
-
-    def test_optional_profiling_with_wrong_request_helps(self):
-        """If profiling is allowed and a request with the marker URL segment
-        is made incorrectly, profiling does not start and help is an action.
-        """
-        self.pushProfilingConfig(profiling_allowed='True')
-        request = self.endRequest('/++profile++')
-        self.assertIs(getattr(request, 'oops', None), None)
-        response = self.getAddedResponse(request)
-        self.assertIn('<h2>Help</h2>', response)
-        self.assertNotIn('Top Inline Time', response)
-        self.assertEqual(self.getMemoryLog(), [])
-        self.assertEqual(self.getProfilePaths(), [])
-        self.assertCleanProfilerState()
-
-    def test_forced_profiling_with_wrong_request_helps(self):
-        """If profiling is forced and a request with the marker URL segment
-        is made incorrectly, profiling starts and help is an action.
-        """
-        self.pushProfilingConfig(
-            profiling_allowed='True', profile_all_requests='True')
-        request = self.endRequest('/++profile++')
-        self.assertIsInstance(request.oops, ErrorReport)
-        response = self.getAddedResponse(request)
-        self.assertIn('<h2>Help</h2>', response)
-        self.assertIn('Profile was logged to', response)
-        self.assertIn('profile_all_requests: True', response)
-        self.assertNotIn('Top Inline Time', response)
-        self.assertEqual(self.getMemoryLog(), [])
-        paths = self.getProfilePaths()
-        self.assertEqual(len(paths), 1)
-        self.assertIn(paths[0], response)
-        self.assertCleanProfilerState()
+
+class TestStdLibProfilerRequestEndHandler(TestBzrProfilerRequestEndHandler):
+    """Tests for the end-request handler of the stdlib profiler.
+
+    If the start-request handler is broken, these tests will fail too, so fix
+    the tests in the above test case first.
+
+    See lib/canonical/doc/profiling.txt for an end-user description
+    of the functionality.
+    """
+    # Take over the BzrProfiler questions to test the stdlib variant.
+
+    def getProfilePaths(self):
+        return glob.glob(os.path.join(self.profile_dir, '*.prof'))
+
+    def endRequest(self, path):
+        return TestBzrProfilerRequestEndHandler.endRequest(self,
+            path.replace('++profile++', '++profile++stdlib,'))
+
+
+class TestMemoryProfilerRequestEndHandler(BaseRequestEndHandlerTest):
+    """Tests for the end-request handler of the memory profile.
+
+    If the start-request handler is broken, these tests will fail too, so fix
+    the tests in the above test case first.
+
+    See lib/canonical/doc/profiling.txt for an end-user description
+    of the functionality.
+    """
 
     def test_memory_profile(self):
         "Does the memory profile work?"
@@ -432,10 +504,20 @@
         self.assertEqual(self.getProfilePaths(), [])
         self.assertCleanProfilerState()
 
+
+class TestOOPSRequestEndHandler(BaseRequestEndHandlerTest):
+    """Tests for the end-request handler of the OOPS output.
+
+    If the start-request handler is broken, these tests will fail too, so fix
+    the tests in the above test case first.
+
+    See lib/canonical/doc/profiling.txt for an end-user description
+    of the functionality.
+    """
+
     def test_profiling_oops_is_informational(self):
         self.pushProfilingConfig(profiling_allowed='True')
         request = self.endRequest('/++profile++show/')
-        response = self.getAddedResponse(request)
         self.assertIsInstance(request.oops, ErrorReport)
         self.assertTrue(request.oops.informational)
         self.assertEquals(request.oops.type, 'ProfilingOops')
@@ -472,8 +554,9 @@
             memory_profile_log='.')
         event = BeforeTraverseEvent(None,
             self._get_request('/++profile++show,log'))
-        with FeatureFixture({'profiling.enabled':'on'}):
+        with FeatureFixture({'profiling.enabled': 'on'}):
             profile.before_traverse(event)
             self.assertTrue(profile._profilers.profiling)
             self.assertIsInstance(profile._profilers.profiler, BzrProfiler)
-            self.assertEquals(profile._profilers.actions, set(('show', 'log')))
+            self.assertEquals(profile._profilers.actions, set(
+                ('show', 'log')))