Merge into devel : branches-timeout-bug-827935 : Code : Launchpad itself

Status:

Merged

Approved by:

Raphaël Badin on 2011-11-01

Approved revision:

no longer in the source branch.

Merged at revision:

14225

Proposed branch:

lp://staging/~rvb/launchpad/branches-timeout-bug-827935

Merge into:

lp://staging/launchpad

Diff against target:

420 lines (+227/-15)

6 files modified

lib/lp/code/browser/branchlisting.py (+83/-4)
lib/lp/code/browser/tests/test_branchlisting.py (+100/-3)
lib/lp/code/interfaces/branchcollection.py (+4/-1)
lib/lp/code/model/branchcollection.py (+10/-6)
lib/lp/code/templates/person-codesummary.pt (+26/-1)
lib/lp/services/features/flags.py (+4/-0)

To merge this branch:

bzr merge lp://staging/~rvb/launchpad/branches-timeout-bug-827935

Related bugs:

Bug #827935: Person:+branches timeouts

Critical

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Graham Binns (community)	code	2011-11-01	Approve on 2011-11-01
Review via email: mp+80875@code.staging.launchpad.net

Commit message

[r=gmb][bug=827935] Add a new simplified menu branch without the counts.

Description of the change

This branch adds a new "simplified" branch menu. The new menu does not include the number of {owned,registered,subscribed} branches which is the main reason why the page https://code.launchpad.net/~ubuntu-branches times out. The display of the new menu is protected by a feature flag so that we will be able to see (with real data) if the performance boost is worth the simplification of the design.

= Details =

The performance for this page was analysed by lifeless, jtv and myself (https://lp-oops.canonical.com/oops.py/?oopsid=OOPS-2124AO73). Obviously, only the teams/persons with a very large number of owned branches is a problem (~ubuntu-branches has ~300k branches). As usual, most of the time is SQL time (~10s out of a total of ~11s for the whole page rendering).

After trying to see if the individual queries could be improved lifeless suggested this —rather dramatic— approach because it was clear that the performance problems were coming from the count queries used to display the number of {owned,registered,subscribed} branches: one of the count queries is taking ~3s and the others ~1s.

= UI =

The new UI will look like this: http://people.canonical.com/~rvb/new_menu_branches.png where the old menu looks like this http://people.canonical.com/~rvb/prev_menu_branches.png.

On the bright side, this will unify the branch menu looks with the bug menu looks (this is how the bug menu looks atm http://people.canonical.com/~rvb/current_bug_menu.png).

= Testing =

Right now, the display of this new menu is protected by a feature flag: code.simplified_branches_menu.enabled. My goal is to:
a) make sure that the performance boost is worth the change
b) ask the list to see if everyone is okay with the change

Also, I've recoded the testing done in lib/lp/code/stories/branches/xx-person-branches.txt so if we decide that this can land for real (i.e. without the FF), this doctest can be removed.

Revision history for this message

Graham Binns (gmb) wrote on 2011-11-01:

#

Hi Raphaël,

Nice branch! I've got a few comments but they're all cosmetic really. Otherwise, r=me.

[1]

62 +
63 + if self.simplified_branch_menu:
64 + return (
65 + self.registered_branch_not_empty or
66 + self.owned_branch_not_empty or
67 + self.subscribed_branch_not_empty or
68 + self.active_review_not_empty
69 + )
70 + else:
71 + return (self.owned_branch_count or
72 + self.registered_branch_count or
73 + self.subscribed_branch_count or
74 + self.active_review_count)

Slightly incosistent formatting here: The form you've used from
64-69 (newline after the opening parenthesis) is the correct one, though
the closing paren should go on the last line as you've done on line 74.

[2]

89 + @cachedproperty
90 + def owned_branch_not_empty(self):
91 + """False if the number of branches owned by self.person is zero."""
92 + return not self._getCountCollection().ownedBy(self.person).is_empty()

I get the feeling this should be named "owned_branches_not_empty" since
you're talking about a collection but the property name suggests you're
talking about a single branch.

[3]

94 + @cachedproperty
95 + def subscribed_branch_not_empty(self):

... And by the same token this should be subscribed_branches_not_empty.

[4]

103 + @cachedproperty
104 + def active_review_not_empty(self):

active_reviews_not_empty

[5]

224 + registered_branches_matcher = soupmatchers.HTMLContains(
225 + soupmatchers.Tag(
226 + 'Registered link', 'a', text='Registered branches',
227 + attrs={'href': 'http://launchpad.dev/~barney'
228 + '/+registeredbranches'}))
229 +

By convention, if you're going to declare a class attribute, you should
do it at the top of the class, since otherwise the reader (i.e. your
humble bearded reviewer) gets a bit confused :).

Hi Raphaël,

Nice branch! I've got a few comments but they're all cosmetic really. Otherwise, r=me.

[1]

62	+
63	+        if self.simplified_branch_menu:
64	+            return (
65	+                self.registered_branch_not_empty or
66	+                self.owned_branch_not_empty or
67	+                self.subscribed_branch_not_empty or
68	+                self.active_review_not_empty
69	+                )
70	+        else:
71	+            return (self.owned_branch_count or
72	+                    self.registered_branch_count or
73	+                    self.subscribed_branch_count or
74	+                    self.active_review_count)

Slightly incosistent formatting here: The form you've used from
64-69 (newline after the opening parenthesis) is the correct one, though
the closing paren should go on the last line as you've done on line 74.

[2]

89	+    @cachedproperty
90	+    def owned_branch_not_empty(self):
91	+        """False if the number of branches owned by self.person is zero."""
92	+        return not self._getCountCollection().ownedBy(self.person).is_empty()

I get the feeling this should be named "owned_branches_not_empty" since
you're talking about a collection but the property name suggests you're
talking about a single branch.

[3]

94	+    @cachedproperty
95	+    def subscribed_branch_not_empty(self):

... And by the same token this should be subscribed_branches_not_empty.

[4]

103	+    @cachedproperty
104	+    def active_review_not_empty(self):

active_reviews_not_empty

[5]

224	+    registered_branches_matcher = soupmatchers.HTMLContains(
225	+        soupmatchers.Tag(
226	+            'Registered link', 'a', text='Registered branches',
227	+            attrs={'href': 'http://launchpad.dev/~barney'
228	+                           '/+registeredbranches'}))
229	+

By convention, if you're going to declare a class attribute, you should
do it at the top of the class, since otherwise the reader (i.e. your
humble bearded reviewer) gets a bit confused :).

review: Approve (code)

Revision history for this message

Raphaël Badin (rvb) wrote on 2011-11-01:

#

> [1]
>
[...]
> Slightly incosistent formatting here: The form you've used from
> 64-69 (newline after the opening parenthesis) is the correct one, though
> the closing paren should go on the last line as you've done on line 74.

Right, I've only written the first part, but I should have harmonized it with the copied code indeed.
Fixed.

> [2]
> [3]
> [4]
> [5]

Right. Done.

> 224 + registered_branches_matcher = soupmatchers.HTMLContains(
> 225 + soupmatchers.Tag(
> 226 + 'Registered link', 'a', text='Registered branches',
> 227 + attrs={'href': 'http://launchpad.dev/~barney'
> 228 + '/+registeredbranches'}))
> 229 +
>
> By convention, if you're going to declare a class attribute, you should
> do it at the top of the class, since otherwise the reader (i.e. your
> humble bearded reviewer) gets a bit confused :).

Okay. I thought it might be a good idea to declare it near where it's used but I suppose it's much better to have a sane default for these.

Thanks for the review Graham!

Launchpad itself

Merge lp://staging/~rvb/launchpad/branches-timeout-bug-827935 into lp://staging/launchpad

Commit message

Description of the change

Preview Diff

Subscribers