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

[Robert Collins]

[r=stub][bug=607935] Reduce overhead when showing only some bug comments.

In this iteration I start doing range requests on BugMessage.index and no longer read in every bug message for every page load.

This required refactoring the stack by which such messages are obtained - we really should ditch BugComment I think and just use BugMessage directly (having it delegate to Message for the message fields). But thats a different project.

Anyhow, there were some properties on the bug task view that required access to all messages to work - e.g. the concept of 'how many bug comments are visible with ?show=all present' requires us to load the text of every bug message because of the algorithm in play. I've taken the view that our users will value speed of page loading over absolute precision and adjusted these to show the total messages in the system, rather than the total that would be shown. This is massively faster.

The largest expected benefit from this branch is less Disk IO - e.g. in bug one, we should skip messages 41->1280 or so. Thats a good 5 seconds of IO when it occurs - and it occurs frequently.

As a drive by I fixed the bug message grouping logic to be stable on bugmessage.index if/when two bug comments have the same datetime - which can happen in tests (there was a fragile test failing 1/3rds of the time for me).

I also fixed the logic for putting 'show all message' fillers in to show them where any hidden message is, though still guarded by the overall check for whether we're bulk-hiding messages. This was needed to avoid going up to two identical queries to get different ranges of comments.

[Stuart Bishop (stub)]

Mainly fine.

 - Remove the commented out eager bugwatch loader if it isn't needed.
 - Add a comment in build_comments_from_chunks() explaining that bugwatches will have been preloaded at this point so the code is not retrieving them one at a time from the db.
 - I see no tests for getMessagesForView's new slice behavior, in particular checking for off-by-one errors.

review: Approve

=== modified file 'lib/lp/bugs/browser/bugcomment.py'
--- lib/lp/bugs/browser/bugcomment.py	2010-12-20 13:39:41 +0000
+++ lib/lp/bugs/browser/bugcomment.py	2011-02-16 20:08:14 +0000
@@ -55,41 +55,33 @@
 COMMENT_ACTIVITY_GROUPING_WINDOW = timedelta(minutes=5)
 
 
-def build_comments_from_chunks(chunks, bugtask, truncate=False):
-    """Build BugComments from MessageChunks."""
+def build_comments_from_chunks(bugtask, truncate=False, slice_info=None):
+    """Build BugComments from MessageChunks.
+    
+    :param truncate: Perform truncation of large messages.
+    :param slice_info: If not None, an iterable of slices to retrieve.
+    """
+    chunks = bugtask.bug.getMessagesForView(slice_info=slice_info)
+    # This would be better as part of indexed_messages eager loading.
     comments = {}
-    index = 0
-    for chunk in chunks:
-        message_id = chunk.message.id
-        bug_comment = comments.get(message_id)
+    for bugmessage, message, chunk in chunks:
+        bug_comment = comments.get(message.id)
         if bug_comment is None:
-            bug_comment = BugComment(
-                index, chunk.message, bugtask)
-            comments[message_id] = bug_comment
-            index += 1
+            bug_comment = BugComment(bugmessage.index, message, bugtask,
+                visible=bugmessage.visible)
+            comments[message.id] = bug_comment
+            # This code path is currently only used from BugTask view which
+            # have already loaded all the bug watches. If we start lazy loading
+            # those, or not needing them we will need to batch lookup watches
+            # here.
+            if bugmessage.bugwatchID is not None:
+                bug_comment.bugwatch = bugmessage.bugwatch
+                bug_comment.synchronized = (
+                    bugmessage.remote_comment_id is not None)
         bug_comment.chunks.append(chunk)
 
-    # Set up the bug watch for all the imported comments. We do it
-    # outside the for loop to avoid issuing one db query per comment.
-    imported_bug_messages = getUtility(IBugMessageSet).getImportedBugMessages(
-        bugtask.bug)
-    for bug_message in imported_bug_messages:
-        message_id = bug_message.message.id
-        comments[message_id].bugwatch = bug_message.bugwatch
-        comments[message_id].synchronized = (
-            bug_message.remote_comment_id is not None)
-
-    for bug_message in bugtask.bug.bug_messages:
-        comment = comments.get(bug_message.messageID, None)
-        # XXX intellectronica 2009-04-22, bug=365092: Currently, there are
-        # some bug messages for which no chunks exist in the DB, so we need to
-        # make sure that we skip them, since the corresponding message wont
-        # have been added to the comments dictionary in the section above.
-        if comment is not None:
-            comment.visible = bug_message.visible
-
     for comment in comments.values():
-        # Once we have all the chunks related to a comment set up,
+        # Once we have all the chunks related to a comment populated,
         # we get the text set up for display.
         comment.setupText(truncate=truncate)
     return comments
@@ -101,20 +93,28 @@
     Generates a stream of comment instances (with the activity grouped within)
     or `list`s of grouped activities.
 
-    :param comments: An iterable of `BugComment` instances.
+    :param comments: An iterable of `BugComment` instances, which should be
+        sorted by index already.
     :param activities: An iterable of `BugActivity` instances.
     """
     window = COMMENT_ACTIVITY_GROUPING_WINDOW
 
     comment_kind = "comment"
+    if comments:
+        max_index = comments[-1].index + 1
+    else:
+        max_index = 0
     comments = (
-        (comment.datecreated, comment.owner, comment_kind, comment)
+        (comment.datecreated, comment.index, comment.owner, comment_kind, comment)
         for comment in comments)
     activity_kind = "activity"
     activity = (
-        (activity.datechanged, activity.person, activity_kind, activity)
+        (activity.datechanged, max_index, activity.person, activity_kind, activity)
         for activity in activities)
-    events = sorted(chain(comments, activity), key=itemgetter(0, 1))
+    # when an action and a comment happen at the same time, the action comes
+    # second, when two events are tied the comment index is used to
+    # disambiguate.
+    events = sorted(chain(comments, activity), key=itemgetter(0, 1, 2))
 
     def gen_event_windows(events):
         """Generate event windows.
@@ -123,12 +123,12 @@
         an integer, and is incremented each time the windowing conditions are
         triggered.
 
-        :param events: An iterable of `(date, actor, kind, event)` tuples in
-            order.
+        :param events: An iterable of `(date, ignored, actor, kind, event)`
+            tuples in order.
         """
         window_comment, window_actor = None, None
         window_index, window_end = 0, None
-        for date, actor, kind, event in events:
+        for date, _, actor, kind, event in events:
             window_ended = (
                 # A window may contain only one comment.
                 (window_comment is not None and kind is comment_kind) or
@@ -174,7 +174,7 @@
     """
     implements(IBugComment)
 
-    def __init__(self, index, message, bugtask, activity=None):
+    def __init__(self, index, message, bugtask, activity=None, visible=True):
         self.index = index
         self.bugtask = bugtask
         self.bugwatch = None
@@ -195,6 +195,7 @@
         self.activity = activity
 
         self.synchronized = False
+        self.visible = visible
 
     @property
     def show_for_admin(self):

=== modified file 'lib/lp/bugs/browser/bugtask.py'
--- lib/lp/bugs/browser/bugtask.py	2011-02-11 18:15:28 +0000
+++ lib/lp/bugs/browser/bugtask.py	2011-02-16 20:08:14 +0000
@@ -293,20 +293,30 @@
     return title.strip()
 
 
-def get_comments_for_bugtask(bugtask, truncate=False):
+def get_comments_for_bugtask(bugtask, truncate=False, for_display=False,
+    slice_info=None):
     """Return BugComments related to a bugtask.
 
     This code builds a sorted list of BugComments in one shot,
     requiring only two database queries. It removes the titles
     for those comments which do not have a "new" subject line
+
+    :param for_display: If true, the zeroth comment is given an empty body so
+        that it will be filtered by get_visible_comments.
+    :param slice_info: If not None, defines a list of slices of the comments to
+        retrieve.
     """
-    chunks = bugtask.bug.getMessageChunks()
-    comments = build_comments_from_chunks(chunks, bugtask, truncate=truncate)
+    comments = build_comments_from_chunks(bugtask, truncate=truncate,
+        slice_info=slice_info)
+    # TODO: further fat can be shaved off here by limiting the attachments we
+    # query to those that slice_info would include.
     for attachment in bugtask.bug.attachments_unpopulated:
         message_id = attachment.message.id
         # All attachments are related to a message, so we can be
         # sure that the BugComment is already created.
-        assert message_id in comments, message_id
+        if message_id not in comments:
+            # We are not showing this message.
+            break
         if attachment.type == BugAttachmentType.PATCH:
             comments[message_id].patches.append(attachment)
         else:
@@ -321,6 +331,12 @@
             # this comment has a new title, so make that the rolling focus
             current_title = comment.title
             comment.display_title = True
+    if for_display and comments and comments[0].index==0:
+        # We show the text of the first comment as the bug description,
+        # or via the special link "View original description", but we want
+        # to display attachments filed together with the bug in the
+        # comment list.
+        comments[0].text_for_display = ''
     return comments
 
 
@@ -344,7 +360,8 @@
 
     # These two lines are here to fill the ValidPersonOrTeamCache cache,
     # so that checking owner.is_valid_person, when rendering the link,
-    # won't issue a DB query.
+    # won't issue a DB query. Note that this should be obsolete now with
+    # getMessagesForView improvements.
     commenters = set(comment.owner for comment in visible_comments)
     getUtility(IPersonSet).getValidPersons(commenters)
 
@@ -686,7 +703,8 @@
         #     This line of code keeps the view's query count down,
         #     possibly using witchcraft. It should be rewritten to be
         #     useful or removed in favour of making other queries more
-        #     efficient.
+        #     efficient. The witchcraft is because the subscribers are accessed
+        #     in the initial page load, so the data is actually used.
         if self.user is not None:
             list(bug.getSubscribersForPerson(self.user))
 
@@ -789,14 +807,8 @@
     @cachedproperty
     def comments(self):
         """Return the bugtask's comments."""
-        comments = get_comments_for_bugtask(self.context, truncate=True)
-        # We show the text of the first comment as the bug description,
-        # or via the special link "View original description", but we want
-        # to display attachments filed together with the bug in the
-        # comment list.
-        comments[0].text_for_display = ''
-        assert len(comments) > 0, "A bug should have at least one comment."
-        return comments
+        return get_comments_for_bugtask(self.context, truncate=True,
+            for_display=True)
 
     @cachedproperty
     def interesting_activity(self):
@@ -834,11 +846,22 @@
                + config.malone.comments_list_truncate_newest_to
                < config.malone.comments_list_max_length)
 
-        recent_comments = self.visible_recent_comments_for_display
-        oldest_comments = self.visible_oldest_comments_for_display
+        if not self.visible_comments_truncated_for_display:
+            comments=self.comments
+        else:
+            # the comment function takes 0-offset counts where comment 0 is
+            # the initial description, so we need to add one to the limits
+            # to adjust.
+            oldest_count = 1 + self.visible_initial_comments
+            new_count = 1 + self.total_comments-self.visible_recent_comments
+            comments = get_comments_for_bugtask(
+                self.context, truncate=True, for_display=True,
+                slice_info=[
+                    slice(None, oldest_count), slice(new_count, None)])
+        visible_comments = get_visible_comments(comments)
 
         event_groups = group_comments_with_activity(
-            comments=chain(oldest_comments, recent_comments),
+            comments=visible_comments,
             activities=self.interesting_activity)
 
         def group_activities_by_target(activities):
@@ -881,77 +904,61 @@
 
         events = map(event_dict, event_groups)
 
-        # Insert blank if we're showing only a subset of the comment list.
-        if len(recent_comments) > 0:
+        # Insert blanks if we're showing only a subset of the comment list.
+        if self.visible_comments_truncated_for_display:
             # Find the oldest recent comment in the event list.
-            oldest_recent_comment = recent_comments[0]
-            for index, event in enumerate(events):
-                if event.get("comment") is oldest_recent_comment:
-                    num_hidden = (
-                        len(self.visible_comments)
-                        - len(oldest_comments)
-                        - len(recent_comments))
+            index = 0
+            prev_comment = None
+            while index < len(events):
+                event = events[index]
+                comment = event.get("comment")
+                if prev_comment is None:
+                    prev_comment = comment
+                    index += 1
+                    continue
+                if comment is None:
+                    index += 1
+                    continue
+                if prev_comment.index + 1 != comment.index:
+                    # There is a gap here, record it.
                     separator = {
-                        'date': oldest_recent_comment.datecreated,
-                        'num_hidden': num_hidden,
+                        'date': prev_comment.datecreated,
+                        'num_hidden': comment.index - prev_comment.index
                         }
                     events.insert(index, separator)
-                    break
-
+                    index += 1
+                prev_comment = comment
+                index += 1
         return events
 
-    @cachedproperty
-    def visible_comments(self):
-        """All visible comments.
-
-        See `get_visible_comments` for the definition of a "visible"
-        comment.
-        """
-        return get_visible_comments(self.comments)
-
-    @cachedproperty
-    def visible_oldest_comments_for_display(self):
-        """The list of oldest visible comments to be rendered.
-
-        This considers truncating the comment list if there are tons
-        of comments, but also obeys any explicitly requested ways to
-        display comments (currently only "all" is recognised).
-        """
-        show_all = (self.request.form_ng.getOne('comments') == 'all')
-        max_comments = config.malone.comments_list_max_length
-        if show_all or len(self.visible_comments) <= max_comments:
-            return self.visible_comments
-        else:
-            oldest_count = config.malone.comments_list_truncate_oldest_to
-            return self.visible_comments[:oldest_count]
-
-    @cachedproperty
-    def visible_recent_comments_for_display(self):
-        """The list of recent visible comments to be rendered.
-
-        If the number of comments is beyond the maximum threshold, this
-        returns the most recent few comments. If we're under the threshold,
-        then visible_oldest_comments_for_display will be returning the bugs,
-        so this routine will return an empty set to avoid duplication.
-        """
-        show_all = (self.request.form_ng.getOne('comments') == 'all')
-        max_comments = config.malone.comments_list_max_length
-        total = len(self.visible_comments)
-        if show_all or total <= max_comments:
-            return []
-        else:
-            start = total - config.malone.comments_list_truncate_newest_to
-            return self.visible_comments[start:total]
-
-    @property
+    @property
+    def visible_initial_comments(self):
+        """How many initial comments are being shown."""
+        return config.malone.comments_list_truncate_oldest_to
+
+    @property
+    def visible_recent_comments(self):
+        """How many recent comments are being shown."""
+        return config.malone.comments_list_truncate_newest_to
+
+    @cachedproperty
     def visible_comments_truncated_for_display(self):
         """Whether the visible comment list is truncated for display."""
-        return (len(self.visible_comments) >
-                len(self.visible_oldest_comments_for_display))
+        show_all = (self.request.form_ng.getOne('comments') == 'all')
+        if show_all:
+            return False
+        max_comments = config.malone.comments_list_max_length
+        return self.total_comments > max_comments
+
+    @cachedproperty
+    def total_comments(self):
+        """We count all comments because the db cannot do visibility yet."""
+        return self.context.bug.bug_messages.count() - 1
 
     def wasDescriptionModified(self):
         """Return a boolean indicating whether the description was modified"""
-        return self.comments[0].text_contents != self.context.bug.description
+        return (self.context.bug.indexed_messages[0].text_contents !=
+            self.context.bug.description)
 
     @cachedproperty
     def linked_branches(self):

=== modified file 'lib/lp/bugs/browser/tests/bug-views.txt'
--- lib/lp/bugs/browser/tests/bug-views.txt	2010-12-21 20:57:11 +0000
+++ lib/lp/bugs/browser/tests/bug-views.txt	2011-02-16 20:08:14 +0000
@@ -206,16 +206,17 @@
     True
 
 The displayable comments for a bug can be obtained from the view
-property visible_oldest_comments_for_display.
+property activity_and_comments.
 
-    >>> viewable_comments = ubuntu_bugview.visible_oldest_comments_for_display
+    >>> comments = [event.get('comment') for event in 
+    ...     ubuntu_bugview.activity_and_comments if event.get('comment')]
 
 Because we omit the first comment, and because the third comment is
 indentical to the second, we really only display one comment:
 
-    >>> print len(viewable_comments)
+    >>> print len(comments)
     1
-    >>> [(c.index, c.owner.name, c.text_contents) for c in viewable_comments]
+    >>> [(c.index, c.owner.name, c.text_contents) for c in comments]
     [(1, u'name16', u'I can reproduce this bug.')]
 
 (Unregister our listener, since we no longer need it.)

=== modified file 'lib/lp/bugs/browser/tests/test_bugcomment.py'
--- lib/lp/bugs/browser/tests/test_bugcomment.py	2010-12-21 15:31:17 +0000
+++ lib/lp/bugs/browser/tests/test_bugcomment.py	2011-02-16 20:08:14 +0000
@@ -19,11 +19,11 @@
 
 class BugActivityStub:
 
-    def __init__(self, datechanged, person=None):
+    def __init__(self, datechanged, owner=None):
         self.datechanged = datechanged
-        if person is None:
-            person = PersonStub()
-        self.person = person
+        if owner is None:
+            owner = PersonStub()
+        self.person = owner
 
     def __repr__(self):
         return "BugActivityStub(%r, %r)" % (
@@ -32,16 +32,18 @@
 
 class BugCommentStub:
 
-    def __init__(self, datecreated, owner=None):
+    def __init__(self, datecreated, index, owner=None):
         self.datecreated = datecreated
         if owner is None:
             owner = PersonStub()
         self.owner = owner
         self.activity = []
+        self.index = index
 
     def __repr__(self):
-        return "BugCommentStub(%r, %r)" % (
-            self.datecreated.strftime('%Y-%m-%d--%H%M'), self.owner)
+        return "BugCommentStub(%r, %d, %r)" % (
+            self.datecreated.strftime('%Y-%m-%d--%H%M'), 
+            self.index, self.owner)
 
 
 class PersonStub:
@@ -61,8 +63,8 @@
     def setUp(self):
         super(TestGroupCommentsWithActivities, self).setUp()
         self.now = datetime.now(utc)
-        self.timestamps = (
-            self.now + timedelta(minutes=counter)
+        self.time_index = (
+            (self.now + timedelta(minutes=counter), counter)
             for counter in count(1))
 
     def group(self, comments, activities):
@@ -79,7 +81,7 @@
         # When no activities are passed in, and the comments passed in don't
         # have any common actors, no grouping is possible.
         comments = [
-            BugCommentStub(next(self.timestamps))
+            BugCommentStub(*next(self.time_index))
             for number in xrange(5)]
         self.assertEqual(
             comments, self.group(comments=comments, activities=[]))
@@ -88,7 +90,7 @@
         # When no comments are passed in, and the activities passed in don't
         # have any common actors, no grouping is possible.
         activities = [
-            BugActivityStub(next(self.timestamps))
+            BugActivityStub(next(self.time_index)[0])
             for number in xrange(5)]
         self.assertEqual(
             [[activity] for activity in activities], self.group(
@@ -97,13 +99,13 @@
     def test_no_common_actor(self):
         # When each activities and comment given has a different actor then no
         # grouping is possible.
-        activity1 = BugActivityStub(next(self.timestamps))
-        comment1 = BugCommentStub(next(self.timestamps))
-        activity2 = BugActivityStub(next(self.timestamps))
-        comment2 = BugCommentStub(next(self.timestamps))
+        activity1 = BugActivityStub(next(self.time_index)[0])
+        comment1 = BugCommentStub(*next(self.time_index))
+        activity2 = BugActivityStub(next(self.time_index)[0])
+        comment2 = BugCommentStub(*next(self.time_index))
 
         activities = set([activity1, activity2])
-        comments = set([comment1, comment2])
+        comments = list([comment1, comment2])
 
         self.assertEqual(
             [[activity1], comment1, [activity2], comment2],
@@ -113,8 +115,8 @@
         # An activity shortly after a comment by the same person is grouped
         # into the comment.
         actor = PersonStub()
-        comment = BugCommentStub(next(self.timestamps), actor)
-        activity = BugActivityStub(next(self.timestamps), actor)
+        comment = BugCommentStub(*next(self.time_index), owner=actor)
+        activity = BugActivityStub(next(self.time_index)[0], owner=actor)
         grouped = self.group(comments=[comment], activities=[activity])
         self.assertEqual([comment], grouped)
         self.assertEqual([activity], comment.activity)
@@ -123,8 +125,8 @@
         # An activity shortly before a comment by the same person is grouped
         # into the comment.
         actor = PersonStub()
-        activity = BugActivityStub(next(self.timestamps), actor)
-        comment = BugCommentStub(next(self.timestamps), actor)
+        activity = BugActivityStub(next(self.time_index)[0], owner=actor)
+        comment = BugCommentStub(*next(self.time_index), owner=actor)
         grouped = self.group(comments=[comment], activities=[activity])
         self.assertEqual([comment], grouped)
         self.assertEqual([activity], comment.activity)
@@ -133,9 +135,9 @@
         # Activities shortly before and after a comment are grouped into the
         # comment's activity.
         actor = PersonStub()
-        activity1 = BugActivityStub(next(self.timestamps), actor)
-        comment = BugCommentStub(next(self.timestamps), actor)
-        activity2 = BugActivityStub(next(self.timestamps), actor)
+        activity1 = BugActivityStub(next(self.time_index)[0], owner=actor)
+        comment = BugCommentStub(*next(self.time_index), owner=actor)
+        activity2 = BugActivityStub(next(self.time_index)[0], owner=actor)
         grouped = self.group(
             comments=[comment], activities=[activity1, activity2])
         self.assertEqual([comment], grouped)
@@ -146,7 +148,7 @@
         # Anything outside of that window is considered separate.
         actor = PersonStub()
         activities = [
-            BugActivityStub(next(self.timestamps), actor)
+            BugActivityStub(next(self.time_index)[0], owner=actor)
             for count in xrange(8)]
         grouped = self.group(comments=[], activities=activities)
         self.assertEqual(2, len(grouped))
@@ -156,8 +158,8 @@
     def test_two_comments_by_common_actor(self):
         # Only one comment will ever appear in a group.
         actor = PersonStub()
-        comment1 = BugCommentStub(next(self.timestamps), actor)
-        comment2 = BugCommentStub(next(self.timestamps), actor)
+        comment1 = BugCommentStub(*next(self.time_index), owner=actor)
+        comment2 = BugCommentStub(*next(self.time_index), owner=actor)
         grouped = self.group(comments=[comment1, comment2], activities=[])
         self.assertEqual([comment1, comment2], grouped)
 
@@ -165,11 +167,11 @@
         # Activity gets associated with earlier comment when all other factors
         # are unchanging.
         actor = PersonStub()
-        activity1 = BugActivityStub(next(self.timestamps), actor)
-        comment1 = BugCommentStub(next(self.timestamps), actor)
-        activity2 = BugActivityStub(next(self.timestamps), actor)
-        comment2 = BugCommentStub(next(self.timestamps), actor)
-        activity3 = BugActivityStub(next(self.timestamps), actor)
+        activity1 = BugActivityStub(next(self.time_index)[0], owner=actor)
+        comment1 = BugCommentStub(*next(self.time_index), owner=actor)
+        activity2 = BugActivityStub(next(self.time_index)[0], owner=actor)
+        comment2 = BugCommentStub(*next(self.time_index), owner=actor)
+        activity3 = BugActivityStub(next(self.time_index)[0], owner=actor)
         grouped = self.group(
             comments=[comment1, comment2],
             activities=[activity1, activity2, activity3])

=== modified file 'lib/lp/bugs/configure.zcml'
--- lib/lp/bugs/configure.zcml	2011-02-14 00:15:22 +0000
+++ lib/lp/bugs/configure.zcml	2011-02-16 20:08:14 +0000
@@ -734,7 +734,7 @@
                     hasBranch
                     security_related
                     tags
-                    getMessageChunks
+                    getMessagesForView
                     isSubscribedToDupes
                     getSubscribersFromDuplicates
                     getSubscriptionsFromDuplicates

=== modified file 'lib/lp/bugs/doc/bug.txt'
--- lib/lp/bugs/doc/bug.txt	2011-02-02 09:55:25 +0000
+++ lib/lp/bugs/doc/bug.txt	2011-02-16 20:08:14 +0000
@@ -1116,15 +1116,15 @@
 ------------
 
 A bug comment is actually made up of a number of chunks. The
-IBug.getMessageChunks() method allows you to retreive these chunks in a
-single shot.
+IBug.getMessagesForView() method allows you to get all the data needed to
+show messages in the bugtask index template in one shot.
 
     >>> from canonical.ftests.pgsql import CursorWrapper
     >>> CursorWrapper.record_sql = True
     >>> queries = len(CursorWrapper.last_executed_sql)
 
-    >>> chunks = bug_two.getMessageChunks()
-    >>> for chunk in sorted(chunks, key=lambda x:x.id):
+    >>> chunks = bug_two.getMessagesForView(None)
+    >>> for _, _1, chunk in sorted(chunks, key=lambda x:x[2].id):
     ...    (chunk.id, chunk.message.id, chunk.message.owner.id,
     ...     chunk.content[:30])
     (4, 1, 16, u'Problem exists between chair a')
@@ -1141,6 +1141,24 @@
     >>> len(CursorWrapper.last_executed_sql) - queries <= 3
     True
 
+getMessagesForView supports slicing operations:
+
+    >>> def message_ids(slices):
+    ...     chunks = bug_two.getMessagesForView(slices)
+    ...     return sorted(set(
+    ...         bugmessage.index for bugmessage, _, _1 in chunks))
+    >>> message_ids([slice(1, 2)])
+    [1]
+
+We use this to get the first N and last M messages in big bugs:
+    >>> message_ids([slice(None, 1), slice(2, None)])
+    [0, 2]
+
+We also support a negative lookup though the bug view does not use that at the
+moment:
+    >>> message_ids([slice(None, 1), slice(-1, None)])
+    [0, 2]
+
 Bugs have a special attribute, `indexed_messages` which returns the collection
 of messages, each decorated with the index of that message in its context
 (the bug) and the primary bug task. This is used for providing an efficient

=== modified file 'lib/lp/bugs/doc/bugcomment.txt'
--- lib/lp/bugs/doc/bugcomment.txt	2010-12-21 15:07:26 +0000
+++ lib/lp/bugs/doc/bugcomment.txt	2011-02-16 20:08:14 +0000
@@ -14,7 +14,7 @@
 The bug's description starts out identical to its first comment. In the course
 of a bug's life, the description may be updated, but the first comment stays
 intact. To improve readability, we never display the first comment in the bug
-page, and this is why visible_oldest_comments_for_display doesn't include it:
+page, and this is why the event stream elides it doesn't include it:
 
     >>> bug_ten = getUtility(IBugSet).get(10)
     >>> bug_ten_bugtask = bug_ten.bugtasks[0]
@@ -22,8 +22,10 @@
     >>> bug_view = getMultiAdapter(
     ...     (bug_ten_bugtask, LaunchpadTestRequest()), name='+index')
     >>> bug_view.initialize()
-    >>> rendered_comments = bug_view.visible_oldest_comments_for_display
-    >>> [bug_comment.index for bug_comment in rendered_comments]
+    >>> def bug_comments(bug_view):
+    ...     return [event.get('comment') for event in 
+    ...         bug_view.activity_and_comments if event.get('comment')]
+    >>> [bug_comment.index for bug_comment in bug_comments(bug_view)]
     [1]
 
 In the case of bug 10, the first comment is identical to the bug's
@@ -42,11 +44,10 @@
 stored as bug attchments of the first comment. Similary, the first
 comment of bugs imported from other bug trackers may have attachments.
 We display these attachments in the comment section of the Web UI,
-hence visible_oldest_comments_for_display contains the first comment, if
-it has attachments.
+hence the activity stream contains the first comment, if it has attachments.
 
 Currently, the first comment of bug 11 has no attachments, hence
-BugTaskView.visible_oldest_comments_for_display does not return the
+BugTaskView.activity_and_comments does not return the
 first comment.
 
     >>> bug_11 = getUtility(IBugSet).get(11)
@@ -54,12 +55,12 @@
     >>> bug_11_view = getMultiAdapter(
     ...     (bug_11_bugtask, LaunchpadTestRequest()), name='+index')
     >>> bug_11_view.initialize()
-    >>> rendered_comments = bug_11_view.visible_oldest_comments_for_display
+    >>> rendered_comments = bug_comments(bug_11_view)
     >>> [bug_comment.index for bug_comment in rendered_comments]
     [1, 2, 3, 4, 5, 6]
 
 If we add an attachment to the first comment, this comment is included
-in visible_oldest_comments_for_display...
+in activity_and_comments...
 
     >>> import StringIO
     >>> login("[e-mail]")
@@ -71,7 +72,7 @@
     >>> bug_11_view = getMultiAdapter(
     ...     (bug_11_bugtask, LaunchpadTestRequest()), name='+index')
     >>> bug_11_view.initialize()
-    >>> rendered_comments = bug_11_view.visible_oldest_comments_for_display
+    >>> rendered_comments = bug_comments(bug_11_view)
     >>> [bug_comment.index for bug_comment in rendered_comments]
     [0, 1, 2, 3, 4, 5, 6]
     >>>
@@ -112,13 +113,13 @@
 comments have been truncated:
 
     >>> [(bug_comment.index, bug_comment.was_truncated)
-    ...  for bug_comment in bug_view.visible_oldest_comments_for_display]
+    ...  for bug_comment in bug_comments(bug_view)]
     [(1, True), (2, True)]
 
 Let's take a closer look at one of the truncated comments. We can
 display the truncated text using text_for_display:
 
-    >>> comment_one = bug_view.visible_oldest_comments_for_display[0]
+    >>> comment_one = bug_comments(bug_view)[0]
     >>> print comment_one.text_for_display #doctest: -ELLIPSIS
     This would be a real...
 
@@ -222,7 +223,7 @@
     ...     (bug_three_bugtask, LaunchpadTestRequest()), name='+index')
     >>> bug_view.initialize()
     >>> [(c.index, c.title, c.text_for_display)
-    ...  for c in bug_view.visible_oldest_comments_for_display]
+    ...  for c in bug_comments(bug_view)]
     [(1, u'Hi', u'Hello there'),
      (3, u'Ho', u'Hello there'),
      (5, u'Ho', u'Hello there'),
@@ -236,9 +237,8 @@
 comments: visible_comments_truncated_for_display.
 
 This is normally false, but for bugs with lots of comments, the
-visible_comments_truncated_for_display property becomes True and the
-visible_oldest_comments_for_display list is truncated to just the oldest
-comments.
+visible_comments_truncated_for_display property becomes True and the activity
+stream has the middle comments elided.
 
 The configuration keys comments_list_max_length,
 comments_list_truncate_oldest_to, and comments_list_truncate_newest_to
@@ -272,24 +272,14 @@
     >>> bug = factory.makeBug()
     >>> add_comments(bug, 9)
 
-If we create a view for this, we can see that all 9 comments are
-visible, and we can see that the list has not been truncated.
+If we create a view for this, we can see that truncation is disabled.
 
     >>> bug_view = getMultiAdapter(
     ...     (bug.default_bugtask, LaunchpadTestRequest()), name='+index')
     >>> bug_view.initialize()
-
-    >>> len(bug_view.visible_oldest_comments_for_display)
-    9
     >>> bug_view.visible_comments_truncated_for_display
     False
 
-When comments aren't being truncated and empty set will be returned by
-visible_recent_comments_for_display.
-
-    >>> len(bug_view.visible_recent_comments_for_display)
-    0
-
 Add two more comments, and the list will be truncated to only 8 total.
 
     >>> add_comments(bug, 2)
@@ -300,9 +290,9 @@
 
     >>> bug_view.visible_comments_truncated_for_display
     True
-    >>> len(bug_view.visible_oldest_comments_for_display)
+    >>> bug_view.visible_initial_comments
     3
-    >>> len(bug_view.visible_recent_comments_for_display)
+    >>> bug_view.visible_recent_comments
     5
 
 The display of all comments can be requested with a form parameter.
@@ -312,8 +302,6 @@
     ...     (bug.default_bugtask, request), name='+index')
     >>> bug_view.initialize()
 
-    >>> len(bug_view.visible_oldest_comments_for_display)
-    11
     >>> bug_view.visible_comments_truncated_for_display
     False
 

=== modified file 'lib/lp/bugs/interfaces/bug.py'
--- lib/lp/bugs/interfaces/bug.py	2011-02-16 13:42:51 +0000
+++ lib/lp/bugs/interfaces/bug.py	2011-02-16 20:08:14 +0000
@@ -695,8 +695,15 @@
                 remote bug tracker, if it's an imported comment.
         """
 
-    def getMessageChunks():
-        """Return MessageChunks corresponding to comments made on this bug"""
+    def getMessagesForView(slice_info):
+        """Return BugMessage,Message,MessageChunks for renderinger.
+        
+        This eager loads message.owner validity associated with the
+        bugmessages.
+
+        :param slice_info: Either None or a list of slices to constraint the
+            returned rows. The step parameter in each slice is ignored.
+        """
 
     def getNullBugTask(product=None, productseries=None,
                     sourcepackagename=None, distribution=None,

=== modified file 'lib/lp/bugs/interfaces/bugmessage.py'
--- lib/lp/bugs/interfaces/bugmessage.py	2011-01-31 08:08:04 +0000
+++ lib/lp/bugs/interfaces/bugmessage.py	2011-02-16 20:08:14 +0000
@@ -51,6 +51,7 @@
     message = Object(schema=IMessage, title=u"The message.")
     bugwatch = Object(schema=IBugWatch,
         title=u"A bugwatch to which the message pertains.")
+    bugwatchID = Int(title=u'The bugwatch id.', readonly=True)
     remote_comment_id = TextLine(
         title=u"The id this comment has in the bugwatch's bug tracker.")
     visible = Bool(title=u"This message is visible or not.", required=False,

=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py	2011-02-14 00:15:22 +0000
+++ lib/lp/bugs/model/bug.py	2011-02-16 20:08:14 +0000
@@ -437,6 +437,8 @@
     @property
     def indexed_messages(self):
         """See `IMessageTarget`."""
+        # Note that this is a decorated result set, so will cache its value (in
+        # the absence of slices)
         return self._indexed_messages(include_content=True)
 
     def _indexed_messages(self, include_content=False, include_parents=True):
@@ -1369,39 +1371,55 @@
         """See `IBug`."""
         return self._question_from_bug
 
-    def getMessageChunks(self):
+    def getMessagesForView(self, slice_info):
         """See `IBug`."""
-        query = """
-            Message.id = MessageChunk.message AND
-            BugMessage.message = Message.id AND
-            BugMessage.bug = %s
-            """ % sqlvalues(self)
-
-        chunks = MessageChunk.select(query,
-            clauseTables=["BugMessage", "Message"],
-            # XXX: kiko 2006-09-16 bug=60745:
-            # There is an issue that presents itself
-            # here if we prejoin message.owner: because Message is
-            # already in the clauseTables, the SQL generated joins
-            # against message twice and that causes the results to
-            # break.
-            prejoinClauseTables=["Message"],
-            # Note the ordering by Message.id here; while datecreated in
-            # production is never the same, it can be in the test suite.
-            orderBy=["Message.datecreated", "Message.id",
-                     "MessageChunk.sequence"])
-        chunks = list(chunks)
-
-        # Since we can't prejoin, cache all people at once so we don't
-        # have to do it while rendering, which is a big deal for bugs
-        # with a million comments.
-        owner_ids = set()
-        for chunk in chunks:
-            if chunk.message.ownerID:
-                owner_ids.add(str(chunk.message.ownerID))
-        list(Person.select("ID in (%s)" % ",".join(owner_ids)))
-
-        return chunks
+        # Note that this function and indexed_messages have significant overlap
+        # and could stand to be refactored.
+        slices = []
+        if slice_info is not None:
+            # NB: This isn't a full implementation of the slice protocol,
+            # merely the bits needed by BugTask:+index.
+            for slice in slice_info:
+                if not slice.start:
+                    assert slice.stop > 0, slice.stop
+                    slices.append(BugMessage.index < slice.stop)
+                elif not slice.stop:
+                    if slice.start < 0:
+                        # If the high index is N, a slice of -1: should
+                        # return index N - so we need to add one to the
+                        # range.
+                        slices.append(BugMessage.index >= SQL(
+                            "(select max(index) from "
+                            "bugmessage where bug=%s) + 1 - %s" % (
+                            sqlvalues(self.id, -slice.start))))
+                    else:
+                        slices.append(BugMessage.index >= slice.start)
+                else:
+                    slices.append(And(BugMessage.index >= slice.start,
+                        BugMessage.index < slice.stop))
+        if slices:
+            ranges = [Or(*slices)]
+        else:
+            ranges = []
+        # We expect:
+        # 1 bugmessage -> 1 message -> small N chunks. For now, using a wide
+        # query seems fine as we have to join out from bugmessage anyway.
+        result = Store.of(self).find((BugMessage, Message, MessageChunk),
+            Message.id==MessageChunk.messageID,
+            BugMessage.messageID==Message.id,
+            BugMessage.bug==self.id,
+            *ranges)
+        result.order_by(BugMessage.index, MessageChunk.sequence)
+        def eager_load_owners(rows):
+            owners = set()
+            for row in rows:
+                owners.add(row[1].ownerID)
+            owners.discard(None)
+            if not owners:
+                return
+            PersonSet().getPrecachedPersonsFromIDs(owners,
+                need_validity=True)
+        return DecoratedResultSet(result, pre_iter_hook=eager_load_owners)
 
     def getNullBugTask(self, product=None, productseries=None,
                     sourcepackagename=None, distribution=None,

=== modified file 'lib/lp/bugs/templates/bugcomment-macros.pt'
--- lib/lp/bugs/templates/bugcomment-macros.pt	2010-07-01 21:53:22 +0000
+++ lib/lp/bugs/templates/bugcomment-macros.pt	2011-02-16 20:08:14 +0000
@@ -66,7 +66,7 @@
                class="sprite retry"
                style="white-space: nowrap">
                view all <span
-               tal:replace="view/visible_comments/count:len"
+               tal:replace="view/total_comments"
                /> comments</a>
           </td>
         </tr>

=== modified file 'lib/lp/bugs/templates/bugtask-index.pt'
--- lib/lp/bugs/templates/bugtask-index.pt	2011-02-15 18:16:28 +0000
+++ lib/lp/bugs/templates/bugtask-index.pt	2011-02-16 20:08:14 +0000
@@ -320,16 +320,16 @@
             tal:condition="view/visible_comments_truncated_for_display">
           <div class="informational message">
             Displaying first <span
-            tal:replace="view/visible_oldest_comments_for_display/count:len">23</span>
+            tal:replace="view/visible_initial_comments">23</span>
             and last <span
-            tal:replace="view/visible_recent_comments_for_display/count:len">32</span>
+            tal:replace="view/visible_recent_comments">32</span>
             comments.
             <tal:what-next
                 define="view_all_href
                         string:${context/fmt:url}?comments=all">
               <a href="#" tal:attributes="href view_all_href">
                 View all <span
-                tal:replace="view/visible_comments/count:len" />
+                tal:replace="view/total_comments" />
                 comments</a> or <a href="#" tal:attributes="href
                 view_all_href">add a comment</a>.
             </tal:what-next>