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

Proposed by Gary Poster
Status: Merged
Approved by: Gary Poster
Approved revision: no longer in the source branch.
Merged at revision: 13850
Proposed branch: lp://staging/~gary/launchpad/bug724025
Merge into: lp://staging/launchpad
Diff against target: 173 lines (+87/-8)
5 files modified
lib/lp/bugs/browser/bugtask.py (+9/-4)
lib/lp/bugs/browser/tests/test_bugtask.py (+42/-0)
lib/lp/bugs/model/bug.py (+2/-2)
lib/lp/code/model/branchcollection.py (+11/-2)
lib/lp/code/model/tests/test_branchcollection.py (+23/-0)
To merge this branch: bzr merge lp://staging/~gary/launchpad/bug724025
Reviewer Review Type Date Requested Status
Jeroen T. Vermeulen (community) Approve
Review via email: mp+73432@code.staging.launchpad.net

Commit message

[r=jtv][bug=724025] implement some low-hanging optimizations when a bug has many branches.

Description of the change

This tries again to implement some low-hanging optimizations when a bug has many branches.

The main idea of the code was already approved.

https://code.launchpad.net/~gary/launchpad/bug724025/+merge/72776

However, that version was qa-bad: it turned out that when a bug didn't have any branches at all, my optimization was getting *all merge proposals* in the system. OOPS, as it were.

To fix this, I made two changes. In the bugtask code I had added before, if there are no branches, I don't bother to try and call getMergeProposals. This alone would still leave a trap for others, though, so I also made getMergeProposals be more careful: if you pass None for branches or revisions, that means no restriction; but if you pass an empty collection, that means you don't get any results. IOW, if you want merge proposals that are associated with an empty set of branches, that results in an empty set of merge proposals.

Thank you

To post a comment you must log in.
Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :
Download full text (7.2 KiB)

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

Read more...

review: Approve
Revision history for this message
Gary Poster (gary) wrote :
Download full text (4.1 KiB)

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

Read more...

Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.