Merge lp:~lifeless/launchpad/bug-724033 into lp:launchpad

[Robert Collins]

[r=jtv][bug=724033] More scaling improvements for BugTask:+index - use the edit form milestone cache for the view rows too.

More scaling improvements for BugTask:+index: this should halve the number of milestone/distro/product lookups happening by using an existing caching layer for milestones more effectively. Further work beyond this involves making the cache eager load for all contexts at once, but these are separate changes and this change alone will make a significant difference.

[Jeroen T. Vermeulen (jtv)]

Strange to see the check that "return None" returns None—and good riddance I suppose.

I would still have liked to see a test, especially since the version I originally tried to review had a performance bug and the overall page is not in a shape that can be tested for query counts yet. I know it's arguably not a "new layer" and this is just a refactoring but you're trying to make an important change and you almost made things worse because of untested new code—however small.

review: Approve (code)

[Robert Collins (lifeless)]

Thanks for the review. I'm going to skip the test - but I'd like to note that the test you suggested would have passed even with the original bug in place.

=== modified file 'lib/canonical/launchpad/doc/vocabularies.txt'
--- lib/canonical/launchpad/doc/vocabularies.txt	2010-10-18 22:24:59 +0000
+++ lib/canonical/launchpad/doc/vocabularies.txt	2011-03-07 02:54:15 +0000
@@ -85,9 +85,6 @@
     >>> debian in using_malone_vocabulary
     False
 
-    >>> using_malone_vocabulary.getQuery() is None
-    True
-
     >>> term = using_malone_vocabulary.getTerm(ubuntu)
     >>> print term.token, term.value.displayname, term.title
     ubuntu Ubuntu Ubuntu

=== modified file 'lib/canonical/launchpad/vocabularies/dbobjects.py'
--- lib/canonical/launchpad/vocabularies/dbobjects.py	2010-12-01 18:58:44 +0000
+++ lib/canonical/launchpad/vocabularies/dbobjects.py	2011-03-07 02:54:15 +0000
@@ -487,9 +487,6 @@
         return (IDistribution.providedBy(obj)
                 and obj.bug_tracking_usage == ServiceUsage.LAUNCHPAD)
 
-    def getQuery(self):
-        return None
-
     def getTerm(self, obj):
         if obj not in self:
             raise LookupError(obj)

=== modified file 'lib/canonical/launchpad/webapp/vocabulary.py'
--- lib/canonical/launchpad/webapp/vocabulary.py	2010-12-01 18:58:44 +0000
+++ lib/canonical/launchpad/webapp/vocabulary.py	2011-03-07 02:54:15 +0000
@@ -281,9 +281,6 @@
             found_obj = self._table.selectOne(clause)
             return found_obj is not None
 
-    def getQuery(self):
-        return None
-
     def getTerm(self, value):
         # Short circuit. There is probably a design problem here since
         # we sometimes get the id and sometimes an SQLBase instance.

=== modified file 'lib/lp/bugs/browser/bugtask.py'
--- lib/lp/bugs/browser/bugtask.py	2011-03-03 10:58:03 +0000
+++ lib/lp/bugs/browser/bugtask.py	2011-03-07 02:54:15 +0000
@@ -3124,14 +3124,25 @@
 
     def __init__(self):
         self.vocabularies = {}
+        self.contexts = set()
 
     def __call__(self, context):
+        assert context in self.contexts, ("context %r not added to "
+            "self.contexts (%r)." % (context, self.contexts))
+        self._load()
         target = MilestoneVocabulary.getMilestoneTarget(context)
-        milestone_vocabulary = self.vocabularies.get(target)
-        if milestone_vocabulary is None:
-            milestone_vocabulary = MilestoneVocabulary(context)
+        return self.vocabularies[target]
+
+    def _load(self):
+        """Load all the vocabularies, once only."""
+        if self.vocabularies:
+            return
+        targets = set(
+            map(MilestoneVocabulary.getMilestoneTarget, self.contexts))
+        # TODO: instantiate for all targets at once.
+        for target in targets:
+            milestone_vocabulary = MilestoneVocabulary(target)
             self.vocabularies[target] = milestone_vocabulary
-        return milestone_vocabulary
 
 
 class BugTasksAndNominationsView(LaunchpadView):
@@ -3205,6 +3216,7 @@
         The view's is_conjoined_slave and is_converted_to_question
         attributes are set, as well as the edit view.
         """
+        self.cached_milestone_source.contexts.add(context)
         view = getMultiAdapter(
             (context, self.request),
             name='+bugtasks-and-nominations-table-row')
@@ -3212,6 +3224,7 @@
         view.is_conjoined_slave = is_conjoined_slave
         if IBugTask.providedBy(context):
             view.target_link_title = self.getTargetLinkTitle(context.target)
+        view.milestone_source = self.cached_milestone_source
 
         view.edit_view = getMultiAdapter(
             (context, self.request), name='+edit-form')
@@ -3383,6 +3396,10 @@
     target_link_title = None
     many_bugtasks = False
 
+    def __init__(self, context, request):
+        super(BugTaskTableRowView, self).__init__(context, request)
+        self.milestone_source = MilestoneVocabulary
+
     def canSeeTaskDetails(self):
         """Whether someone can see a task's status details.
 
@@ -3509,7 +3526,7 @@
     @cachedproperty
     def _visible_milestones(self):
         """The visible milestones for this context."""
-        return MilestoneVocabulary(self.context).visible_milestones
+        return self.milestone_source(self.context).visible_milestones
 
     @property
     def milestone_widget_items(self):

=== modified file 'lib/lp/registry/vocabularies.py'
--- lib/lp/registry/vocabularies.py	2011-01-31 19:44:36 +0000
+++ lib/lp/registry/vocabularies.py	2011-03-07 02:54:15 +0000
@@ -1291,6 +1291,9 @@
         for milestone in self.visible_milestones:
             yield self.toTerm(milestone)
 
+    def __len__(self):
+        return len(self.visible_milestones)
+
     def __contains__(self, obj):
         if IProjectGroupMilestone.providedBy(obj):
             # ProjectGroup milestones are pseudo content objects