So milestones. This one was a little torture-like for me, due to a bug in the userbrowser: with a wrong password it silently did something other than retrieving the right page. :(
Other than that few-hour-distraction, I had a great performance tuesday. My cachedproperty branch and registry branches have landed. This means: - model caching in a (reasonably) safe way is now available to everyone - The /participation API should take ~11 queries to run :). Always (modulo missing a case, of course - certainly the badly behaved query we saw should be addressed). - there is a new storm release soon and we'll want it - I found a new storm bug in my registry branch. __storm_loaded__ was being called before objects are initialised, and this makes the Person 'isTeam' related _init hook cause a separate *single* db lookup - per person loaded when a cache is hit in this way. Hopefully this isn't affecting prod, but - we never know. So I started with https://bugs.edge.launchpad.net/launchpad-registry/+bug/447418 which was pretty clearly death-by-sql. Most of the SQL time was doing bug subscription lookups. The root cause of that is: for subscription in self.subscriptions: if user.inTeam(subscription.person): return True This is a problem: it looks like simple python code, but its dealing with hundreds of objects and causing DB access on simple attribute access to bugs. So, while we can make the code faster (see https://bugs.edge.launchpad.net/malone/+bug/619039) it would still be one-query-per-bug, which is going to scale very poorly. cachedproperty to the rescue. The patch is below (minus tests and refactorings to make it have more impact). Its simple conceptually: - use a DecoratedResultSet in bugtask queries - cache the user whom we queried-on-behalf-of-if-we're-returning-private-bugs - use that to satisfy userCanView lookups in bug permission checks. Some minor details about the arc I took, in case its of use in seeing patterns. I grabbed a fresh oops straight away (which was good - the queries counts alone were quite different) I ignored a python profile, because the sql time was so clearly bonkers. The oops told me that the top repeated query is 90% of sql time, and its a single-bug lookup for subscriptions I added a test for the query count with 1 and then 3 private bugs, to check that it didn't increase-per-bug In passing, I saw logs of custom-join code like Person._all_members and BugTaskSet.query that wasn't reused - it was very very similar in many places. e.g. in mailinglist.py I found it very trying to get a reproducable test case: once I had one it was about 1 hours work to put this fix together with reasonable confidence that it will work. Specific things that gave me trouble were: - the test browser password issue - some lack of familiarity with various test helpers - wouldn't affect any of you :) - not knowing what was triggering all the subscription lookups from the oops - I mean, I had one, from the faiing query, but for whatever reason (I claim ECOFFEE) I didn't twig that it was going to be totally representative until rather late in the process. Specifically I assumed stuff in security.py and authorization.py would be a) generic and b) fast. But its done. \o/. I don't know how many queries we'll see in prod with this, but I'm pretty hopeful it would be rather few. === modified file 'lib/canonical/launchpad/utilities/personroles.py' --- lib/canonical/launchpad/utilities/personroles.py 2010-01-14 11:28:51 +0000 +++ lib/canonical/launchpad/utilities/personroles.py 2010-08-17 09:10:19 +0000 @@ -35,6 +35,10 @@ except AttributeError: raise AttributeError(errortext) + @property + def id(self): + return self.person.id + === modified file 'lib/lp/bugs/model/bug.py' --- lib/lp/bugs/model/bug.py 2010-08-07 14:54:40 +0000 +++ lib/lp/bugs/model/bug.py 2010-08-17 09:09:33 +0000 @@ -42,6 +42,10 @@ ObjectCreatedEvent, ObjectDeletedEvent, ObjectModifiedEvent) from lazr.lifecycle.snapshot import Snapshot +from canonical.cachedproperty import ( + cachedproperty, + clear_property, + ) from canonical.config import config from canonical.database.constants import UTC_NOW from canonical.database.datetimecol import UtcDateTimeCol @@ -555,6 +559,7 @@ # disabled see the change. store.flush() self.updateHeat() + clear_property(self, '_cached_viewers') return def unsubscribeFromDupes(self, person, unsubscribed_by): @@ -1547,21 +1552,32 @@ self, self.messages[comment_number]) bug_message.visible = visible + @cachedproperty('_cached_viewers') + def _known_viewers(self): + """A dict of of known persons able to view this bug.""" + return set() + def userCanView(self, user): - """See `IBug`.""" + """See `IBug`. + + Note that Editing is also controlled by this check, + because we permit editing of any bug one can see. + """ + if user.id in self._known_viewers: + return True admins = getUtility(ILaunchpadCelebrities).admin if not self.private: # This is a public bug. return True - elif user.inTeam(admins): + elif user.in_admin: # Admins can view all bugs. return True else: # This is a private bug. Only explicit subscribers may view it. for subscription in self.subscriptions: if user.inTeam(subscription.person): + self._known_viewers.add(user.id) return True - return False def linkHWSubmission(self, submission): === modified file 'lib/lp/bugs/model/bugtask.py' --- lib/lp/bugs/model/bugtask.py 2010-08-03 08:49:19 +0000 +++ lib/lp/bugs/model/bugtask.py 2010-08-17 08:37:42 +0000 @@ -42,7 +42,7 @@ from lazr.enum import DBItem from canonical.config import config - +from canonical.cachedproperty import cache_property from canonical.database.sqlbase import ( SQLBase, block_implicit_flushes, convert_storm_clause_to_string, cursor, quote, quote_like, sqlvalues) @@ -51,6 +51,7 @@ from canonical.database.nl_search import nl_phrase_search from canonical.database.enumcol import EnumCol +from canonical.launchpad.components.decoratedresultset import DecoratedResultSet from canonical.launchpad.interfaces.lpstorm import IStore from canonical.launchpad.webapp.interfaces import ILaunchBag @@ -1534,11 +1535,15 @@ """Build and return an SQL query with the given parameters. Also return the clauseTables and orderBy for the generated query. + + :return: A query, the tables to query, ordering expression and a + decorator to call on each returned row. """ assert isinstance(params, BugTaskSearchParams) extra_clauses = ['Bug.id = BugTask.bug'] clauseTables = ['BugTask', 'Bug'] + decorators = [] # These arguments can be processed in a loop without any other # special handling. @@ -1814,6 +1819,11 @@ clause = get_bug_privacy_filter(params.user) if clause: extra_clauses.append(clause) + userid = params.user.id + def cache_user_can_view_bug(bugtask): + cache_property(bugtask.bug, '_cached_viewers', set([userid])) + return bugtask + decorators.append(cache_user_can_view_bug) hw_clause = self._buildHardwareRelatedClause(params) if hw_clause is not None: @@ -1842,7 +1852,15 @@ orderby_arg = self._processOrderBy(params) query = " AND ".join(extra_clauses) - return query, clauseTables, orderby_arg + + if not decorators: + decorator = lambda x:x + else: + def decorator(obj): + for decor in decorators: + obj = decor(obj) + return obj + return query, clauseTables, orderby_arg, decorator def _buildUpstreamClause(self, params): """Return an clause for returning upstream data if the data exists. @@ -2074,22 +2092,23 @@ """See `IBugTaskSet`.""" store_selector = getUtility(IStoreSelector) store = store_selector.get(MAIN_STORE, SLAVE_FLAVOR) - query, clauseTables, orderby = self.buildQuery(params) + query, clauseTables, orderby, decorator = self.buildQuery(params) if len(args) == 0: # Do normal prejoins, if we don't have to do any UNION # queries. Prejoins don't work well with UNION, and the way # we prefetch objects without prejoins cause problems with # COUNT(*) queries, which get inefficient. - return BugTask.select( + resultset = BugTask.select( query, clauseTables=clauseTables, orderBy=orderby, prejoins=['product', 'sourcepackagename'], prejoinClauseTables=['Bug']) + return DecoratedResultSet(resultset, result_decorator=decorator) bugtask_fti = SQL('BugTask.fti') result = store.find((BugTask, bugtask_fti), query, AutoTables(SQL("1=1"), clauseTables)) for arg in args: - query, clauseTables, dummy = self.buildQuery(arg) + query, clauseTables, dummy, decorator = self.buildQuery(arg) result = result.union( store.find((BugTask, bugtask_fti), query, AutoTables(SQL("1=1"), clauseTables))) @@ -2108,7 +2127,7 @@ (BugTask, Bug, Product, SourcePackageName)) bugtasks = SQLObjectResultSet(BugTask, orderBy=orderby, prepared_result_set=result) - return bugtasks + return DecoratedResultSet(bugtasks, result_decorator=decorator) def getAssignedMilestonesFromSearch(self, search_results): """See `IBugTaskSet`.""" _______________________________________________ Mailing list: https://launchpad.net/~launchpad-dev Post to : [email protected] Unsubscribe : https://launchpad.net/~launchpad-dev More help : https://help.launchpad.net/ListHelp
