launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

The proposal to merge lp:~jtv/launchpad/bug-884649-branch-1 into lp:launchpad has been updated.

Description changed to:

= Summary =

The Dominator is having performance problems, now that we've introduced binary double-domination.  (It's probably not as exciting as it sounds, but it'll fix a lot of uninstallable packages.)

Getting dominator performance under control needs some careful but radical internal reorganizations.

== Proposed fix ==

Prepare for some of the coming changes (yes, there's a bug-884649-branch-2 in the works) and, while we're at it, cut some dead wood out of the main query involved in the added functionality.

That main query is executed for almost any binary package publication at some point in its lifetime.  The dead wood is two tables being joined through SourcePackageRelease, when all that's needed is an equijoin on the foreign keys to SourcePackageRelease.  The table itself isn't needed in the join; this was easy to miss given the complexity of the search.


== Pre-implementation notes ==

Discussed extensively with Julian.  see bug 884649 for notes.

== Implementation details ==

You'll see two complex queries extracted into new methods, so as to keep the higher-level logic clearer and more isolated.  This serves another purpose: the same logic was constructed once and then invoked twice, from a little locally-defined function that I need to break up.  Had to find other ways to minimize repetition.

I'm also adding some code duplication: two poorly-related conditions in an if/elif chain that have the same, very simple body.  It was clearer as an intermediate step to separate those into separate elifs.  But rest assured: my next branch hoists that logic out of there and into a dedicated policy function (which is used only where needed, thus saving time).  So the duplication is only there so that the complexity can be more easily removed.


== Tests ==

I ran them all, actually.  But anything with “dominator” or “domination” or “dominate” in the name is particularly likely to be relevant:
{{{
./bin/test -vvc -t dominat
}}}

== Demo and Q/A ==

The dominator should still work, and probably be just slightly faster.  It's hard to benchmark accurately though, since every run will face different sets of active publications.  We could just run it twice on a second server and time the second run; the second run won't be as representative because it has no changes to make, but at least it will run in predictable time.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/archivepublisher/domination.py
  lib/lp/soyuz/model/publishing.py

For more details, see:
https://code.launchpad.net/~jtv/launchpad/bug-884649-branch-1/+merge/80988
-- 
https://code.launchpad.net/~jtv/launchpad/bug-884649-branch-1/+merge/80988
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/bug-884649-branch-1 into lp:launchpad.