launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #05352
[Merge] lp:~rvb/launchpad/branches-timeout-bug-827935 into lp:launchpad
Raphaël Badin has proposed merging lp:~rvb/launchpad/branches-timeout-bug-827935 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #827935 in Launchpad itself: "Person:+branches timeouts"
https://bugs.launchpad.net/launchpad/+bug/827935
For more details, see:
https://code.launchpad.net/~rvb/launchpad/branches-timeout-bug-827935/+merge/80875
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.
--
https://code.launchpad.net/~rvb/launchpad/branches-timeout-bug-827935/+merge/80875
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/launchpad/branches-timeout-bug-827935 into lp:launchpad.
=== modified file 'lib/lp/code/browser/branchlisting.py'
--- lib/lp/code/browser/branchlisting.py 2011-10-14 02:12:06 +0000
+++ lib/lp/code/browser/branchlisting.py 2011-11-01 08:51:16 +0000
@@ -31,6 +31,7 @@
]
from operator import attrgetter
+import urlparse
from lazr.delegates import delegates
from lazr.enum import (
@@ -41,7 +42,6 @@
Asc,
Desc,
)
-import urlparse
from z3c.ptcompat import ViewPageTemplateFile
from zope.component import getUtility
from zope.formlib import form
@@ -135,6 +135,7 @@
from lp.registry.interfaces.sourcepackage import ISourcePackageFactory
from lp.registry.model.sourcepackage import SourcePackage
from lp.services.browser_helpers import get_plural_text
+from lp.services.features import getFeatureFlag
from lp.services.propertycache import cachedproperty
@@ -850,7 +851,9 @@
usedfor = IPerson
facet = 'branches'
links = ['registered', 'owned', 'subscribed', 'addbranch',
- 'active_reviews', 'mergequeues']
+ 'active_reviews', 'mergequeues',
+ 'simplified_subscribed', 'simplified_registered',
+ 'simplified_owned', 'simplified_active_reviews']
extra_attributes = [
'active_review_count',
'owned_branch_count',
@@ -858,6 +861,7 @@
'show_summary',
'subscribed_branch_count',
'mergequeue_count',
+ 'simplified_branch_menu',
]
def _getCountCollection(self):
@@ -881,13 +885,86 @@
"""
return self.context
+ @cachedproperty
+ def has_branches(self):
+ """Should the template show the summary view with the links."""
+
@property
def show_summary(self):
"""Should the template show the summary view with the links."""
- return (self.owned_branch_count or
- self.registered_branch_count or
- self.subscribed_branch_count or
- self.active_review_count)
+
+ if self.simplified_branch_menu:
+ return (
+ self.registered_branch_not_empty or
+ self.owned_branch_not_empty or
+ self.subscribed_branch_not_empty or
+ self.active_review_not_empty
+ )
+ else:
+ return (self.owned_branch_count or
+ self.registered_branch_count or
+ self.subscribed_branch_count or
+ self.active_review_count)
+
+ @cachedproperty
+ def simplified_branch_menu(self):
+ return getFeatureFlag('code.simplified_branches_menu.enabled')
+
+ @cachedproperty
+ def registered_branch_not_empty(self):
+ """False if the number of branches registered by self.person
+ is zero.
+ """
+ return (
+ not self._getCountCollection().registeredBy(
+ self.person).is_empty())
+
+ @cachedproperty
+ def owned_branch_not_empty(self):
+ """False if the number of branches owned by self.person is zero."""
+ return not self._getCountCollection().ownedBy(self.person).is_empty()
+
+ @cachedproperty
+ def subscribed_branch_not_empty(self):
+ """False if the number of branches subscribed to by self.person
+ is zero.
+ """
+ return (
+ not self._getCountCollection().subscribedBy(
+ self.person).is_empty())
+
+ @cachedproperty
+ def active_review_not_empty(self):
+ """Return the number of active reviews for self.person's branches."""
+ active_reviews = PersonActiveReviewsView(self.context, self.request)
+ return not active_reviews.getProposals().is_empty()
+
+ def simplified_owned(self):
+ return Link(
+ canonical_url(self.context, rootsite='code'),
+ 'Owned branches',
+ enabled=self.owned_branch_not_empty)
+
+ def simplified_registered(self):
+ person_is_individual = (not self.person.is_team)
+ return Link(
+ '+registeredbranches',
+ 'Registered branches',
+ enabled=(
+ person_is_individual and
+ self.registered_branch_not_empty))
+
+ def simplified_subscribed(self):
+ return Link(
+ '+subscribedbranches',
+ 'Subscribed branches',
+ enabled=self.subscribed_branch_not_empty)
+
+ def simplified_active_reviews(self):
+ return Link(
+ '+activereviews',
+ 'Active reviews',
+ enabled=self.active_review_not_empty)
@cachedproperty
def registered_branch_count(self):
=== modified file 'lib/lp/code/browser/tests/test_branchlisting.py'
--- lib/lp/code/browser/tests/test_branchlisting.py 2011-10-14 02:12:06 +0000
+++ lib/lp/code/browser/tests/test_branchlisting.py 2011-11-01 08:51:16 +0000
@@ -11,18 +11,22 @@
import re
from lazr.uri import URI
+import soupmatchers
from storm.expr import (
Asc,
Desc,
)
+from testtools.matchers import Not
from zope.component import getUtility
from canonical.launchpad.testing.pages import (
extract_text,
+ find_main_content,
find_tag_by_id,
- find_main_content)
+ )
from canonical.launchpad.webapp import canonical_url
from canonical.launchpad.webapp.servers import LaunchpadTestRequest
+from canonical.testing import LaunchpadFunctionalLayer
from canonical.testing.layers import DatabaseFunctionalLayer
from lp.code.browser.branchlisting import (
BranchListingSort,
@@ -31,7 +35,10 @@
PersonProductSubscribedBranchesView,
SourcePackageBranchesView,
)
-from lp.code.enums import BranchVisibilityRule
+from lp.code.enums import (
+ BranchMergeProposalStatus,
+ BranchVisibilityRule,
+ )
from lp.code.model.branch import Branch
from lp.code.model.seriessourcepackagebranch import (
SeriesSourcePackageBranchSet,
@@ -59,8 +66,8 @@
COMMERCIAL_ADMIN_EMAIL,
)
from lp.testing.views import (
+ create_initialized_view,
create_view,
- create_initialized_view,
)
@@ -257,6 +264,97 @@
self._test_batch_template(self.barney)
+SIMPLIFIED_BRANCHES_MENU_FLAG = {
+ 'code.simplified_branches_menu.enabled': 'on'}
+
+
+class TestSimplifiedPersonOwnedBranchesView(TestCaseWithFactory):
+
+ layer = LaunchpadFunctionalLayer
+
+ def setUp(self):
+ TestCaseWithFactory.setUp(self)
+ self.user = self.factory.makePerson()
+
+ self.person = self.factory.makePerson(name='barney')
+ self.team = self.factory.makeTeam(owner=self.person)
+ self.product = self.factory.makeProduct(name='bambam')
+
+ def get_branch_list_page(self, page_name='+branches', user=None):
+ if user is None:
+ user = self.person
+ with FeatureFixture(SIMPLIFIED_BRANCHES_MENU_FLAG):
+ with person_logged_in(self.user):
+ return create_initialized_view(
+ user, page_name, rootsite='code',
+ principal=self.user)()
+
+ def test_branch_list_h1(self):
+ page = self.get_branch_list_page()
+ h1_matcher = soupmatchers.HTMLContains(
+ soupmatchers.Tag(
+ 'Title', 'h1', text='Bazaar branches owned by Barney'))
+ self.assertThat(page, h1_matcher)
+
+ registered_branches_matcher = soupmatchers.HTMLContains(
+ soupmatchers.Tag(
+ 'Registered link', 'a', text='Registered branches',
+ attrs={'href': 'http://launchpad.dev/~barney'
+ '/+registeredbranches'}))
+
+ def test_branch_list_empty(self):
+ page = self.get_branch_list_page()
+ empty_message_matcher = soupmatchers.HTMLContains(
+ soupmatchers.Tag(
+ 'Empty message', 'p',
+ text='There are no branches related to Barney '
+ 'in Launchpad today.'))
+ self.assertThat(page, empty_message_matcher)
+ self.assertThat(page, Not(self.registered_branches_matcher))
+
+ def test_branch_list_registered_link(self):
+ self.factory.makeAnyBranch(owner=self.person)
+ page = self.get_branch_list_page()
+ self.assertThat(page, self.registered_branches_matcher)
+
+ def test_branch_list_owned_link(self):
+ owned_branches_matcher = soupmatchers.HTMLContains(
+ soupmatchers.Tag(
+ 'Owned link', 'a', text='Owned branches',
+ attrs={'href': 'http://code.launchpad.dev/~barney'}))
+ self.factory.makeAnyBranch(owner=self.person)
+ page = self.get_branch_list_page('+subscribedbranches')
+ self.assertThat(page, owned_branches_matcher)
+
+ def test_branch_list_subscribed_link(self):
+ subscribed_branches_matcher = soupmatchers.HTMLContains(
+ soupmatchers.Tag(
+ 'Subscribed link', 'a', text='Subscribed branches',
+ attrs={'href': 'http://launchpad.dev/~barney'
+ '/+subscribedbranches'}))
+ self.factory.makeAnyBranch(owner=self.person)
+ page = self.get_branch_list_page()
+ self.assertThat(page, subscribed_branches_matcher)
+
+ def test_branch_list_activereviews_link(self):
+ active_review_matcher = soupmatchers.HTMLContains(
+ soupmatchers.Tag(
+ 'Active reviews link', 'a', text='Active reviews',
+ attrs={'href': 'http://launchpad.dev/~barney'
+ '/+activereviews'}))
+ branch = self.factory.makeAnyBranch(owner=self.person)
+ self.factory.makeBranchMergeProposal(
+ target_branch=branch, registrant=self.person,
+ set_state=BranchMergeProposalStatus.NEEDS_REVIEW)
+ page = self.get_branch_list_page()
+ self.assertThat(page, active_review_matcher)
+
+ def test_branch_list_no_registered_link_team(self):
+ self.factory.makeAnyBranch(owner=self.person)
+ page = self.get_branch_list_page(user=self.team)
+ self.assertThat(page, Not(self.registered_branches_matcher))
+
+
class TestSourcePackageBranchesView(TestCaseWithFactory):
layer = DatabaseFunctionalLayer
=== modified file 'lib/lp/code/interfaces/branchcollection.py'
--- lib/lp/code/interfaces/branchcollection.py 2011-08-25 20:37:02 +0000
+++ lib/lp/code/interfaces/branchcollection.py 2011-11-01 08:51:16 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
# pylint: disable-msg=E0211, E0213
@@ -50,6 +50,9 @@
def count():
"""The number of branches in this collection."""
+ def is_empty():
+ """Is this collection empty?"""
+
def ownerCounts():
"""Return the number of different branch owners.
=== modified file 'lib/lp/code/model/branchcollection.py'
--- lib/lp/code/model/branchcollection.py 2011-10-09 23:35:43 +0000
+++ lib/lp/code/model/branchcollection.py 2011-11-01 08:51:16 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Implementations of `IBranchCollection`."""
@@ -33,31 +33,31 @@
DecoratedResultSet,
)
from canonical.launchpad.interfaces.lpstorm import IStore
+from canonical.launchpad.searchbuilder import any
from canonical.launchpad.webapp.interfaces import (
DEFAULT_FLAVOR,
IStoreSelector,
MAIN_STORE,
)
-from canonical.launchpad.searchbuilder import any
from canonical.launchpad.webapp.vocabulary import CountableIterator
from lp.bugs.interfaces.bugtask import (
+ BugTaskSearchParams,
IBugTaskSet,
- BugTaskSearchParams,
)
from lp.bugs.interfaces.bugtaskfilter import filter_bugtasks_by_context
from lp.bugs.model.bugbranch import BugBranch
from lp.bugs.model.bugtask import BugTask
+from lp.code.enums import BranchMergeProposalStatus
from lp.code.interfaces.branch import user_has_special_branch_access
from lp.code.interfaces.branchcollection import (
IBranchCollection,
InvalidFilter,
)
+from lp.code.interfaces.branchlookup import IBranchLookup
+from lp.code.interfaces.codehosting import LAUNCHPAD_SERVICES
from lp.code.interfaces.seriessourcepackagebranch import (
IFindOfficialBranchLinks,
)
-from lp.code.enums import BranchMergeProposalStatus
-from lp.code.interfaces.branchlookup import IBranchLookup
-from lp.code.interfaces.codehosting import LAUNCHPAD_SERVICES
from lp.code.model.branch import Branch
from lp.code.model.branchmergeproposal import BranchMergeProposal
from lp.code.model.branchsubscription import BranchSubscription
@@ -132,6 +132,10 @@
"""See `IBranchCollection`."""
return self.getBranches(eager_load=False).count()
+ def is_empty(self):
+ """See `IBranchCollection`."""
+ return self.getBranches(eager_load=False).is_empty()
+
def ownerCounts(self):
"""See `IBranchCollection`."""
is_team = Person.teamowner != None
=== modified file 'lib/lp/code/templates/person-codesummary.pt'
--- lib/lp/code/templates/person-codesummary.pt 2010-11-18 12:05:34 +0000
+++ lib/lp/code/templates/person-codesummary.pt 2011-11-01 08:51:16 +0000
@@ -8,7 +8,7 @@
features request/features"
tal:condition="menu/show_summary">
- <table>
+ <table tal:condition="not: menu/simplified_branch_menu">
<tr class="code-links">
<td class="code-count" tal:content="menu/owned_branch_count">100</td>
<td tal:content="structure menu/owned/render"
@@ -34,4 +34,29 @@
/>
</tr>
</table>
+
+ <table tal:condition="menu/simplified_branch_menu">
+ <tr class="code-links"
+ tal:condition="menu/simplified_owned/enabled">
+ <td tal:content="structure menu/simplified_owned/render" />
+ </tr>
+ <tr class="code-links"
+ tal:condition="menu/simplified_registered/enabled">
+ <td tal:content="structure menu/simplified_registered/render" />
+ </tr>
+ <tr class="code-links"
+ tal:condition="menu/simplified_subscribed/enabled">
+ <td tal:content="structure menu/simplified_subscribed/render" />
+ </tr>
+ <tr class="code-links"
+ tal:condition="menu/simplified_active_reviews/enabled">
+ <td tal:content="structure menu/simplified_active_reviews/render" />
+ </tr>
+ <tr tal:condition="features/code.branchmergequeue" id="mergequeue-counts">
+ <td class="code-count" tal:content="menu/mergequeue_count">5</td>
+ <td tal:condition="menu"
+ tal:content="structure menu/mergequeues/render"
+ />
+ </tr>
+ </table>
</div>
=== modified file 'lib/lp/services/features/flags.py'
--- lib/lp/services/features/flags.py 2011-10-27 01:11:39 +0000
+++ lib/lp/services/features/flags.py 2011-11-01 08:51:16 +0000
@@ -64,6 +64,10 @@
'boolean',
'Shows incremental diffs on merge proposals.',
''),
+ ('code.simplified_branches_menu.enabled',
+ 'boolean',
+ ('Display a simplified version of the branch menu (omit the counts).'),
+ ''),
('hard_timeout',
'float',
'Sets the hard request timeout in milliseconds.',
