Merge lp:~wgrant/launchpad/tm-suggest-constant into lp:launchpad

[William Grant]

Preload suggestion details in POFile:+translate to eliminate per-suggestion queries. But there are still lots of per-message queries.

Drop POFile:+translate from O(messages + suggestions) queries to O(messages), by doing lots of pretty boring preloading. The only thing with significant complexity is preloadPOFilesAndSequences, which reimplements getOnePOFile in a bulk manner.

We still do the suggestion calculation inside the per-message subviews, so the preloading is done once for each message. But it's a good step in reducing the query count of this page, and we can pull the suggestion calculation and preloading into the upper view later.

I've added a query count test for POFile:+translate, which tests all the preloading I've done so far. It doesn't currently add new messages, just new suggestions, since extra messages still generate extra queries.

[Celso Providelo (cprov)]

Nice change, introducing preloadPOFilesAndSequence really solves the problem for loading suggestions for a given message.

We can land it ASAP.

(thinking out loud here, feel free to ignore me)

Although, I am not seeing a clear path for going further and pre-loading this for multiple messages. Maybe we won't need it (this step will bring enough benefits) or we could try to investigate something at the model level for making it possible.

review: Approve

=== modified file 'lib/lp/translations/browser/tests/test_pofile_view.py'
--- lib/lp/translations/browser/tests/test_pofile_view.py	2014-06-10 11:25:51 +0000
+++ lib/lp/translations/browser/tests/test_pofile_view.py	2014-07-02 06:12:24 +0000
@@ -3,20 +3,91 @@
 
 __metaclass__ = type
 
+from storm.store import Store
+from testtools.matchers import Equals
+from zope.component import getUtility
+
 from lp.app.errors import UnexpectedFormData
+from lp.app.interfaces.launchpad import ILaunchpadCelebrities
+from lp.services.webapp.interfaces import ILaunchBag
 from lp.services.webapp.servers import LaunchpadTestRequest
 from lp.testing import (
     BrowserTestCase,
     login,
+    login_person,
     person_logged_in,
+    record_two_runs,
     TestCaseWithFactory,
     )
 from lp.testing.layers import (
     DatabaseFunctionalLayer,
     ZopelessDatabaseLayer,
     )
+from lp.testing.matchers import HasQueryCount
+from lp.testing.views import create_initialized_view
 from lp.translations.browser.pofile import POFileTranslateView
 from lp.translations.enums import TranslationPermission
+from lp.translations.interfaces.potemplate import IPOTemplateSet
+from lp.translations.interfaces.translationsperson import ITranslationsPerson
+from lp.translations.model.pofiletranslator import POFileTranslator
+
+
+class TestQueryCount(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def test_query_count(self):
+        person = self.factory.makePerson()
+        login_person(person)
+        ITranslationsPerson(person).translations_relicensing_agreement = True
+        product = self.factory.makeProduct(owner=person)
+        product.translationpermission = TranslationPermission.OPEN
+        pofile = self.factory.makePOFile(
+            potemplate=self.factory.makePOTemplate(
+                productseries=product.series[0]))
+        pofile.potemplate.productseries.product
+        potmsgsets = [
+            self.factory.makePOTMsgSet(pofile.potemplate) for i in range(10)]
+
+        # Preload a few transaction-crossing caches that would give
+        # extra queries to the first request.
+        getUtility(ILaunchBag).time_zone
+        getUtility(ILaunchpadCelebrities).ubuntu
+        person.inTeam(getUtility(ILaunchpadCelebrities).admin)
+        person.inTeam(getUtility(ILaunchpadCelebrities).rosetta_experts)
+
+        def create_suggestions():
+            for potmsgset in potmsgsets:
+                pot = self.factory.makePOTemplate()
+                self.factory.makeCurrentTranslationMessage(
+                    potmsgset=self.factory.makePOTMsgSet(
+                        singular=potmsgset.msgid_singular.msgid,
+                        potemplate=pot),
+                    language=pofile.language,
+                    translations=[self.factory.getUniqueUnicode()])
+                # A suggestion only shows up if it's actually in a
+                # POFile.
+                self.factory.makePOFile(
+                    potemplate=pot, language=pofile.language)
+                self.factory.makeSuggestion(
+                    pofile=pofile, potmsgset=potmsgset)
+
+            # Ensure that these are valid suggestions.
+            templateset = getUtility(IPOTemplateSet)
+            templateset.wipeSuggestivePOTemplatesCache()
+            templateset.populateSuggestivePOTemplatesCache()
+
+            # And ensure that the credits string is empty, as that's
+            # not currently constant.
+            Store.of(pofile).find(POFileTranslator, pofile=pofile).set(
+                pofileID=self.factory.makePOFile().id)
+
+        nb_objects = 2
+        recorder1, recorder2 = record_two_runs(
+            lambda: create_initialized_view(
+                pofile, '+translate', principal=person)(),
+            create_suggestions, nb_objects)
+        self.assertThat(recorder2, HasQueryCount(Equals(recorder1.count)))
 
 
 class TestPOFileTranslateViewInvalidFiltering(TestCaseWithFactory):

=== modified file 'lib/lp/translations/browser/translationmessage.py'
--- lib/lp/translations/browser/translationmessage.py	2014-07-02 04:41:13 +0000
+++ lib/lp/translations/browser/translationmessage.py	2014-07-02 06:12:24 +0000
@@ -1212,24 +1212,6 @@
             else:
                 self.can_confirm_and_dismiss = True
 
-    def _setOnePOFile(self, messages):
-        """Return a list of messages that all have a browser_pofile set.
-
-        If a pofile cannot be found for a message, it is not included in
-        the resulting list.
-        """
-        result = []
-        for message in messages:
-            if message.browser_pofile is None:
-                pofile = message.getOnePOFile()
-                if pofile is None:
-                    # Do not include in result.
-                    continue
-                else:
-                    message.setPOFile(pofile)
-            result.append(message)
-        return result
-
     def _buildAllSuggestions(self):
         """Builds all suggestions and puts them into suggestions_block.
 
@@ -1277,8 +1259,8 @@
 
             self._set_dismiss_flags(local, other)
 
-            for suggestion in local:
-                suggestion.setPOFile(self.pofile)
+            getUtility(ITranslationMessageSet).preloadDetails(
+                local, need_potranslation=True, need_people=True)
 
             # Get a list of translations which are _used_ as translations
             # for this same message in a different translation template.
@@ -1289,19 +1271,36 @@
                 potmsgset.getExternallySuggestedOrUsedTranslationMessages(
                     suggested_languages=[language],
                     used_languages=used_languages))
+
+            # Suggestions from other templates need full preloading,
+            # including picking a POFile. preloadDetails requires that
+            # all messages have the same language, so we invoke it
+            # separately for the alternate suggestion language.
+            preload_groups = [
+                translations[language].used + translations[language].suggested,
+                translations[self.sec_lang].used,
+                ]
+            for group in preload_groups:
+                getUtility(ITranslationMessageSet).preloadDetails(
+                    group, need_pofile=True, need_potemplate=True,
+                    need_potemplate_context=True,
+                    need_potranslation=True, need_potmsgset=True,
+                    need_people=True)
+
             alt_external = translations[self.sec_lang].used
-            externally_used = self._setOnePOFile(sorted(
-                translations[language].used,
+            externally_used = sorted(
+                [m for m in translations[language].used if m.browser_pofile],
                 key=operator.attrgetter("date_created"),
-                reverse=True))
+                reverse=True)
 
             # Get a list of translations which are suggested as
             # translations for this same message in a different translation
             # template, but are not used.
-            externally_suggested = self._setOnePOFile(sorted(
-                translations[language].suggested,
+            externally_suggested = sorted(
+                [m for m in translations[language].suggested
+                 if m.browser_pofile],
                 key=operator.attrgetter("date_created"),
-                reverse=True))
+                reverse=True)
         else:
             # Don't show suggestions for anonymous users.
             local = externally_used = externally_suggested = []

=== modified file 'lib/lp/translations/interfaces/potemplate.py'
--- lib/lp/translations/interfaces/potemplate.py	2013-10-21 04:58:19 +0000
+++ lib/lp/translations/interfaces/potemplate.py	2014-07-02 06:12:24 +0000
@@ -698,6 +698,9 @@
         Return None if there is no such `IPOTemplate`.
         """
 
+    def preloadPOTemplateContexts(templates):
+        """Preload context objects for a sequence of POTemplates."""
+
     def wipeSuggestivePOTemplatesCache():
         """Erase suggestive-templates cache.
 

=== modified file 'lib/lp/translations/interfaces/translationmessage.py'
--- lib/lp/translations/interfaces/translationmessage.py	2013-01-07 02:40:55 +0000
+++ lib/lp/translations/interfaces/translationmessage.py	2014-07-02 06:12:24 +0000
@@ -343,3 +343,25 @@
             return.
         :param order_by: An SQL ORDER BY clause.
         """
+
+    def preloadDetails(messages, pofile=None, need_pofile=False,
+                       need_potemplate=False, need_potemplate_context=False,
+                       need_potranslation=False, need_potmsgset=False,
+                       need_people=False):
+        """Preload lots of details for `TranslationMessage`s.
+
+        If need_pofile is True, pofile may be left None to indicate that
+        an arbitrary `POFile` containing that message may be picked, or
+        set to a specific `POFile` in which all the messages must exist.
+        In either case, all messages must be for the same language.
+        """
+
+    def preloadPOFilesAndSequences(messages, pofile=None):
+        """Preload browser_pofile and sequence for `TranslationMessage`s.
+
+        All messages must be for the same language.
+
+        If pofile is None, an arbitrary POFile containing each message
+        will be used. Otherwise, each message must exist in the given
+        POFile.
+        """

=== modified file 'lib/lp/translations/model/potemplate.py'
--- lib/lp/translations/model/potemplate.py	2013-10-21 04:58:19 +0000
+++ lib/lp/translations/model/potemplate.py	2014-07-02 06:12:24 +0000
@@ -1347,6 +1347,14 @@
                     len(preferred_matches))
                 return None
 
+    def preloadPOTemplateContexts(self, templates):
+        """See `IPOTemplateSet`."""
+        from lp.registry.model.product import Product
+        from lp.registry.model.productseries import ProductSeries
+        pses = load_related(ProductSeries, templates, ['productseriesID'])
+        load_related(Product, pses, ['productID'])
+        load_related(SourcePackageName, templates, ['sourcepackagenameID'])
+
     def wipeSuggestivePOTemplatesCache(self):
         """See `IPOTemplateSet`."""
         return IMasterStore(POTemplate).execute(

=== modified file 'lib/lp/translations/model/translationmessage.py'
--- lib/lp/translations/model/translationmessage.py	2014-06-13 07:43:41 +0000
+++ lib/lp/translations/model/translationmessage.py	2014-07-02 06:12:24 +0000
@@ -22,9 +22,18 @@
 from storm.expr import And
 from storm.locals import SQL
 from storm.store import Store
+from zope.component import getUtility
 from zope.interface import implements
+from zope.security.proxy import removeSecurityProxy
 
-from lp.registry.interfaces.person import validate_public_person
+from lp.registry.interfaces.person import (
+    IPersonSet,
+    validate_public_person,
+    )
+from lp.services.database.bulk import (
+    load,
+    load_related,
+    )
 from lp.services.database.constants import (
     DEFAULT,
     UTC_NOW,
@@ -41,6 +50,7 @@
     cachedproperty,
     get_property_cache,
     )
+from lp.translations.interfaces.potemplate import IPOTemplateSet
 from lp.translations.interfaces.side import TranslationSide
 from lp.translations.interfaces.translationmessage import (
     ITranslationMessage,
@@ -49,6 +59,7 @@
     TranslationValidationStatus,
     )
 from lp.translations.interfaces.translations import TranslationConstants
+from lp.translations.model.potranslation import POTranslation
 
 
 UTC = pytz.timezone('UTC')
@@ -113,10 +124,13 @@
             elements.append(suffix)
         return self.potmsgset.makeHTMLID('_'.join(elements))
 
-    def setPOFile(self, pofile):
+    def setPOFile(self, pofile, sequence=None):
         """See `ITranslationMessage`."""
         self.browser_pofile = pofile
-        del get_property_cache(self).sequence
+        if sequence is not None:
+            get_property_cache(self).sequence = sequence
+        else:
+            del get_property_cache(self).sequence
 
     @cachedproperty
     def sequence(self):
@@ -396,7 +410,7 @@
               """(SELECT potemplate
                     FROM TranslationTemplateItem
                     WHERE potmsgset = %s AND sequence > 0
-                    LIMIT 1)""" % sqlvalues(self.potmsgset)),
+                    LIMIT 1)""" % sqlvalues(self.potmsgsetID)),
             POFile.language == self.language).one()
         return pofile
 
@@ -530,3 +544,67 @@
     def selectDirect(self, where=None, order_by=None):
         """See `ITranslationMessageSet`."""
         return TranslationMessage.select(where, orderBy=order_by)
+
+    def preloadDetails(self, messages, pofile=None, need_pofile=False,
+                       need_potemplate=False, need_potemplate_context=False,
+                       need_potranslation=False, need_potmsgset=False,
+                       need_people=False):
+        """See `ITranslationMessageSet`."""
+        from lp.translations.model.potemplate import POTemplate
+        from lp.translations.model.potmsgset import POTMsgSet
+        assert need_pofile or not need_potemplate
+        assert need_potemplate or not need_potemplate_context
+        tms = [removeSecurityProxy(tm) for tm in messages]
+        if need_pofile:
+            self.preloadPOFilesAndSequences(tms, pofile)
+        if need_potemplate:
+            pofiles = [tm.browser_pofile for tm in tms]
+            pots = load_related(
+                POTemplate,
+                (removeSecurityProxy(pofile) for pofile in pofiles),
+                ['potemplateID'])
+        if need_potemplate_context:
+            getUtility(IPOTemplateSet).preloadPOTemplateContexts(pots)
+        if need_potranslation:
+            load_related(
+                POTranslation, tms,
+                ['msgstr%dID' % form
+                 for form in xrange(TranslationConstants.MAX_PLURAL_FORMS)])
+        if need_potmsgset:
+            load_related(POTMsgSet, tms, ['potmsgsetID'])
+        if need_people:
+            list(getUtility(IPersonSet).getPrecachedPersonsFromIDs(
+                [tm.submitterID for tm in tms]
+                + [tm.reviewerID for tm in tms]))
+
+    def preloadPOFilesAndSequences(self, messages, pofile=None):
+        """See `ITranslationMessageSet`."""
+        from lp.translations.model.pofile import POFile
+        from lp.translations.model.translationtemplateitem import (
+            TranslationTemplateItem,
+            )
+
+        if len(messages) == 0:
+            return
+        language = messages[0].language
+        if pofile is not None:
+            pofile_constraints = [POFile.id == pofile.id]
+        else:
+            pofile_constraints = [POFile.language == language]
+        results = IStore(POFile).find(
+            (TranslationTemplateItem.potmsgsetID, POFile.id,
+             TranslationTemplateItem.sequence),
+            TranslationTemplateItem.potmsgsetID.is_in(
+                message.potmsgsetID for message in messages),
+            POFile.potemplateID == TranslationTemplateItem.potemplateID,
+            *pofile_constraints
+            ).config(distinct=(TranslationTemplateItem.potmsgsetID,))
+        potmsgset_map = dict(
+            (potmsgset_id, (pofile_id, sequence))
+            for potmsgset_id, pofile_id, sequence in results)
+        load(POFile, (pofile_id for pofile_id, _ in potmsgset_map.values()))
+        for message in messages:
+            assert message.language == language
+            pofile_id, sequence = potmsgset_map.get(
+                message.potmsgsetID, (None, None))
+            message.setPOFile(IStore(POFile).get(POFile, pofile_id), sequence)