Merge lp://staging/~gary/launchpad/bug724025 into lp://staging/launchpad

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,...

On Aug 31, 2011, at 2:13 AM, Jeroen T. Vermeulen wrote:

> Review: Approve
> Hi Gary,

Hi Jeroen. Thank you for the thoughtful review.

> 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

...

> The comment about dev cache sizes makes me nervous. Is it that noticeable?

It is noticeable if you are testing large values of objects in a page. I was doing this in order to diagnose the problem I addressed here and related ones. The large values both approximated the levels of the problem case and caused the problem sources to be easier to spot.

The way I noticed the cache size is not with speed, but with unexpected SQL queries for values that should have already been cached. Because the cache was so small on devel, values would be evicted in my experiments before they would on production, and so "populate the cache" optimizations would not behave as expected even though they were in fact correct.

> 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.

I omitted it. Ehn. :-)

> === 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

...

> 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?

I simplified it a bit. I made a local helper; I think the situation I'm trying to duplicate is not quite right for the factory.

> Also, does it make sense to include the makePackageBranch calls in the query count?

No. That's test set up for the comparison case.

If your question was rhetorical because you thought I *was* including makePackageBranch in the query count, it does not. The QueryCollector gets the queries from the most recent request, so the browser call I made is what we see in the results.

> === 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

...

> 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 # …

I was fascinated by my reaction to this comment. :-) I really did not want to do what you said because I did not like the extra compari...

> If your question was rhetorical because you thought I *was* including makePackageBranch in the query count, it does
> not. The QueryCollector gets the queries from the most recent request, so the browser call I made is what we see in
> the results.

Indeed, I thought it counted all queries. Evidently I was wrong. (Which is ironic because I think I was the one to introduce query counts in tests over 3 years ago, and have done some work on the query collector).