launchpad-reviewers team mailing list archive

[Robert Collins <robertc@xxxxxxxxxxxxxxxxx>]

Robert Collins has proposed merging lp:~lifeless/launchpad/bug-734751 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #734751 in Launchpad itself: "BranchSet:CollectionResource:#branches timeouts"
  https://bugs.launchpad.net/launchpad/+bug/734751

For more details, see:
https://code.launchpad.net/~lifeless/launchpad/bug-734751/+merge/54146

More eager loading good. I've extended bulk to have a higher level loader than load().
-- 
https://code.launchpad.net/~lifeless/launchpad/bug-734751/+merge/54146
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~lifeless/launchpad/bug-734751 into lp:launchpad.

=== modified file 'lib/lp/code/model/branch.py'
--- lib/lp/code/model/branch.py	2011-03-15 14:31:00 +0000
+++ lib/lp/code/model/branch.py	2011-03-21 03:54:30 +0000
@@ -612,7 +612,7 @@
         else:
             return True
 
-    @property
+    @cachedproperty
     def code_import(self):
         from lp.code.model.codeimport import CodeImportSet
         return CodeImportSet().getByBranch(self)

=== modified file 'lib/lp/code/model/branchcollection.py'
--- lib/lp/code/model/branchcollection.py	2011-03-07 12:43:40 +0000
+++ lib/lp/code/model/branchcollection.py	2011-03-21 03:54:30 +0000
@@ -59,6 +59,7 @@
     )
 from lp.code.model.branchmergeproposal import BranchMergeProposal
 from lp.code.model.branchsubscription import BranchSubscription
+from lp.code.model.codeimport import CodeImport
 from lp.code.model.codereviewcomment import CodeReviewComment
 from lp.code.model.codereviewvote import CodeReviewVoteReference
 from lp.code.model.seriessourcepackagebranch import SeriesSourcePackageBranch
@@ -71,6 +72,7 @@
 from lp.registry.model.product import Product
 from lp.registry.model.sourcepackagename import SourcePackageName
 from lp.registry.model.teammembership import TeamParticipation
+from lp.services.database.bulk import load_related
 from lp.services.propertycache import get_property_cache
 
 
@@ -199,6 +201,19 @@
                 cache = caches[link.branchID]
                 cache._associatedSuiteSourcePackages.append(
                     link.suite_sourcepackage)
+            load_related(Product, rows, ['productID'])
+            # So far have only needed the persons for their canonical_url - no
+            # need for validity etc in the /branches API call.
+            load_related(Person, rows,
+                ['ownerID', 'registrantID', 'reviewerID'])
+            # Cache all branches as having no code imports to prevent fruitless
+            # lookups on the ones we don't find.
+            for cache in caches.values():
+                cache.code_import = None
+            for code_import in IStore(CodeImport).find(
+                CodeImport, CodeImport.branchID.is_in(branch_ids)):
+                cache = caches[code_import.branchID]
+                cache.code_import = code_import
         return DecoratedResultSet(resultset, pre_iter_hook=do_eager_load)
 
     def getMergeProposals(self, statuses=None, for_branches=None,

=== modified file 'lib/lp/services/database/bulk.py'
--- lib/lp/services/database/bulk.py	2011-03-17 01:28:36 +0000
+++ lib/lp/services/database/bulk.py	2011-03-21 03:54:30 +0000
@@ -11,6 +11,7 @@
 
 
 from collections import defaultdict
+from functools import partial
 
 from storm.info import get_cls_info
 from storm.store import Store
@@ -70,10 +71,29 @@
             "Compound primary keys are not supported: %s." %
             object_type.__name__)
     primary_key_column = primary_key[0]
-    # Turn the primary keys into a set to eliminate duplicates,
-    # shortening the query.
-    condition = primary_key_column.is_in(set(primary_keys))
+    primary_keys = set(primary_keys)
+    primary_keys.discard(None)
+    if not primary_keys:
+        return []
+    condition = primary_key_column.is_in(primary_keys)
     if store is None:
         store = IStore(object_type)
-    query = store.find(object_type, condition)
-    return list(query)
+    return list(store.find(object_type, condition))
+
+
+def load_related(object_type, owning_objects, foreign_keys):
+    """Load objects of object_type referred to by owning_objects.
+
+    Note that complex types like Person are best loaded through dedicated
+    helpers that can eager load other related things (e.g. validity for
+    Person).
+
+    :param object_type: The object type to load - e.g. Person.
+    :param owning_objects: The objects holding the references. E.g. Bug.
+    :param foreign_keys: A list of attributes that should be inspected for
+        keys. e.g. ['ownerID']
+    """
+    keys = set()
+    for owning_object in owning_objects:
+        keys.update(map(partial(getattr, owning_object), foreign_keys))
+    return load(object_type, keys)

=== modified file 'lib/lp/services/database/tests/test_bulk.py'
--- lib/lp/services/database/tests/test_bulk.py	2011-03-08 09:14:30 +0000
+++ lib/lp/services/database/tests/test_bulk.py	2011-03-21 03:54:30 +0000
@@ -21,6 +21,7 @@
     )
 from canonical.testing.layers import DatabaseFunctionalLayer
 from lp.bugs.model.bug import BugAffectsPerson
+from lp.registry.model.person import Person
 from lp.services.database import bulk
 from lp.soyuz.model.component import Component
 from lp.testing import (
@@ -191,3 +192,14 @@
             Component, [db_object.id], store=slave_store)
         self.assertEqual(
             Store.of(db_object_from_slave), slave_store)
+
+    def test_load_related(self):
+        owning_objects = [
+            self.factory.makeBug(),
+            self.factory.makeBug(),
+            ]
+        expected = set(bug.owner for bug in owning_objects)
+        self.assertEqual(expected,
+            set(bulk.load_related(Person, owning_objects, ['ownerID'])))
+
+