launchpad-reviewers team mailing list archive

[Robert Collins <robertc@xxxxxxxxxxxxxxxxx>]

Robert Collins has proposed merging lp:~lifeless/launchpad/malone into lp:launchpad/devel with lp:~lifeless/launchpad/decoratedresultset as a prerequisite.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #497386 ScopedCollection:CollectionResource:#message-page-resource timeouts for bugs with many messages
  https://bugs.launchpad.net/bugs/497386
  #607776 +filebug-show-similar FTI query timeouts
  https://bugs.launchpad.net/bugs/607776


Remove listification from the internals of the bugs/message API.
-- 
https://code.launchpad.net/~lifeless/launchpad/malone/+merge/35511
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~lifeless/launchpad/malone into lp:launchpad/devel.

=== modified file 'lib/canonical/launchpad/database/message.py'
--- lib/canonical/launchpad/database/message.py	2010-08-29 21:24:47 +0000
+++ lib/canonical/launchpad/database/message.py	2010-09-15 10:01:12 +0000
@@ -82,6 +82,7 @@
 from lp.app.errors import NotFoundError
 from lp.registry.interfaces.person import validate_public_person
 from lp.services.job.model.job import Job
+from lp.services.propertycache import cachedproperty
 
 # this is a hard limit on the size of email we will be willing to store in
 # the database.
@@ -158,10 +159,14 @@
         """See IMessage."""
         return self.owner
 
-    @property
+    @cachedproperty
     def text_contents(self):
         """See IMessage."""
-        bits = [unicode(chunk) for chunk in self if chunk.content]
+        return Message.chunks_text(self.chunks)
+
+    @classmethod
+    def chunks_text(klass, chunks):
+        bits = [unicode(chunk) for chunk in chunks if chunk.content]
         return '\n\n'.join(bits)
 
     # XXX flacoste 2006-09-08: Bogus attribute only present so that

=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py	2010-09-09 17:02:33 +0000
+++ lib/lp/bugs/model/bug.py	2010-09-15 10:01:12 +0000
@@ -58,6 +58,7 @@
     SQLRaw,
     Union,
     )
+from storm.info import ClassAlias
 from storm.store import (
     EmptyResultSet,
     Store,
@@ -426,21 +427,79 @@
     @property
     def indexed_messages(self):
         """See `IMessageTarget`."""
+        return self._indexed_messages(content=True)
+
+    def _indexed_messages(self, content=False):
+        """Get the bugs messages, indexed.
+
+        :param content: If True retrieve the content for the msssages too.
+        """
+        # Make all messages be 'in' the main bugtask.
         inside = self.default_bugtask
-        messages = list(self.messages)
-        message_set = set(messages)
-
-        indexed_messages = []
-        for index, message in enumerate(messages):
-            if message.parent not in message_set:
-                parent = None
-            else:
-                parent = message.parent
-
-            indexed_message = IndexedMessage(message, inside, index, parent)
-            indexed_messages.append(indexed_message)
-
-        return indexed_messages
+        store = Store.of(self)
+        message_by_id = {}
+        def eager_load_owners(rows):
+            # Because we may have multiple owners, we spend less time in storm
+            # with very large bugs by not joining and instead querying a second
+            # time. If this starts to show high db time, we can left outer join
+            # instead.
+            owner_ids = set(row[0].ownerID for row in rows)
+            owner_ids.discard(None)
+            if not owner_ids:
+                return
+            list(store.find(Person, Person.id.is_in(owner_ids)))
+        def eager_load_content(rows):
+            # To avoid the complexity of having multiple rows per
+            # message, or joining in the database (though perhaps in
+            # future we should do that), we do a single separate query
+            # for the message content.
+            message_ids = set(row[0].id for row in rows)
+            chunks = store.find(MessageChunk,
+                MessageChunk.messageID.is_in(message_ids))
+            chunks.order_by(MessageChunk.id)
+            chunk_map = {}
+            for chunk in chunks:
+                message_chunks = chunk_map.setdefault(chunk.messageID, [])
+                message_chunks.append(chunk)
+            for row in rows:
+                message = row[0]
+                if message.id not in chunk_map:
+                    continue
+                cache = IPropertyCache(message)
+                cache.text_contents = Message.chunks_text(
+                    chunk_map[message.id])
+        def eager_load(rows, slice_info):
+            eager_load_owners(rows)
+            if content:
+                eager_load_content(rows)
+        def index_message(row, index):
+            # convert row to an IndexedMessage
+            message, parent = row
+            if parent is not None:
+                # If there is an IndexedMessage available as parent, use that
+                # to reduce on-demand parent lookups.
+                parent = message_by_id.get(parent.id, parent)
+            result = IndexedMessage(message, inside, index, parent)
+            # This message may be the parent for another: stash it to permit
+            # use.
+            message_by_id[message.id] = result
+            return result
+        # There is possibly some nicer way to do this in storm, but
+        # this is a lot easier to figure out.
+        # Remaining issue:
+        ParentMessage = ClassAlias(Message, name="parent_message")
+        tables = SQL("Message left outer join message as parent_message on ("
+            "message.parent=parent_message.id and parent_message.id in (select "
+            "bugmessage.message from bugmessage where bugmessage.bug=%s)), "
+            "BugMessage" % sqlvalues(self.id))
+        results = store.using(tables).find(
+            (Message, ParentMessage),
+            BugMessage.bugID == self.id,
+            BugMessage.messageID == Message.id,
+            )
+        results.order_by(Message.datecreated, Message.id)
+        return DecoratedResultSet(results, index_message,
+            pre_iter_hook=eager_load, slice_info=True)
 
     @property
     def displayname(self):
@@ -1785,7 +1844,7 @@
         when attachments is evaluated, not when the resultset is processed).
         """
         message_to_indexed = {}
-        for message in self.indexed_messages:
+        for message in self._indexed_messages():
             message_to_indexed[message.id] = message
         def set_indexed_message(row):
             attachment = row[0]

=== modified file 'lib/lp/bugs/tests/test_bugs_webservice.py'
--- lib/lp/bugs/tests/test_bugs_webservice.py	2010-09-02 19:30:03 +0000
+++ lib/lp/bugs/tests/test_bugs_webservice.py	2010-09-15 10:01:12 +0000
@@ -143,7 +143,7 @@
             rendered_comment, self.expected_comment_html)
 
 
-class TestBugAttachments(TestCaseWithFactory):
+class TestBugScaling(TestCaseWithFactory):
 
     layer = LaunchpadFunctionalLayer
 
@@ -165,7 +165,7 @@
         collector = QueryCollector()
         collector.register()
         self.addCleanup(collector.unregister)
-        url = '/bugs/%d/attachments' % self.bug.id
+        url = '/bugs/%d/attachments?ws.size=75' % self.bug.id
         #First request
         store.flush()
         store.reset()
@@ -183,6 +183,43 @@
         response = webservice.get(url)
         self.assertThat(collector, HasQueryCount(Equals(with_2_count+1)))
 
+    def test_messages_query_counts_constant(self):
+        # XXX Robert Collins 2010-09-15 bug=619017
+        # This test may be thrown off by the reference bug. To get around the
+        # problem, flush and reset are called on the bug storm cache before
+        # each call to the webservice. When lp's storm is updated to release
+        # the committed fix for this bug, please see about updating this test.
+        login(USER_EMAIL)
+        bug = self.factory.makeBug()
+        store = Store.of(bug)
+        self.factory.makeBugComment(bug)
+        self.factory.makeBugComment(bug)
+        self.factory.makeBugComment(bug)
+        person = self.factory.makePerson()
+        webservice = LaunchpadWebServiceCaller(
+            'launchpad-library', 'salgado-change-anything')
+        collector = QueryCollector()
+        collector.register()
+        self.addCleanup(collector.unregister)
+        url = '/bugs/%d/messages?ws.size=75' % bug.id
+        #First request
+        store.flush()
+        store.reset()
+        response = webservice.get(url)
+        self.assertThat(collector, HasQueryCount(LessThan(21)))
+        with_2_count = collector.count
+        self.failUnlessEqual(response.status, 200)
+        login(USER_EMAIL)
+        for i in range(50):
+            self.factory.makeBugComment(bug)
+        self.factory.makeBugAttachment(bug)
+        logout()
+        #Second request
+        store.flush()
+        store.reset()
+        response = webservice.get(url)
+        self.assertThat(collector, HasQueryCount(Equals(with_2_count)))
+
 
 class TestBugMessages(TestCaseWithFactory):
 

=== modified file 'lib/lp/bugs/tests/test_searchtasks_webservice.py'
--- lib/lp/bugs/tests/test_searchtasks_webservice.py	2010-08-20 21:48:35 +0000
+++ lib/lp/bugs/tests/test_searchtasks_webservice.py	2010-09-15 10:01:12 +0000
@@ -5,8 +5,6 @@
 
 __metaclass__ = type
 
-from unittest import TestLoader
-
 from canonical.launchpad.ftests import login
 from lp.testing import TestCaseWithFactory
 from canonical.launchpad.testing.pages import LaunchpadWebServiceCaller
@@ -37,7 +35,3 @@
         response = self.webservice.named_get('/mebuntu/inkanyamba',
             'searchTasks', api_version='devel').jsonBody()
         self.assertEqual(response['total_size'], 1)
-
-
-def test_suite():
-    return TestLoader().loadTestsFromName(__name__)