launchpad-reviewers team mailing list archive

[Raphaël Victor Badin <raphael.badin@xxxxxxxxxxxxx>]

Raphaël Victor Badin has proposed merging lp:~rvb/launchpad/fix-messagebody-bug-760733 into lp:launchpad/db-devel with lp:~rvb/launchpad/fix-packageset-bug-760733 as a prerequisite.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #760733 in Launchpad itself: "+localpackagediffs performance needs more work"
  https://bugs.launchpad.net/launchpad/+bug/760733

For more details, see:
https://code.launchpad.net/~rvb/launchpad/fix-messagebody-bug-760733/+merge/61249

This branch builds up on the work that has been done to improve the performance of the difference pages. It caches the message chunks of the last comment associated with each displayed DSD. The number of queries is now independent of the number of displayed DSD.

= Test =
./bin/test -cvv test_distroseries test_queries_

= Q/A =
n/a
-- 
https://code.launchpad.net/~rvb/launchpad/fix-messagebody-bug-760733/+merge/61249
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/launchpad/fix-messagebody-bug-760733 into lp:launchpad/db-devel.

=== modified file 'lib/lp/registry/browser/tests/test_distroseries.py'
--- lib/lp/registry/browser/tests/test_distroseries.py	2011-05-16 15:44:48 +0000
+++ lib/lp/registry/browser/tests/test_distroseries.py	2011-05-17 15:18:53 +0000
@@ -672,15 +672,11 @@
         self.addDetail(
             "statement-count-per-row-average",
             text_content(u"%.2f" % statement_count_per_row))
-        # XXX: GavinPanella 2011-04-12 bug=760733: Reducing the query count
-        # further needs work. Ideally this test would be along the lines of
-        # recorder3.count == recorder2.count. 2 queries above the recorder2
-        # count is 1 query per difference which is not acceptable, but is
-        # *far* better than without the changes introduced by landing this.
-        compromise_statement_count = recorder2.count + 2
+        # Query count is ~O(1) (i.e. not dependent of the number of
+        # differences displayed).
         self.assertThat(
             recorder3, HasQueryCount(
-                LessThan(compromise_statement_count + 1)))
+                LessThan(recorder2.count + 1)))
 
     def test_queries_single_parent(self):
         dsp = self.factory.makeDistroSeriesParent()

=== modified file 'lib/lp/registry/model/distroseriesdifference.py'
--- lib/lp/registry/model/distroseriesdifference.py	2011-05-17 14:40:38 +0000
+++ lib/lp/registry/model/distroseriesdifference.py	2011-05-17 15:18:53 +0000
@@ -42,7 +42,6 @@
 from canonical.launchpad.components.decoratedresultset import (
     DecoratedResultSet,
     )
-from lp.services.messages.model.message import Message
 from canonical.launchpad.interfaces.lpstorm import (
     IMasterStore,
     IStore,
@@ -74,6 +73,10 @@
 from lp.registry.model.sourcepackagename import SourcePackageName
 from lp.services.database import bulk
 from lp.services.database.stormbase import StormBase
+from lp.services.messages.model.message import (
+    Message,
+    MessageChunk,
+    )
 from lp.services.propertycache import (
     cachedproperty,
     clear_property_cache,
@@ -222,11 +225,27 @@
 
     grouped = {}
     for dsd_id, packageset in results:
-        if dsd_id in grouped:
-            grouped[dsd_id].append(packageset)
-        else:
-            grouped[dsd_id] = [packageset]
-
+        packagesets = grouped.setdefault(dsd_id, [])
+        packagesets.append(packageset)
+    return grouped
+
+
+def message_chunks(messages):
+    """Return the message chunks for the given messages.
+
+    Returns a dict with the list of `MessageChunk` for each message id.
+
+    :param messages: An iterable of `Message` instances.
+    """
+    store = IStore(MessageChunk)
+    chunks = store.find(MessageChunk,
+        MessageChunk.messageID.is_in(m.id for m in messages))
+
+    grouped = {}
+    for chunk in chunks:
+        message_id = chunk.messageID
+        chunks = grouped.setdefault(message_id, [])
+        chunks.append(chunk)
     return grouped
 
 
@@ -383,6 +402,15 @@
             dsd_packagesets = packagesets(dsds, in_parent=False)
             dsd_parent_packagesets = packagesets(dsds, in_parent=True)
 
+            # Cache latest messages contents (MessageChunk).
+            messages = bulk.load_related(
+                Message, latest_comments, ['message_id'])
+            chunks = message_chunks(messages)
+            for msg in messages:
+                cache = get_property_cache(msg)
+                cache.text_contents = Message.chunks_text(
+                    chunks.get(msg.id, []))
+
             for dsd in dsds:
                 spn_id = dsd.source_package_name_id
                 cache = get_property_cache(dsd)

=== modified file 'lib/lp/registry/templates/distroseries-localdifferences.pt'
--- lib/lp/registry/templates/distroseries-localdifferences.pt	2011-05-13 15:13:54 +0000
+++ lib/lp/registry/templates/distroseries-localdifferences.pt	2011-05-17 15:18:53 +0000
@@ -149,11 +149,6 @@
                 </tal:derived>
               </td>
 
-              <tal:comment replace="nothing">
-                XXX: GavinPanella 2011-04-14 bug=760733: In
-                TestDistroSeriesLocalDifferences.test_queries, this
-                cell needs 1 QUERY per row: MessageChunk
-              </tal:comment>
               <td>
                 <tal:comment tal:define="comment difference/latest_comment"
                              tal:condition="comment">