Hi Gary, Nice cover letter. Worth a chuckle. The distinction between None and [] makes perfect sense to me. Just a very few nitpicks: === modified file 'lib/lp/bugs/browser/bugtask.py' --- lib/lp/bugs/browser/bugtask.py 2011-08-30 16:43:16 +0000 +++ lib/lp/bugs/browser/bugtask.py 2011-08-30 19:47:25 +0000 @@ -882,10 +883,18 @@ @cachedproperty def linked_branches(self): """Filter out the bug_branch links to non-visible private branches.""" - linked_branches = [] - for linked_branch in self.context.bug.linked_branches: - if check_permission('launchpad.View', linked_branch.branch): - linked_branches.append(linked_branch) + linked_branches = list( + self.context.bug.getVisibleLinkedBranches( + self.user, eager_load=True)) + # This is an optimization for when we look at the merge proposals. + # Note that, like all of these sorts of Storm cache optimizations, it + # only helps if [launchpad] storm_cache_size in launchpad-lazr.conf is + # pretty big--and as of this writing, it isn't for developer + # instances. + if linked_branches: + list(getUtility(IAllBranches).getMergeProposals( + for_branches=[link.branch for link in linked_branches], + eager_load=True)) The comment about dev cache sizes makes me nervous. Is it that noticeable? On dev systems I would expect cold-start overhead to dominate anyway. Or if it's not a dramatic issue, I'd just leave that note out. === modified file 'lib/lp/bugs/browser/tests/test_bugtask.py' --- lib/lp/bugs/browser/tests/test_bugtask.py 2011-08-30 16:43:16 +0000 +++ lib/lp/bugs/browser/tests/test_bugtask.py 2011-08-30 19:47:25 +0000 @@ -124,6 +124,51 @@ self.factory.makeBugAttachment(bug=task.bug) self.assertThat(task, browses_under_limit) + def test_rendered_query_counts_reduced_with_branches(self): + f = self.factory + owner = f.makePerson() + ds = f.makeDistroSeries() + sourcepackagenames = [ + f.makeSourcePackageName('testsourcepackagename%d' % i) + for i in range(10)] + sourcepackages = [ + f.makeSourcePackage( + sourcepackagename=name, distroseries=ds, publish=True) + for name in sourcepackagenames] + bug = f.makeBug() + bugtasks = [] + for sp in sourcepackages: + bugtask = f.makeBugTask(bug=bug, owner=owner, target=sp) + bugtasks.append(bugtask) + task = bugtasks[0] + url = canonical_url(task) + recorder = QueryCollector() + recorder.register() + self.addCleanup(recorder.unregister) + self.invalidate_caches(task) + self.getUserBrowser(url, owner) + # At least 20 of these should be removed. + self.assertThat(recorder, HasQueryCount(LessThan(100))) + count_with_no_branches = recorder.count + self.invalidate_caches(task) + with person_logged_in(owner): + for sp in sourcepackages: + target_branch = f.makePackageBranch( + sourcepackage=sp, owner=owner) + source_branch = f.makeBranchTargetBranch( + target_branch.target, owner=owner) + bug.linkBranch(source_branch, owner) + f.makeBranchMergeProposal( + target_branch=target_branch, + registrant=owner, + source_branch=source_branch) + self.getUserBrowser(url, owner) + # Ideally this should be much fewer, but this tries to keep a win of + # removing more than half of these. + self.assertThat(recorder, HasQueryCount( + LessThan(count_with_no_branches + 45), + )) That is a lot of code. Is there no way to ship some of it out to helpers so that it's easier to get the gist of what goes on? Also, does it make sense to include the makePackageBranch calls in the query count? === modified file 'lib/lp/code/model/branchcollection.py' --- lib/lp/code/model/branchcollection.py 2011-08-29 07:55:48 +0000 +++ lib/lp/code/model/branchcollection.py 2011-08-30 19:47:25 +0000 @@ -280,8 +281,12 @@ target_branch=None, merged_revnos=None, eager_load=False): """See `IBranchCollection`.""" - if (self._asymmetric_filter_expressions or for_branches or - target_branch or merged_revnos): + if (self._asymmetric_filter_expressions or for_branches is not None or + target_branch is not None or merged_revnos is not None): + if ((for_branches is not None and not for_branches) or + (merged_revnos is not None and not merged_revnos)): + # Make a shortcut. + return EmptyResultSet() There must be a clearer way to say this! For starters, the two sides of the inner if's "or" could be broken out into separate "ifs." Also, either of the inner if's conditions implies the outer if's condition. So you're free to move those two shortcuts out of the inner if. That also gives you room to explain what each condition means: if for_branches is not None and not for_branches: # Empty branches list. return EmptyResultSet() if merged_revnos is not None and not merged_revnos: # Empty revnos list. return EmptyResultSet() if (self._asymmetric_filter_expressions or # … Or for bonus points, extract the "is this an empty sequence?" into a meaningfully-named function. That way you won't even need comments. === modified file 'lib/lp/code/model/tests/test_branchcollection.py' --- lib/lp/code/model/tests/test_branchcollection.py 2011-08-29 07:55:48 +0000 +++ lib/lp/code/model/tests/test_branchcollection.py 2011-08-30 19:47:25 +0000 @@ -742,6 +743,28 @@ proposals = self.all_branches.getMergeProposals() self.assertEqual([], list(proposals)) + def test_empty_branches_shortcut(self): + # If you explicitly pass an empty collection of branches, + # the method shortcuts and gives you an empty result set. In this + # way, for_branches=None (the default) has a very different behavior + # than for_branches=[]: the first is no restriction, while the second + # excludes everything. + mp = self.factory.makeBranchMergeProposal() + proposals = self.all_branches.getMergeProposals(for_branches=[]) + self.assertEqual([], list(proposals)) + self.assertIsInstance(proposals, EmptyResultSet) + + def test_empty_revisions_shortcut(self): + # If you explicitly pass an empty collection of revision numbers, + # the method shortcuts and gives you an empty result set. In this + # way, merged_revnos=None (the default) has a very different behavior + # than merged_revnos=[]: the first is no restriction, while the second + # excludes everything. + mp = self.factory.makeBranchMergeProposal() + proposals = self.all_branches.getMergeProposals(merged_revnos=[]) + self.assertEqual([], list(proposals)) + self.assertIsInstance(proposals, EmptyResultSet) + I assume there's also a test where you let both for_branches and merged_revnos default to None and get a nonempty list back? Jeroen