launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #03895
[Merge] lp:~adeuring/launchpad/bug-735991 into lp:launchpad
Abel Deuring has proposed merging lp:~adeuring/launchpad/bug-735991 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #735991 in Launchpad itself: "Person:+packagebugs timeouts"
https://bugs.launchpad.net/launchpad/+bug/735991
For more details, see:
https://code.launchpad.net/~adeuring/launchpad/bug-735991/+merge/64171
This branch fixes bug 735991: Person:+packagebugs timeouts
The OOPS reports show that SourcePackageNames are loaded quite
ineffciently, which takes nearly 7 seconds. The reason is the
inefficent implementation of Person.getBugSubscriberPackages().
This method retrieves all structural subscription for a
person/team, then checks if the target is a
DistributionSourcePackage, and if so, it loads the DSP.
The fix is obvious: Using the WHERE clause from
Peron.structuralsubscriptions, we can retrieve a join of the
tables Distribution, SourcePackageName and
StructuralSubscription. The decorator of a DecoratedResultSet
then builds the DSP instances.
This should save ca 6 or 7 seconds from the total execution
time as seen in the OOPS reports from the bug, but there
remains a lot of "Python time". Looking at the profile data
(https://staging.launchpad.net/~motuscience/+packagebugs/++profile++show)
showed a culprit which I did not expect: ca. 3400 calls of
canonical_url(self.context) required ca 3 seconds.
The function is called six times for each DSP, in order to
build links to bug search pages. The fix is trivial: Cache
the result of canonical_url(self.context).
I also noticed another small bottleneck: 3400 calls of
urllib.urlencode() needed nearly one second. These calls
are used to get the "cross product" of the search parameters
(open bugs, critical bugs, high bugs,...) with all DSPs.
I considered to make this additional change:
1. store the query parameteres for the different search
types as class properties.
2. call urlencode() once for each DSP
3. combine the urlencode() results from 1 and 2 in
getOpenBugsURL(), getCriticalBugsURL() etc with a simple
'%s&%s' % (dsp_params, filter_params)
This would reduce the number of urlencode() calls by 5/6. The
time saving might be even larger, because urlencode()
iterates over a parameter sequence, and setting the filter
needs more parameters than setting the DSP.
I did not do this out of lazyness and because it felt wrong
to do this "base level URL construction" in a "high level" view
class.
tests:
./bin/test bugs -vvt lp.bugs.browser.tests.test_person_bugs
./bin/test registry -vvt lp.registry.tests.test_person
--
https://code.launchpad.net/~adeuring/launchpad/bug-735991/+merge/64171
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~adeuring/launchpad/bug-735991 into lp:launchpad.
=== modified file 'lib/lp/bugs/browser/tests/test_person_bugs.py'
--- lib/lp/bugs/browser/tests/test_person_bugs.py 2011-02-14 15:54:25 +0000
+++ lib/lp/bugs/browser/tests/test_person_bugs.py 2011-06-10 12:58:30 +0000
@@ -7,7 +7,13 @@
from canonical.testing.layers import DatabaseFunctionalLayer
from lp.app.errors import UnexpectedFormData
-from lp.testing import TestCaseWithFactory
+from lp.bugs.interfaces.bugtask import BugTaskStatus
+from lp.registry.browser import person
+from lp.testing import (
+ person_logged_in,
+ TestCaseWithFactory,
+ )
+from lp.testing.fakemethod import FakeMethod
from lp.testing.views import create_initialized_view
@@ -68,3 +74,26 @@
self.person, name='+packagebugs-search', form=form)
self.assertRaises(
UnexpectedFormData, getattr, view, 'current_package')
+
+ def test_one_call_of_canonical_url(self):
+ # canonical_url(self.context) is frequently needed to build
+ # URLs pointing to specific search listings in the
+ # +packagebugs page. These URLs are returned, among other
+ # data, by
+ # BugSubscriberPackageBugsSearchListingView.package_bug_counts
+ # This call is relatively expensive, hence a cached value is
+ # used.
+ view = create_initialized_view(self.person, name='+packagebugs')
+ self.factory.makeBug(
+ distribution=self.distribution, sourcepackagename=self.spn,
+ status=BugTaskStatus.INPROGRESS)
+ with person_logged_in(self.person):
+ self.dsp.addSubscription(self.person, subscribed_by=self.person)
+ fake_canonical_url = FakeMethod(result='')
+ real_canonical_url = person.canonical_url
+ person.canonical_url = fake_canonical_url
+ try:
+ view.package_bug_counts
+ self.assertEqual(1, fake_canonical_url.call_count)
+ finally:
+ person.canonical_url = real_canonical_url
=== modified file 'lib/lp/registry/browser/person.py'
--- lib/lp/registry/browser/person.py 2011-06-01 12:31:23 +0000
+++ lib/lp/registry/browser/person.py 2011-06-10 12:58:30 +0000
@@ -2005,6 +2005,10 @@
return package_links
+ @cachedproperty
+ def person_url(self):
+ return canonical_url(self.context)
+
def getBugSubscriberPackageSearchURL(self, distributionsourcepackage=None,
advanced=False, extra_params=None):
"""Construct a default search URL for a distributionsourcepackage.
@@ -2029,14 +2033,13 @@
params.update(extra_params)
- person_url = canonical_url(self.context)
query_string = urllib.urlencode(sorted(params.items()), doseq=True)
if advanced:
- return (person_url + '/+packagebugs-search?advanced=1&%s'
+ return (self.person_url + '/+packagebugs-search?advanced=1&%s'
% query_string)
else:
- return person_url + '/+packagebugs-search?%s' % query_string
+ return self.person_url + '/+packagebugs-search?%s' % query_string
def getBugSubscriberPackageAdvancedSearchURL(self,
distributionsourcepackage=None):
=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py 2011-06-03 10:38:25 +0000
+++ lib/lp/registry/model/person.py 2011-06-10 12:58:30 +0000
@@ -271,6 +271,7 @@
)
from lp.registry.model.personlocation import PersonLocation
from lp.registry.model.pillar import PillarName
+from lp.registry.model.sourcepackagename import SourcePackageName
from lp.registry.model.teammembership import (
TeamMembership,
TeamMembershipSet,
@@ -943,11 +944,30 @@
# rather than package bug supervisors.
def getBugSubscriberPackages(self):
"""See `IPerson`."""
- packages = [sub.target for sub in self.structural_subscriptions
- if (sub.distribution is not None and
- sub.sourcepackagename is not None)]
- packages.sort(key=lambda x: x.name)
- return packages
+ # Avoid circular imports.
+ from lp.registry.model.distributionsourcepackage import (
+ DistributionSourcePackage,
+ )
+ from lp.registry.model.distribution import Distribution
+ origin = (
+ StructuralSubscription,
+ Join(
+ Distribution,
+ StructuralSubscription.distributionID == Distribution.id),
+ Join(
+ SourcePackageName,
+ StructuralSubscription.sourcepackagenameID ==
+ SourcePackageName.id)
+ )
+ result = Store.of(self).using(*origin).find(
+ (Distribution, SourcePackageName),
+ self.structural_subscriptions_clause)
+ result.order_by(SourcePackageName.name)
+
+ def decorator(row):
+ return DistributionSourcePackage(*row)
+
+ return DecoratedResultSet(result, decorator)
def findPathToTeam(self, team):
"""See `IPerson`."""
@@ -2755,11 +2775,15 @@
return bugtask_count > 0
@property
+ def structural_subscriptions_clause(self):
+ return StructuralSubscription.subscriberID == self.id
+
+ @property
def structural_subscriptions(self):
"""See `IPerson`."""
return IStore(self).find(
StructuralSubscription,
- StructuralSubscription.subscriberID == self.id).order_by(
+ self.structural_subscriptions_clause).order_by(
Desc(StructuralSubscription.date_created))
def autoSubscribeToMailingList(self, mailinglist, requester=None):
=== modified file 'lib/lp/registry/tests/test_person.py'
--- lib/lp/registry/tests/test_person.py 2011-05-27 21:12:25 +0000
+++ lib/lp/registry/tests/test_person.py 2011-06-10 12:58:30 +0000
@@ -8,7 +8,10 @@
from lazr.lifecycle.snapshot import Snapshot
import pytz
from storm.store import Store
-from testtools.matchers import LessThan
+from testtools.matchers import (
+ Equals,
+ LessThan,
+ )
import transaction
from zope.component import getUtility
from zope.interface import providedBy
@@ -374,6 +377,59 @@
with person_logged_in(person):
self.assertRaises(Unauthorized, getattr, other, 'canWrite')
+ def makeSubscribedDistroSourcePackages(self):
+ # Create a person, a distribution and three
+ # DistributionSourcePacakage. Subscribe the person to two
+ # DSPs.
+ user = self.factory.makePerson()
+ distribution = self.factory.makeDistribution()
+ dsp1 = self.factory.makeDistributionSourcePackage(
+ sourcepackagename='sp-b', distribution=distribution)
+ distribution = self.factory.makeDistribution()
+ dsp2 = self.factory.makeDistributionSourcePackage(
+ sourcepackagename='sp-a', distribution=distribution)
+ dsp3 = self.factory.makeDistributionSourcePackage(
+ sourcepackagename='sp-c', distribution=distribution)
+ with person_logged_in(user):
+ dsp1.addSubscription(user, subscribed_by=user)
+ dsp2.addSubscription(user, subscribed_by=user)
+ return user, dsp1, dsp2, dsp3
+
+ def test_getBugSubscriberPackages(self):
+ # getBugSubscriberPackages() returns the DistributionSourcePackages
+ # to which a user is subscribed.
+ user, dsp1, dsp2, dsp3 = self.makeSubscribedDistroSourcePackages()
+
+ # We cannot directly compare the objects returned by
+ # getBugSubscriberPackages() with the expected DSPs:
+ # These are different objects and the clas does not have
+ # an __eq__ operator. So we compare the attributes distribution
+ # and sourcepackagename.
+
+ def get_distribution(dsp):
+ return dsp.distribution
+
+ def get_spn(dsp):
+ return dsp.sourcepackagename
+
+ result = user.getBugSubscriberPackages()
+ self.assertEqual(
+ [get_distribution(dsp) for dsp in (dsp2, dsp1)],
+ [get_distribution(dsp) for dsp in result])
+ self.assertEqual(
+ [get_spn(dsp) for dsp in (dsp2, dsp1)],
+ [get_spn(dsp) for dsp in result])
+
+ def test_getBugSubscriberPackages__one_query(self):
+ # getBugSubscriberPackages() retrieves all objects
+ # needed to build the DistributionSourcePackages in
+ # one SQL query.
+ user, dsp1, dsp2, dsp3 = self.makeSubscribedDistroSourcePackages()
+ Store.of(user).invalidate()
+ with StormStatementRecorder() as recorder:
+ list(user.getBugSubscriberPackages())
+ self.assertThat(recorder, HasQueryCount(Equals(1)))
+
class TestPersonStates(TestCaseWithFactory):
