launchpad-reviewers team mailing list archive

[Данило Шеган <danilo@xxxxxxxxxxxxx>]

Данило Шеган has proposed merging lp:~danilo/launchpad/bug-662552-fast-pofile-selection into lp:launchpad/devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #662552 Timeout error trying to access the translation interface
  https://bugs.launchpad.net/bugs/662552


= Bug 662552 =

After 8.4 migration, a specific query has become much slower and that's causing a big number of timeouts on POFile:+translate page (the most important page in Translations).  Proper solution would be to refactor the entire page and views to not emit hundreds of queries, but that's more work that we'll have to do over time.

For now, we first worked around the problem by disabling global suggestions, one of very useful features in Translations.  That bought us some time to fix the problem (before users start complaining about global suggestions not showing up).

And this branch is now, hopefully, fix for the problem. We'll see how much it helps when it gets rolled out to production (I'll cowboy it on staging as soon as a LOSA shows up to test it out), but it does a few what might seem to be nasty things.

1. It uses a subselect for what would otherwise be a very straightforward join query
2. It uses LIMIT in the subselect, so we can't use Storm syntax for the subselect (afaik)
3. It's not equivalent query to the previous one, but my knowledge of the data model allows me to optimize it better (i.e. restrict more before the join)

getOnePOFile does not return a predictable result.  The point is that it needs to return one POFile where TranslationMessage is used, and it doesn't really matter which one it is.

I've also added a few unit tests since this method was not tested so far.

= Tests =

bin/test -cvvt getOnePOFile


= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/translations/model/translationmessage.py
  lib/lp/translations/tests/test_translationmessage.py

-- 
https://code.launchpad.net/~danilo/launchpad/bug-662552-fast-pofile-selection/+merge/40615
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~danilo/launchpad/bug-662552-fast-pofile-selection into lp:launchpad/devel.

=== modified file 'lib/lp/translations/model/translationmessage.py'
--- lib/lp/translations/model/translationmessage.py	2010-10-27 18:30:31 +0000
+++ lib/lp/translations/model/translationmessage.py	2010-11-11 11:51:48 +0000
@@ -35,6 +35,7 @@
     SQLBase,
     sqlvalues,
     )
+from canonical.launchpad.interfaces.lpstorm import IStore
 from lp.registry.interfaces.person import validate_public_person
 from lp.services.propertycache import cachedproperty
 from lp.translations.interfaces.translationmessage import (
@@ -360,20 +361,23 @@
     def getOnePOFile(self):
         """See `ITranslationMessage`."""
         from lp.translations.model.pofile import POFile
-        clauses = [
-            "POFile.potemplate = TranslationTemplateItem.potemplate",
-            "TranslationTemplateItem.potmsgset = %s" % (
-                sqlvalues(self.potmsgset)),
-            "POFile.language = %s" % sqlvalues(self.language),
-            ]
 
-        pofiles = POFile.select(' AND '.join(clauses),
-                                clauseTables=['TranslationTemplateItem'])
-        pofile = list(pofiles[:1])
-        if len(pofile) > 0:
-            return pofile[0]
-        else:
-            return None
+        # Get any POFile where this translation exists.
+        # Because we can't create a subselect with "LIMIT" using Storm,
+        # we directly embed a subselect using raw SQL instead.
+        # We can do this because our message sharing code ensures a POFile
+        # exists for any of the sharing templates.
+        # This approach gives us roughly a 100x performance improvement
+        # compared to straightforward join as of 2010-11-11. - danilo
+        pofile = IStore(self).find(
+            POFile,
+            POFile.potemplateID == SQL(
+              """(SELECT potemplate
+                    FROM TranslationTemplateItem
+                    WHERE potmsgset = %s AND sequence > 0
+                    LIMIT 1)""" % sqlvalues(self.potmsgset)),
+            POFile.language == self.language).one()
+        return pofile
 
     def ensureBrowserPOFile(self):
         """See `ITranslationMessage`."""

=== modified file 'lib/lp/translations/tests/test_translationmessage.py'
--- lib/lp/translations/tests/test_translationmessage.py	2010-10-06 18:53:53 +0000
+++ lib/lp/translations/tests/test_translationmessage.py	2010-11-11 11:51:48 +0000
@@ -32,6 +32,36 @@
         tm = self.factory.makeTranslationMessage(pofile=pofile)
 
 
+class TestTranslationMessage(TestCaseWithFactory):
+    """Basic unit tests for TranslationMessage class.
+    """
+    layer = ZopelessDatabaseLayer
+
+    def test_getOnePOFile(self):
+        language = self.factory.makeLanguage('sr@test')
+        pofile = self.factory.makePOFile(language.code)
+        tm = self.factory.makeTranslationMessage(pofile=pofile)
+        self.assertEquals(pofile, tm.getOnePOFile())
+
+    def test_getOnePOFile_shared(self):
+        language = self.factory.makeLanguage('sr@test')
+        pofile1 = self.factory.makePOFile(language.code)
+        pofile2 = self.factory.makePOFile(language.code)
+        tm = self.factory.makeTranslationMessage(pofile=pofile1)
+        # Share this POTMsgSet with the other POTemplate (and POFile).
+        tm.potmsgset.setSequence(pofile2.potemplate, 1)
+        self.assertTrue(tm.getOnePOFile() in [pofile1, pofile2])
+
+    def test_getOnePOFile_no_pofile(self):
+        # When POTMsgSet is obsolete (sequence=0), no matching POFile
+        # is returned.
+        language = self.factory.makeLanguage('sr@test')
+        pofile = self.factory.makePOFile(language.code)
+        tm = self.factory.makeTranslationMessage(pofile=pofile)
+        tm.potmsgset.setSequence(pofile.potemplate, 0)
+        self.assertEquals(None, tm.getOnePOFile())
+
+
 class TestTranslationMessageFindIdenticalMessage(TestCaseWithFactory):
     """Tests for `TranslationMessage.findIdenticalMessage`."""