rhbz1120034 - improve push performance

- move queries outside of loop
- eager fetch needed entities to avoid n + 1
- use named query

zanata-model/src/main/java/org/zanata/model/HLocale.java

@@ -46,6 +46,7 @@
 import org.zanata.common.LocaleId;
 import org.zanata.model.type.LocaleIdType;
 
+import com.google.common.annotations.VisibleForTesting;
 import com.ibm.icu.util.ULocale;
 
 @Entity
@@ -70,6 +71,14 @@ public HLocale(@Nonnull LocaleId localeId) {
         this.localeId = localeId;
     }
 
+    @VisibleForTesting
+    public HLocale(@Nonnull LocaleId localeId, boolean enabledByDefault,
+            boolean active) {
+        this.localeId = localeId;
+        this.enabledByDefault = enabledByDefault;
+        this.active = active;
+    }
+
     // TODO PERF @NaturalId(mutable=false) for better criteria caching
     @SuppressWarnings("null")
     @NaturalId

zanata-model/src/main/java/org/zanata/model/HTextFlow.java

@@ -39,6 +39,8 @@
 import javax.persistence.ManyToOne;
 import javax.persistence.MapKey;
 import javax.persistence.MapKeyColumn;
+import javax.persistence.NamedQueries;
+import javax.persistence.NamedQuery;
 import javax.persistence.OneToMany;
 import javax.persistence.OneToOne;
 import javax.persistence.PostLoad;
@@ -93,8 +95,14 @@
 @NoArgsConstructor
 @ToString(of = { "resId", "revision", "comment", "obsolete" })
 @Slf4j
+@NamedQueries({
+        @NamedQuery(
+                name = HTextFlow.QUERY_GET_BY_DOC_AND_RES_ID_BATCH,
+                query = "from HTextFlow tf left join fetch tf.targets tft left join fetch tft.history where tf.document = :document and tf.resId in (:resIds)")
+})
 public class HTextFlow extends HTextContainer implements Serializable,
         ITextFlowHistory, HasSimpleComment, HasContents, ITextFlow {
+    public static final String QUERY_GET_BY_DOC_AND_RES_ID_BATCH = "HTextFlow.getByDocumentAndResIdBatch";
     private static final long serialVersionUID = 3023080107971905435L;
 
     private Long id;
@@ -229,12 +237,12 @@ public void setObsolete(boolean obsolete) {
     @JoinColumn(name = "document_id", insertable = false, updatable = false,
             nullable = false)
     // TODO PERF @NaturalId(mutable=false) for better criteria caching
-            @NaturalId
-            @AccessType("field")
-            @Field(analyze = Analyze.NO)
-            @FieldBridge(impl = ContainingWorkspaceBridge.class)
-            public
-            HDocument getDocument() {
+    @NaturalId
+    @AccessType("field")
+    @Field(analyze = Analyze.NO)
+    @FieldBridge(impl = ContainingWorkspaceBridge.class)
+    public
+    HDocument getDocument() {
         return document;
     }
 

zanata-war/pom.xml

@@ -2062,7 +2062,7 @@
       <groupId>com.jamonapi</groupId>
       <artifactId>jamon</artifactId>
       <version>2.75</version>
-      <scope>test</scope>
+      <scope>provided</scope>
     </dependency>
 
     <!--  missing dependencies identified by mvn dependency:analyze: -->

zanata-war/src/main/java/org/zanata/dao/TextFlowDAO.java

@@ -21,7 +21,9 @@
 package org.zanata.dao;
 
 import java.util.ArrayList;
+import java.util.LinkedList;
 import java.util.List;
+import java.util.Map;
 
 import org.hibernate.Query;
 import org.hibernate.Session;
@@ -37,6 +39,9 @@
 import org.zanata.search.FilterConstraintToQuery;
 import org.zanata.search.FilterConstraints;
 import org.zanata.webtrans.shared.model.DocumentId;
+import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Lists;
 import lombok.extern.slf4j.Slf4j;
 
 @Name("textFlowDAO")
@@ -234,4 +239,39 @@ public long countActiveTextFlowsInProjectIteration(Long versionId) {
                 "TextFlowDAO.countTextFlowsInProjectIteration");
         return (Long) q.uniqueResult();
     }
+
+    /**
+     * This will eagerly fetch HTextFlow properties including targets and target
+     * histories. It will use a literal list in IN clause. Mysql has no limit on
+     * In clause itself but performance wide, IN is only faster when the number
+     * is relatively small i.e. less than 500. We are using this query in
+     * translation batch update which is 100 per batch.
+     *
+     * @see org.zanata.model.HTextFlow
+     * @see <a href="http://stackoverflow.com/a/1532454/345718">MySQL number of
+     *      items within “in clause”/a>
+     *
+     * @param document
+     *            document
+     * @param resIds
+     *            resID list
+     * @return a map with HTextFlow resId as key and HTextFlow as value
+     */
+    public Map<String, HTextFlow> getByDocumentAndResIds(HDocument document,
+            List<String> resIds) {
+        Query query = getSession()
+                .getNamedQuery(HTextFlow.QUERY_GET_BY_DOC_AND_RES_ID_BATCH)
+                .setParameter("document", document)
+                .setParameterList("resIds", resIds)
+                .setComment("TextFlowDAO.getByDocumentAndResIds");
+        @SuppressWarnings("unchecked")
+        List<HTextFlow> results = query.list();
+        ImmutableMap.Builder<String, HTextFlow> builder = ImmutableMap.builder();
+        for (HTextFlow textFlow : results) {
+            builder.put(textFlow.getResId(), textFlow);
+        }
+
+        return builder.build();
+    }
+
 }

zanata-war/src/main/java/org/zanata/service/impl/TranslationServiceImpl.java

@@ -25,6 +25,7 @@
 import java.util.Collection;
 import java.util.HashSet;
 import java.util.List;
+import java.util.Map;
 import java.util.Set;
 
 import javax.annotation.Nonnull;
@@ -86,6 +87,7 @@
 import org.zanata.webtrans.shared.model.TransUnitUpdateRequest;
 import org.zanata.webtrans.shared.model.ValidationAction;
 
+import com.google.common.base.Function;
 import com.google.common.base.Optional;
 import com.google.common.base.Predicate;
 import com.google.common.collect.Iterables;
@@ -715,10 +717,25 @@ private final class SaveBatchWork extends Work<Boolean> {
         protected Boolean work() throws Exception {
             boolean changed = false;
 
+            // we need a fresh object in this session,
+            // so that it can lazily load associated objects
+            textFlowTargetDAO.clear();
+            HProjectIteration iterationReloaded =
+                    projectIterationDAO.findById(projectIterationId);
+            Map<String, HTextFlow> resIdToTextFlowMap = textFlowDAO.getByDocumentAndResIds(document, Lists.transform(
+                    batch, new Function<TextFlowTarget, String>() {
+
+                        @Override
+                        public String apply(TextFlowTarget input) {
+                            return input.getResId();
+                        }
+                    }));
+            final int numPlurals = resourceUtils
+                    .getNumPlurals(document, locale);
             for (TextFlowTarget incomingTarget : batch) {
                 String resId = incomingTarget.getResId();
                 String sourceHash = incomingTarget.getSourceHash();
-                HTextFlow textFlow = textFlowDAO.getById(document, resId);
+                HTextFlow textFlow = resIdToTextFlowMap.get(resId);
                 if (textFlow == null) {
                     // return warning for unknown resId to caller
                     String warning =
@@ -733,8 +750,8 @@ protected Boolean work() throws Exception {
                     String warning =
                             MessageFormat
                                     .format("TextFlowTarget {0} may be obsolete; "
-                                            + "associated source hash: {1}; "
-                                            + "expected hash is {2} for source: {3}",
+                                                    + "associated source hash: {1}; "
+                                                    + "expected hash is {2} for source: {3}",
                                             resId, sourceHash,
                                             textFlow.getContentHash(),
                                             textFlow.getContents());
@@ -743,10 +760,6 @@ protected Boolean work() throws Exception {
                             "skipping TextFlowTarget {} with unknown sourceHash: {}",
                             resId, sourceHash);
                 } else {
-                    // we need a fresh object in this session,
-                    // so that it can lazily load associated objects
-                    HProjectIteration iterationReloaded =
-                            projectIterationDAO.findById(projectIterationId);
                     String validationMessage =
                             validateTranslations(incomingTarget.getState(),
                                     iterationReloaded,
@@ -760,11 +773,9 @@ protected Boolean work() throws Exception {
                         continue;
                     }
 
-                    int nPlurals = getNumPlurals(locale, textFlow);
-                    HTextFlowTarget hTarget =
-                            textFlowTargetDAO.getTextFlowTarget(textFlow,
-                                    locale);
-
+                    int nPlurals = textFlow.isPlural() ? numPlurals : 1;
+                    // we have eagerly loaded all targets upfront
+                    HTextFlowTarget hTarget = textFlow.getTargets().get(locale.getId());
                     ContentState currentState = ContentState.New;
                     if (hTarget != null) {
                         currentState = hTarget.getState();
@@ -822,17 +833,15 @@ protected Boolean work() throws Exception {
                     }
                 }
 
-                personDAO.flush();
-                textFlowTargetDAO.flush();
-                personDAO.clear();
-                textFlowTargetDAO.clear();
                 if (handleOp.isPresent()) {
                     handleOp.get().increaseProgress(1);
                 }
             }
+            textFlowTargetDAO.flush();
 
             return changed;
         }
+
     }
 
     public static class TranslationResultImpl implements TranslationResult {

zanata-war/src/test/java/org/zanata/model/DocumentJPATest.java

@@ -11,6 +11,7 @@
 import javax.persistence.EntityManager;
 
 import org.dbunit.operation.DatabaseOperation;
+import org.hamcrest.Matchers;
 import org.hibernate.Session;
 import org.jboss.seam.security.Identity;
 import org.testng.annotations.BeforeClass;
@@ -20,6 +21,8 @@
 import org.zanata.common.ContentType;
 import org.zanata.common.LocaleId;
 import org.zanata.dao.LocaleDAO;
+import com.google.common.base.Function;
+import com.google.common.collect.Lists;
 
 @Test(groups = { "jpa-tests" })
 public class DocumentJPATest extends ZanataDbunitJpaTest {
@@ -72,8 +75,14 @@ public void traverseProjectGraph() throws Exception {
         assertThat("Project should have 3 targets", projectTargets.size(),
                 is(3));
 
-        HProjectIteration target = projectTargets.get(0);
-        assertThat("Expect target with id 1", target.getId(), is(1l));
+        List<Long> iterationIds = Lists.transform(projectTargets,
+                new Function<HProjectIteration, Long>() {
+                    @Override
+                    public Long apply(HProjectIteration input) {
+                        return input.getId();
+                    }
+                });
+        assertThat(iterationIds, Matchers.containsInAnyOrder(1L, 2L, 900L));
     }
 
     @Test

zanata-war/src/test/java/org/zanata/rest/service/ResourceUtilsJpaTest.java

@@ -7,6 +7,7 @@
 import org.testng.annotations.BeforeMethod;
 import org.testng.annotations.Test;
 import org.zanata.PerformanceProfiling;
+import org.zanata.SlowTest;
 import org.zanata.ZanataJpaTest;
 import org.zanata.common.ContentType;
 import org.zanata.common.LocaleId;
@@ -65,8 +66,8 @@ void transferFromResourceMetadata() {
         // TODO check the results in 'to'
     }
 
-//    @Test
-//    @SlowTest
+    @Test(enabled = false, description = "This should be executed manually in IDE")
+    @SlowTest
     @PerformanceProfiling
     // ideally change persistence.xml to use a local mysql database and monitor general log etc.
     public void transferFromResource() {