Merge lp:~jtv/launchpad/bug-515702 into lp:launchpad

Proposed by Jeroen T. Vermeulen
Status: Merged
Approved by: Muharem Hrnjadovic
Approved revision: not available
Merged at revision: not available
Proposed branch: lp:~jtv/launchpad/bug-515702
Merge into: lp:launchpad
Diff against target: 12 lines (+1/-1)
1 file modified
lib/lp/translations/browser/ (+1/-1)
To merge this branch: bzr merge lp:~jtv/launchpad/bug-515702
Reviewer Review Type Date Requested Status
Muharem Hrnjadovic (community) code Approve
Review via email:

Commit message

Speed up Distribution/DistroSeries translations overviews.

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

= Bug 515702 =

We're getting timeouts on the Distribution and DistroSeries translation overviews. In each of the oopses I saw, a majority of time was spent querying Language, Country, and the linking table between the two (in roughly a 2:1:1 ratio). These are very simple queries, but insanely repetitive: the top query is looking up a Language by language code several thousand times for one page. Given that we only have a few hundred languages in the database, we clearly need some caching.

It turns out that the timeouts all happened while the browser code was figuring out whether a given language is among the user's preferred languages, as determined based on user preference settings and http request parameters. Each of these checks again queried the full lists of these languages from the database. The check is needed because by default, we now hide non-preferred languages in the displayed page.

To speed this up, I made the TranslationsMixin.translatable_languages property cached. This is browser code, so should be perfectly safe to cache. Doing so breaks no browser or stories tests. As far as Henning and I can tell, there is no reason not to make this property cached except that it wasn't called so repetitively in the past.

No lint. No tests are affected; there is no semantical change, only a performance improvement on large, lifelike data sets. If the change does break anything, that's a hole in our existing test coverage which we can't realistically know about unless and until something does break.


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

I take back the bit about no tests: existing tests do cover this, but we don't test the affected property directly and that's something I can fix.

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

No, no, no, we do test it directly: lib/lp/translations/doc/translationsperson.txt. Integration-testing is covered by lib/lp/translations/windmill/tests/

Revision history for this message
Muharem Hrnjadovic (al-maisan) wrote :

Land that puppy!

review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/translations/browser/'
2--- lib/lp/translations/browser/ 2009-09-22 18:45:02 +0000
3+++ lib/lp/translations/browser/ 2010-02-01 21:38:13 +0000
4@@ -51,7 +51,7 @@
5 class TranslationsMixin:
6 """Provide Translations specific properties."""
8- @property
9+ @cachedproperty
10 def translatable_languages(self):
11 """Return a set of the Person's translatable languages."""
12 english = getUtility(ILaunchpadCelebrities).english