Merge lp:~gary/launchpad/bug724025 into lp:launchpad

[Gary Poster]

[incr] [r=gmb][bug=724025] Speed up an inner loop for BugTask:+index with many tasks.

This branch takes the previous work to reduce the Python cost of many tasks on the BugTask:+index page and extends it a bit more. This time, we optimize the inner loop of rendering many tasks by calculating the shared data for the view carefully at initialization, and by using a Chameleon template for the rendering.

Note that this code has *no* SQL queries while the inner loop is run, thanks to previous work by Robert. This is entirely rendering time.

The initialization saves a bit less than half a second on my system with 160/320 bugtasks (the doubled number represents the parent bugtask for a sourcepackage). Hopefully it is fairly straightforward to read: I collect the data, and then the template is simplified a bit.

The switch to Chameleon is even less to read. It also saves around half a second in the conditions I described above. You might know that I disabled Chameleon earlier for ++profile++. The new version does not write compiled files to disk by default. While I would prefer it to do this in the future, it is simpler to do what I have done here. The first time BugTask:+index is rendered, a bit of time will be spent to calculate the bytecode for the template; subsequently, the cost will be gone until the process restarts.

There are no tests, because it is existing functionality, and we already were not generating SQL in the inner loop.

Lint is happy.

To QA, go to any BugTask:+index page. The first time a +index page is rendered for that process, it will be a bit slower, as described above. Subsequently, you should have a modest speed gain.

This bug will get one or two more changes before I move on. Minimally, I plan to make only the first 50 bugtasks render initially, with the current context bugtask at the top, and then JS to render the remaining tasks on demand. Ideally, I'll also address an unrelated aspect of why this page is slow: we need to pre-load associated branches, using code that Danilo worked on in another branch.

Thank you

[Graham Binns (gmb)]

review: Approve (code)

[Gary Poster (gary)]

This should have been [incr]. :-/

=== modified file 'lib/lp/bugs/browser/bugtask.py'
--- lib/lp/bugs/browser/bugtask.py	2011-08-22 13:13:28 +0000
+++ lib/lp/bugs/browser/bugtask.py	2011-08-23 01:53:28 +0000
@@ -75,7 +75,7 @@
 from lazr.uri import URI
 from pytz import utc
 from simplejson import dumps
-from z3c.ptcompat import ViewPageTemplateFile
+from z3c.pt.pagetemplate import ViewPageTemplateFile
 from zope import (
     component,
     formlib,
@@ -1067,7 +1067,7 @@
 class BugTaskBugWatchMixin:
     """A mixin to be used where a BugTask view displays BugWatch data."""
 
-    @property
+    @cachedproperty
     def bug_watch_error_message(self):
         """Return a browser-useable error message for a bug watch."""
         if not self.context.bugwatch:
@@ -3383,10 +3383,50 @@
     target_link_title = None
     many_bugtasks = False
 
+    template = ViewPageTemplateFile(
+        '../templates/bugtask-tasks-and-nominations-table-row.pt')
+
     def __init__(self, context, request):
         super(BugTaskTableRowView, self).__init__(context, request)
         self.milestone_source = MilestoneVocabulary
 
+    def initialize(self):
+        super(BugTaskTableRowView, self).initialize()
+        link = canonical_url(self.context)
+        edit_link = link + '/+editstatus'
+        view_link = link + '/+viewstatus'
+        can_edit = check_permission('launchpad.Edit', self.context)
+        task_link = edit_link if can_edit else view_link
+        bugtask_id = self.context.id
+        launchbag = getUtility(ILaunchBag)
+        is_primary = self.context.id == launchbag.bugtask.id
+        self.data = dict(
+            # Looking at many_bugtasks is an important optimization.  With
+            # 150+ bugtasks, it can save three or four seconds of rendering
+            # time.
+            expandable=(not self.many_bugtasks and self.canSeeTaskDetails()),
+            indent_task=ISeriesBugTarget.providedBy(self.context.target),
+            is_conjoined_slave=self.is_conjoined_slave,
+            task_link=task_link,
+            edit_link=edit_link,
+            view_link=view_link,
+            can_edit=can_edit,
+            link=link,
+            id=bugtask_id,
+            row_id='tasksummary%d' % bugtask_id,
+            form_row_id='task%d' % bugtask_id,
+            row_css_class='highlight' if is_primary else None,
+            target_link=canonical_url(self.context.target),
+            target_link_title=self.target_link_title,
+            user_can_edit_importance=self.context.userCanEditImportance(
+                self.user),
+            importance_css_class='importance' + self.context.importance.name,
+            importance_title=self.context.importance.title,
+            # We always look up all milestones, so there's no harm
+            # using len on the list here and avoid the COUNT query.
+            target_has_milestones=len(self._visible_milestones) > 0,
+            )
+
     def canSeeTaskDetails(self):
         """Whether someone can see a task's status details.
 
@@ -3405,42 +3445,6 @@
                 self.context.bug.duplicateof is None and
                 not self.is_converted_to_question)
 
-    def expandable(self):
-        """Can the task's details be expanded?
-
-        They can if there are not too many bugtasks, and if the user can see
-        the task details."""
-        # Looking at many_bugtasks is an important optimization.  With 150+
-        # bugtasks, it can save three or four seconds of rendering time.
-        return not self.many_bugtasks and self.canSeeTaskDetails()
-
-    def getTaskRowCSSClass(self):
-        """The appropriate CSS class for the row in the Affects table.
-
-        Currently this consists solely of highlighting the current context.
-        """
-        bugtask = self.context
-        if bugtask == getUtility(ILaunchBag).bugtask:
-            return 'highlight'
-        else:
-            return None
-
-    def shouldIndentTask(self):
-        """Should this task be indented in the task listing on the bug page?
-
-        Returns True or False.
-        """
-        return ISeriesBugTarget.providedBy(self.context.target)
-
-    def taskLink(self):
-        """Return the proper link to the bugtask whether it's editable."""
-        user = getUtility(ILaunchBag).user
-        bugtask = self.context
-        if check_permission('launchpad.Edit', user):
-            return canonical_url(bugtask) + "/+editstatus"
-        else:
-            return canonical_url(bugtask) + "/+viewstatus"
-
     def _getSeriesTargetNameHelper(self, bugtask):
         """Return the short name of bugtask's targeted series."""
         series = bugtask.distroseries or bugtask.productseries
@@ -3535,15 +3539,6 @@
 
         return items
 
-    @cachedproperty
-    def target_has_milestones(self):
-        """Are there any milestones we can target?
-
-        We always look up all milestones, so there's no harm
-        using len on the list here and avoid the COUNT query.
-        """
-        return len(self._visible_milestones) > 0
-
     def bugtask_canonical_url(self):
         """Return the canonical url for the bugtask."""
         return canonical_url(self.context)
@@ -3564,7 +3559,7 @@
         """
         return self.user is not None
 
-    @property
+    @cachedproperty
     def user_can_edit_milestone(self):
         """Can the user edit the Milestone field?
 
@@ -3602,44 +3597,40 @@
                     'title': filter.title,
                     'description': filter.description,
                     })
-
         # Display the search field only if the user can set any person
         # or team
-        user = getUtility(ILaunchBag).user
+        user = self.user
         hide_assignee_team_selection = (
             not self.context.userCanSetAnyAssignee(user) and
             (user is None or user.teams_participated_in.count() == 0))
-        return dumps({
-            'row_id': 'tasksummary%s' % self.context.id,
-            'bugtask_path': '/'.join(
-                [''] + canonical_url(self.context).split('/')[3:]),
-            'prefix': get_prefix(self.context),
-            'assignee_value': self.context.assignee
-                and self.context.assignee.name,
-            'assignee_is_team': self.context.assignee
-                and self.context.assignee.is_team,
-            'assignee_vocabulary': assignee_vocabulary,
-            'assignee_vocabulary_filters': filter_details,
-            'hide_assignee_team_selection': hide_assignee_team_selection,
-            'user_can_unassign': self.context.userCanUnassign(user),
-            'target_is_product': IProduct.providedBy(self.context.target),
-            'status_widget_items': self.status_widget_items,
-            'status_value': self.context.status.title,
-            'importance_widget_items': self.importance_widget_items,
-            'importance_value': self.context.importance.title,
-            'milestone_widget_items': self.milestone_widget_items,
-            'milestone_value': (self.context.milestone and
-                                canonical_url(
-                                    self.context.milestone,
-                                    request=IWebServiceClientRequest(
-                                        self.request)) or
-                                None),
-            'user_can_edit_assignee': self.user_can_edit_assignee,
-            'user_can_edit_milestone': self.user_can_edit_milestone,
-            'user_can_edit_status': not self.context.bugwatch,
-            'user_can_edit_importance': (
-                self.user_can_edit_importance and
-                not self.context.bugwatch)})
+        cx = self.context
+        return dumps(dict(
+            row_id=self.data['row_id'],
+            bugtask_path='/'.join([''] + self.data['link'].split('/')[3:]),
+            prefix=get_prefix(cx),
+            assignee_value=cx.assignee and cx.assignee.name,
+            assignee_is_team=cx.assignee and cx.assignee.is_team,
+            assignee_vocabulary=assignee_vocabulary,
+            assignee_vocabulary_filters=filter_details,
+            hide_assignee_team_selection=hide_assignee_team_selection,
+            user_can_unassign=cx.userCanUnassign(user),
+            target_is_product=IProduct.providedBy(cx.target),
+            status_widget_items=self.status_widget_items,
+            status_value=cx.status.title,
+            importance_widget_items=self.importance_widget_items,
+            importance_value=cx.importance.title,
+            milestone_widget_items=self.milestone_widget_items,
+            milestone_value=(
+                canonical_url(
+                    cx.milestone,
+                    request=IWebServiceClientRequest(self.request))
+                if cx.milestone else None),
+            user_can_edit_assignee=self.user_can_edit_assignee,
+            user_can_edit_milestone=self.user_can_edit_milestone,
+            user_can_edit_status=not cx.bugwatch,
+            user_can_edit_importance=(
+                self.user_can_edit_importance and not cx.bugwatch)
+            ))
 
 
 class BugsBugTaskSearchListingView(BugTaskSearchListingView):

=== modified file 'lib/lp/bugs/browser/configure.zcml'
--- lib/lp/bugs/browser/configure.zcml	2011-08-16 14:21:39 +0000
+++ lib/lp/bugs/browser/configure.zcml	2011-08-23 01:53:28 +0000
@@ -564,8 +564,7 @@
             for="lp.bugs.interfaces.bugtask.IBugTask"
             class="lp.bugs.browser.bugtask.BugTaskTableRowView"
             permission="launchpad.View"
-            name="+bugtasks-and-nominations-table-row"
-            template="../templates/bugtask-tasks-and-nominations-table-row.pt"/>
+            name="+bugtasks-and-nominations-table-row"/>
         <browser:page
             for="lp.bugs.interfaces.bugtarget.IBugTarget"
             permission="zope.Public"

=== modified file 'lib/lp/bugs/templates/bugtask-tasks-and-nominations-table-row.pt'
--- lib/lp/bugs/templates/bugtask-tasks-and-nominations-table-row.pt	2011-08-10 16:14:13 +0000
+++ lib/lp/bugs/templates/bugtask-tasks-and-nominations-table-row.pt	2011-08-23 01:53:28 +0000
@@ -1,44 +1,30 @@
 <tal:bugtask
   xmlns:tal="http://xml.zope.org/namespaces/tal"
   xmlns:metal="http://xml.zope.org/namespaces/metal"
-  define="expandable view/expandable;
-          indent_task view/shouldIndentTask;
-          is_conjoined_slave view/is_conjoined_slave;
-          tasklink view/taskLink;
-          row_id string:tasksummary${context/id};
-          form_row_id string:task${context/id}">
-  <tr tal:define="editstatus_url string:${context/fmt:url}/+editstatus"
-      tal:attributes="class view/getTaskRowCSSClass; id row_id">
+  define="data view/data">
+  <tr tal:attributes="class data/row_css_class; id data/row_id">
     <td>
-      <metal:expander
-          metal:define-macro="expander"
-          tal:define="expander_link_text expander_link_text|nothing">
-        <a tal:condition="expandable"
-           tal:attributes="href tasklink" class="bugtask-expander">
-          <tal:link_text tal:replace="expander_link_text" />
-          &#8203;
-        </a>
-        <tal:no_expander tal:condition="not: expandable"
-                         tal:replace="expander_link_text" />
-      </metal:expander>
+      <a tal:condition="data/expandable"
+         tal:attributes="href data/task_link" class="bugtask-expander">
+        &#8203;
+      </a>
     </td>
     <td style="padding: 0.3em 0em 0.3em 1.5em"
-        tal:condition="indent_task"
-        tal:define="series_targetname view/getSeriesTargetName">
+        tal:condition="data/indent_task">
       <span class="sprite milestone"></span>
-      <tal:not-conjoined-task condition="not: is_conjoined_slave">
+      <tal:not-conjoined-task condition="not: data/is_conjoined_slave">
         <a
-          tal:attributes="href context/target/fmt:url"
-          tal:content="series_targetname"
+          tal:attributes="href data/target_link"
+          tal:content="view/getSeriesTargetName"
         />
       </tal:not-conjoined-task>
     </td>
-    <td tal:condition="not:indent_task">
-      <span tal:attributes="id string:bugtarget-picker-${row_id}">
+    <td tal:condition="not:data/indent_task">
+      <span tal:attributes="id string:bugtarget-picker-${data/row_id}">
         <span class="yui3-activator-data-box">
           <span title="This project&rsquo;s license has not been specified.">
-            <a tal:attributes="href context/target/fmt:url;
-                               title view/target_link_title;
+            <a tal:attributes="href data/target_link;
+                               title data/target_link_title;
                                class context/target/image:sprite_css"
                tal:content="context/bugtargetdisplayname" />
           </span>
@@ -50,7 +36,7 @@
       </span>
     </td>
 
-    <tal:conjoined-task condition="is_conjoined_slave">
+    <tal:conjoined-task condition="data/is_conjoined_slave">
     <td colspan="5" style="vertical-align: middle">
       <span class="discreet lesser" style="font-size: 100%">
         Status tracked in
@@ -61,79 +47,85 @@
     </td>
     </tal:conjoined-task>
 
-    <tal:not-conjoined-task condition="not:is_conjoined_slave">
+    <tal:not-conjoined-task condition="not:data/is_conjoined_slave">
     <td style="width: 20%; vertical-align: middle">
       <div class="status-content"
-           style="width: 100%; float: left">
+           style="width: 100%; float: left"
+           tal:define="status context/status">
         <a href="+editstatus"
-           tal:attributes="class string:value status${context/status/name};
-                           href editstatus_url"
+           tal:attributes="class string:value status${status/name};
+                           href data/edit_link"
            style="float: left"
-           tal:content="context/status/title" />
+           tal:content="status/title" />
         <a href="+editstatus" style="margin-left: 3px"
-           tal:attributes="href editstatus_url">
+           tal:attributes="href data/edit_link">
           <img class="editicon" src="/@@/edit" />
         </a>
       </div>
     </td>
 
-    <td tal:condition="view/user_can_edit_importance"
+    <td tal:condition="data/user_can_edit_importance"
         style="width: 20%; vertical-align: middle">
       <div class="importance-content"
            style="width: 100%; float: left">
         <a href="+editstatus"
-           tal:attributes="class string:value importance${context/importance/name};
-                           href editstatus_url"
+           tal:attributes="class string:value ${data/importance_css_class};
+                           href data/edit_link"
            style="float: left"
-           tal:content="context/importance/title" />
+           tal:content="data/importance_title" />
         <a href="+editstatus" style="margin-left: 3px"
-           tal:attributes="href editstatus_url">
+           tal:attributes="href data/edit_link">
           <img class="editicon" src="/@@/edit" />
         </a>
       </div>
     </td>
-    <td tal:condition="not: view/user_can_edit_importance"
+    <td tal:condition="not: data/user_can_edit_importance"
         style="width: 15em; vertical-align: middle">
       <div class="importance-content"
            style="width: 100%; float: left">
         <span
-           tal:attributes="class string:value importance${context/importance/name}"
+           tal:attributes="class string:value ${data/importance_css_class}"
            style="float: left"
-           tal:content="context/importance/title" />
+           tal:content="data/importance_title" />
       </div>
     </td>
 
     <td style="width:20%; margin: 0; padding: 0;
-               vertical-align: middle; padding-left: 0.5em">
-      <tal:has_watch condition="context/bugwatch">
-        <div style="text-decoration: none; padding: 0.25em">
-          <tal:bugtracker-active
-              condition="context/bugwatch/bugtracker/active">
-            <span tal:condition="not:context/bugwatch/last_error_type"
-                  class="sprite bug-remote"></span>
-            <a tal:condition="context/bugwatch/last_error_type"
-                tal:attributes="href view/bug_watch_error_message/help_url"
-                target="help"
-                class="icon help">
-              <span class="sprite warning-icon"
-                    tal:attributes="title view/bug_watch_error_message/message"
-                    id="bugwatch-error-sprite"></span>
-            </a>
+               vertical-align: middle; padding-left: 0.5em"
+        tal:define="bugwatch context/bugwatch;">
+      <tal:has_watch condition="bugwatch">
+        <div style="text-decoration: none; padding: 0.25em"
+             tal:define="active_bugtracker bugwatch/bugtracker/active;">
+          <tal:bugtracker-active condition="active_bugtracker">
+            <tal:block define="last_error_type bugwatch/last_error_type;">
+              <span tal:condition="not:last_error_type"
+                    class="sprite bug-remote"></span>
+              <a tal:condition="last_error_type"
+                  tal:attributes="href view/bug_watch_error_message/help_url"
+                  target="help"
+                  class="icon help">
+                <span class="sprite warning-icon"
+                      tal:attributes=
+                            "title view/bug_watch_error_message/message"
+                      id="bugwatch-error-sprite"></span>
+              </a>
+            </tal:block>
           </tal:bugtracker-active>
-          <span tal:condition="not:context/bugwatch/bugtracker/active"
+          <span tal:condition="not:active_bugtracker"
                 class="sprite warning-icon"></span>
-          <a tal:replace="structure context/bugwatch/fmt:external-link" />
+          <a tal:replace="structure bugwatch/fmt:external-link" />
         </div>
       </tal:has_watch>
 
-      <tal:has_no_watch condition="not: context/bugwatch">
-        <span tal:attributes="id string:assignee-picker-${row_id}">
+      <tal:has_no_watch condition="not: bugwatch">
+        <span tal:attributes="id string:assignee-picker-${data/row_id}"
+              tal:define="assignee context/assignee">
           <span class="yui3-activator-data-box">
-            <a tal:condition="context/assignee"
-               tal:attributes="href context/assignee/fmt:url;
-                               class context/assignee/image:sprite_css"
-               tal:content="context/assignee/fmt:displayname" />
-            <tal:unassigned condition="not: context/assignee">
+            <a tal:condition="assignee"
+               tal:attributes="href assignee/fmt:url;
+                               class assignee/image:sprite_css"
+               tal:content="assignee/fmt:displayname" />
+            <tal:unassigned condition="not: assignee">
               Unassigned
             </tal:unassigned>
           </span>
@@ -147,11 +139,11 @@
 
     <td style="width: 20%; vertical-align: middle">
       <div class="milestone-content"
-           tal:condition="view/target_has_milestones"
+           tal:condition="data/target_has_milestones"
            style="width: 100%; float: left">
         <a tal:condition="view/user_can_edit_milestone"
            class="nulltext addicon js-action sprite add"
-           tal:attributes="href editstatus_url;
+           tal:attributes="href data/edit_link;
                            style view/style_for_add_milestone">
           Target to milestone
         </a>
@@ -159,7 +151,7 @@
            tal:attributes="href context/milestone/fmt:url | nothing"
            tal:content="context/milestone/title | nothing" />
         <a tal:condition="view/user_can_edit_milestone"
-           tal:attributes="href editstatus_url"
+           tal:attributes="href data/edit_link"
           ><img class="editicon" src="/@@/edit"
                 tal:attributes="style view/style_for_edit_milestone"
           /></a>
@@ -178,8 +170,8 @@
           Y.lp.bugs.bugtask_index.setup_bugtask_row(${view/js_config});
 
           // Set-up the expander on the bug task summary row.
-          var icon_node = Y.one('tr#${row_id} a.bugtask-expander');
-          var row_node = Y.one('tr#${form_row_id}');
+          var icon_node = Y.one('tr#${data/row_id} a.bugtask-expander');
+          var row_node = Y.one('tr#${data/form_row_id}');
           if (Y.Lang.isValue(row_node)) {
             // When no row is present, this is bug task on a project with
             // multiple per-series tasks, so we do not need to set
@@ -195,8 +187,8 @@
 
   <tal:form condition="view/displayEditForm">
     <tr
-      tal:attributes="id form_row_id"
-      tal:condition="expandable"
+      tal:attributes="id data/form_row_id"
+      tal:condition="data/expandable"
       class="bugtask-collapsible-content unseen"
     >
      <td colspan="7" tal:content="structure view/edit_view" />

=== modified file 'setup.py'
--- setup.py	2011-08-21 22:06:09 +0000
+++ setup.py	2011-08-23 01:53:28 +0000
@@ -29,8 +29,7 @@
         'ampoule',
         'BeautifulSoup',
         'bzr',
-        'chameleon.core',
-        'chameleon.zpt',
+        'Chameleon',
         'cssutils',
         # Required for pydkim
         'dnspython',

=== modified file 'versions.cfg'
--- versions.cfg	2011-08-22 07:09:21 +0000
+++ versions.cfg	2011-08-23 01:53:28 +0000
@@ -8,8 +8,7 @@
 amqplib = 0.6.1
 BeautifulSoup = 3.1.0.1
 bzr = 2.3.3
-chameleon.core = 1.0b35
-chameleon.zpt = 1.0b17
+Chameleon = 2.4.0
 ClientForm = 0.2.10
 cssutils = 0.9.6
 docutils = 0.5
@@ -52,6 +51,7 @@
 oops = 0.0.6
 oops-datedir-repo = 0.0.3
 oops-wsgi = 0.0.2
+ordereddict = 1.1
 paramiko = 1.7.4
 Paste = 1.7.2
 PasteDeploy = 1.3.3
@@ -91,6 +91,7 @@
 transaction = 1.0.0
 txamqp = 0.4
 Twisted = 11.0.0
+unittest2 = 0.5.1
 uuid = 1.30
 van.testing = 2.0.1
 wadllib = 1.2.0
@@ -116,7 +117,7 @@
 z3c.menu = 0.2.0
 z3c.optionstorage = 1.0.4
 z3c.pagelet = 1.0.2
-z3c.pt = 1.0b16
+z3c.pt = 2.1.3
 z3c.ptcompat = 0.5.3
 z3c.recipe.filetemplate = 2.1.0
 z3c.recipe.i18n = 0.5.3