Performance improvement during property writes

- Touch node and copy node caches *before* writing updated cache entry
 - Added concurrency tests for aspect updates


git-svn-id: https://svn.alfresco.com/repos/alfresco-enterprise/alfresco/HEAD/root@31446 c4b6b30b-aa2e-2d43-bbcb-ca4b014f7261

source/java/org/alfresco/repo/domain/node/AbstractNodeDAOImpl.java

@@ -1606,7 +1606,16 @@ public abstract class AbstractNodeDAOImpl implements NodeDAO, BatchingDAO
         }
         
         // The node is remaining in the current store
-        int count = updateNode(nodeUpdate);
+        int count = 0;
+        Throwable concurrencyException = null;
+        try
+        {
+            count = updateNode(nodeUpdate);
+        }
+        catch (Throwable e)
+        {
+            concurrencyException = e;
+        }
         // Do concurrency check
         if (count != 1)
         {
@@ -1614,7 +1623,7 @@ public abstract class AbstractNodeDAOImpl implements NodeDAO, BatchingDAO
             nodesCache.removeByKey(nodeId);
             nodesCache.removeByValue(nodeUpdate);
             
-            throw new ConcurrencyFailureException("Failed to update node " + nodeId);
+            throw new ConcurrencyFailureException("Failed to update node " + nodeId, concurrencyException);
         }
         else
         {
@@ -1622,7 +1631,6 @@ public abstract class AbstractNodeDAOImpl implements NodeDAO, BatchingDAO
             nodeUpdate.lock();
             nodesCache.setValue(nodeId, nodeUpdate);
             // The node's version has moved on so no need to invalidate caches
-            // TODO: Should we copy values between the cache keys?
         }
 
         // Done
@@ -1969,7 +1977,18 @@ public abstract class AbstractNodeDAOImpl implements NodeDAO, BatchingDAO
         boolean modifyProps = propsToDelete.size() > 0 || propsToAdd.size() > 0;
         boolean updated = modifyProps || nodeUpdate.isUpdateAnything();
         
-        // Touch to bring into current txn
+        // Now we know that the node needs modification or not; grab the lock
+        if (nodeUpdate.isUpdateAnything())
+        {
+            // We have to explicitly update the node (sys:locale or cm:auditable)
+            updateNodeImpl(node, nodeUpdate);
+        }
+        else if (modifyProps)
+        {
+            // Touch the node; all caches are fine
+            touchNode(nodeId, null, false, false, false);
+        }
+        
         if (modifyProps)
         {
             // Clean up content properties
@@ -2032,12 +2051,6 @@ public abstract class AbstractNodeDAOImpl implements NodeDAO, BatchingDAO
             // Update cache
             setNodePropertiesCached(nodeId, propsToCache);
         }
-        // Touch to bring into current transaction
-        if (updated)
-        {
-            // We have to explicitly update the node (sys:locale or cm:auditable)
-            updateNodeImpl(node, nodeUpdate);
-        }
         
         // Done
         if (isDebugEnabled && updated)
@@ -2099,12 +2112,12 @@ public abstract class AbstractNodeDAOImpl implements NodeDAO, BatchingDAO
 
         if (deleteCount > 0)
         {
+            // Touch the node; all caches are fine
+            touchNode(nodeId, null, false, false, false);
             // Update cache
             Map<QName, Serializable> cachedProps = getNodePropertiesCached(nodeId);
             cachedProps.keySet().removeAll(propertyQNames);
             setNodePropertiesCached(nodeId, cachedProps);
-            // Touch the node; all caches are fine
-            touchNode(nodeId, null, false, false, false);
         }
         // Done
         return deleteCount > 0;
@@ -2333,11 +2346,7 @@ public abstract class AbstractNodeDAOImpl implements NodeDAO, BatchingDAO
             executeBatch();
         }
         
-        // Manually update the cache
+        // Bring the node into the transaction
-        Set<QName> newAspectQNames = new HashSet<QName>(existingAspectQNames);
-        newAspectQNames.addAll(aspectQNamesToAdd);
-        setNodeAspectsCached(nodeId, newAspectQNames);
-        
         if (aspectQNamesToAdd.contains(ContentModel.ASPECT_ROOT))
         {
             // This is a special case.  The presence of the aspect affects the path
@@ -2350,6 +2359,11 @@ public abstract class AbstractNodeDAOImpl implements NodeDAO, BatchingDAO
             touchNode(nodeId, null, false, false, false);
         }
         
+        // Manually update the cache
+        Set<QName> newAspectQNames = new HashSet<QName>(existingAspectQNames);
+        newAspectQNames.addAll(aspectQNamesToAdd);
+        setNodeAspectsCached(nodeId, newAspectQNames);
+
         // Done
         return true;
     }
@@ -2746,13 +2760,6 @@ public abstract class AbstractNodeDAOImpl implements NodeDAO, BatchingDAO
         // Persist it
         assoc.setId(assocId);
         
-        // Primary associations accompany new nodes, so we only have to bring the
-        // node into the current transaction for secondary associations
-        if (!isPrimary)
-        {
-            updateNode(childNodeId, null, null);
-        }
-        
         // Done
         if (isDebugEnabled)
         {

source/java/org/alfresco/repo/node/BaseNodeServiceTest_model.xml

@@ -468,6 +468,27 @@
          </properties>
       </aspect>
             
+      <aspect name="test:Thread-0-0" />
+      <aspect name="test:Thread-0-1" />
+      <aspect name="test:Thread-0-2" />
+      <aspect name="test:Thread-0-3" />
+      <aspect name="test:Thread-0-4" />
+      <aspect name="test:Thread-0-5" />
+      <aspect name="test:Thread-0-6" />
+      <aspect name="test:Thread-0-7" />
+      <aspect name="test:Thread-0-8" />
+      <aspect name="test:Thread-0-9" />
+      <aspect name="test:Thread-1-0" />
+      <aspect name="test:Thread-1-1" />
+      <aspect name="test:Thread-1-2" />
+      <aspect name="test:Thread-1-3" />
+      <aspect name="test:Thread-1-4" />
+      <aspect name="test:Thread-1-5" />
+      <aspect name="test:Thread-1-6" />
+      <aspect name="test:Thread-1-7" />
+      <aspect name="test:Thread-1-8" />
+      <aspect name="test:Thread-1-9" />
+            
    </aspects>
 
 </model>

source/java/org/alfresco/repo/node/ConcurrentNodeServiceTest.java

@@ -19,35 +19,25 @@
 package org.alfresco.repo.node;
 
 import java.io.InputStream;
+import java.io.Serializable;
 import java.util.ArrayList;
-import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
-
+import java.util.Set;
-import javax.transaction.UserTransaction;
 
 import junit.framework.TestCase;
 
-import org.alfresco.model.ContentModel;
+import org.alfresco.repo.cache.SimpleCache;
 import org.alfresco.repo.dictionary.DictionaryDAO;
 import org.alfresco.repo.dictionary.M2Model;
-import org.alfresco.repo.search.impl.lucene.fts.FullTextSearchIndexer;
 import org.alfresco.repo.security.authentication.AuthenticationComponent;
 import org.alfresco.repo.security.authentication.AuthenticationUtil;
-import org.alfresco.repo.tagging.TaggingServiceImpl;
 import org.alfresco.repo.transaction.RetryingTransactionHelper;
 import org.alfresco.repo.transaction.RetryingTransactionHelper.RetryingTransactionCallback;
 import org.alfresco.service.ServiceRegistry;
-import org.alfresco.service.cmr.repository.ChildAssociationRef;
-import org.alfresco.service.cmr.repository.MLText;
 import org.alfresco.service.cmr.repository.NodeRef;
 import org.alfresco.service.cmr.repository.NodeService;
 import org.alfresco.service.cmr.repository.StoreRef;
-import org.alfresco.service.cmr.search.ResultSet;
-import org.alfresco.service.cmr.search.SearchService;
-import org.alfresco.service.namespace.DynamicNamespacePrefixResolver;
-import org.alfresco.service.namespace.NamespacePrefixResolver;
-import org.alfresco.service.namespace.NamespaceService;
 import org.alfresco.service.namespace.QName;
 import org.alfresco.service.transaction.TransactionService;
 import org.alfresco.util.ApplicationContextHelper;
@@ -79,7 +69,6 @@ public class ConcurrentNodeServiceTest extends TestCase
 
     private NodeService nodeService;
     private TransactionService transactionService;
-    private RetryingTransactionHelper retryingTransactionHelper;
     private NodeRef rootNodeRef;
     private AuthenticationComponent authenticationComponent;
 
@@ -103,10 +92,10 @@ public class ConcurrentNodeServiceTest extends TestCase
         model = M2Model.createModel(modelStream);
         dictionaryDao.putModel(model);
 
-        nodeService = (NodeService) ctx.getBean("dbNodeService");
+        ServiceRegistry serviceRegistry = (ServiceRegistry) ctx.getBean(ServiceRegistry.SERVICE_REGISTRY);
-        transactionService = (TransactionService) ctx.getBean("transactionComponent");
+        nodeService = serviceRegistry.getNodeService();
-        retryingTransactionHelper = (RetryingTransactionHelper) ctx.getBean("retryingTransactionHelper");
+        transactionService = serviceRegistry.getTransactionService();
-        this.authenticationComponent = (AuthenticationComponent) ctx.getBean("authenticationComponent");
+        authenticationComponent = (AuthenticationComponent) ctx.getBean("authenticationComponent");
 
         this.authenticationComponent.setSystemUserAsCurrentUser();
 
@@ -121,7 +110,7 @@ public class ConcurrentNodeServiceTest extends TestCase
                 return null;
             }
         };
-        retryingTransactionHelper.doInTransaction(createRootNodeCallback);
+        transactionService.getRetryingTransactionHelper().doInTransaction(createRootNodeCallback);
     }
 
     @Override
@@ -131,184 +120,6 @@ public class ConcurrentNodeServiceTest extends TestCase
         super.tearDown();
     }
     
-    protected Map<QName, ChildAssociationRef> buildNodeGraph() throws Exception
-    {
-        return BaseNodeServiceTest.buildNodeGraph(nodeService, rootNodeRef);
-    }
-
-    protected Map<QName, ChildAssociationRef> commitNodeGraph() throws Exception
-    {
-        RetryingTransactionCallback<Map<QName, ChildAssociationRef>> buildGraphCallback =
-            new RetryingTransactionCallback<Map<QName, ChildAssociationRef>>()
-        {
-            public Map<QName, ChildAssociationRef> execute() throws Exception
-            {
-
-                Map<QName, ChildAssociationRef> answer = buildNodeGraph();
-                return answer;
-            }
-        };
-        return retryingTransactionHelper.doInTransaction(buildGraphCallback);
-    }
-
-    public void xtest1() throws Exception
-    {
-        testConcurrent();
-    }
-
-    public void xtest2() throws Exception
-    {
-        testConcurrent();
-    }
-
-    public void xtest3() throws Exception
-    {
-        testConcurrent();
-    }
-
-    public void xtest4() throws Exception
-    {
-        testConcurrent();
-    }
-
-    public void xtest5() throws Exception
-    {
-        testConcurrent();
-    }
-
-    public void xtest6() throws Exception
-    {
-        testConcurrent();
-    }
-
-    public void xtest7() throws Exception
-    {
-        testConcurrent();
-    }
-
-    public void xtest8() throws Exception
-    {
-        testConcurrent();
-    }
-
-    public void xtest9() throws Exception
-    {
-        testConcurrent();
-    }
-
-    public void xtest10() throws Exception
-    {
-        testConcurrent();
-    }
-
-    public void testConcurrent() throws Exception
-    {
-        Map<QName, ChildAssociationRef> assocRefs = commitNodeGraph();
-        Thread runner = null;
-
-        for (int i = 0; i < COUNT; i++)
-        {
-            runner = new Nester("Concurrent-" + i, runner, REPEATS);
-        }
-        if (runner != null)
-        {
-            runner.start();
-
-            try
-            {
-                runner.join();
-                System.out.println("Query thread has waited for " + runner.getName());
-            }
-            catch (InterruptedException e)
-            {
-                e.printStackTrace();
-            }
-        }
-
-        /*
-         * Builds a graph of child associations as follows:
-         * <pre>
-         * Level 0:     root
-         * Level 1:     root_p_n1   root_p_n2
-         * Level 2:     n1_p_n3     n2_p_n4     n1_n4       n2_p_n5     n1_n8
-         * Level 3:     n3_p_n6     n4_n6       n5_p_n7
-         * Level 4:     n6_p_n8     n7_n8
-         * </pre>
-         */
-        RetryingTransactionCallback<Object> testCallback = new RetryingTransactionCallback<Object>()
-        {
-            public Object execute() throws Exception
-            {
-                // There are two nodes at the base level in each test
-                assertEquals(2 * ((COUNT * REPEATS) + 1), nodeService.getChildAssocs(rootNodeRef).size());
-
-                SearchService searcher = (SearchService) ctx.getBean(ServiceRegistry.SEARCH_SERVICE.getLocalName());
-                assertEquals(
-                        2 * ((COUNT * REPEATS) + 1),
-                        searcher.selectNodes(rootNodeRef, "/*", null, getNamespacePrefixResolver(""), false).size());
-
-                return null;
-            }
-        };
-        retryingTransactionHelper.doInTransaction(testCallback);
-    }
-
-    /**
-     * Daemon thread
-     */
-    private class Nester extends Thread
-    {
-        Thread waiter;
-
-        int repeats;
-
-        Nester(String name, Thread waiter, int repeats)
-        {
-            super(name);
-            this.setDaemon(true);
-            this.waiter = waiter;
-            this.repeats = repeats;
-        }
-
-        public void run()
-        {
-            authenticationComponent.setSystemUserAsCurrentUser();
-
-            if (waiter != null)
-            {
-                System.out.println("Starting " + waiter.getName());
-                waiter.start();
-            }
-            try
-            {
-                System.out.println("Start " + this.getName());
-                for (int i = 0; i < repeats; i++)
-                {
-                    Map<QName, ChildAssociationRef> assocRefs = commitNodeGraph();
-                    System.out.println(" " + this.getName() + " " + i);
-                }
-                System.out.println("End " + this.getName());
-            }
-            catch (Exception e)
-            {
-                e.printStackTrace();
-            }
-            if (waiter != null)
-            {
-                try
-                {
-                    waiter.join();
-                    System.out.println("Thread "
-                            + this.getName() + " has waited for " + (waiter == null ? "null" : waiter.getName()));
-                }
-                catch (InterruptedException e)
-                {
-                    System.err.println(e);
-                }
-            }
-        }
-    }
-    
     /**
      * Tests that when multiple threads try to edit different
      *  properties on a node, that transactions + retries always
@@ -316,7 +127,7 @@ public class ConcurrentNodeServiceTest extends TestCase
      *  
      * @since 3.4 
      */
-    public void testMultiThreadedNodePropertiesWrites() throws Exception
+    public void testMultiThreaded_PropertyWrites() throws Exception
     {
         final List<Thread> threads = new ArrayList<Thread>();
         final int loops = 200;
@@ -328,11 +139,13 @@ public class ConcurrentNodeServiceTest extends TestCase
                 QName.createQName("test2", "MadeUp2"),
                 QName.createQName("test3", "MadeUp3"),
                 QName.createQName("test4", "MadeUp4"),
-                QName.createQName("test5", "MadeUp5")
+                QName.createQName("test4", "MadeUp5"),
         };
-        for(QName prop : properties)
+        final int[] propCounts = new int[properties.length];
+        for (int propNum = 0; propNum < properties.length; propNum++)
         {
-            final QName property = prop;
+            final QName property = properties[propNum];
+            final int propNumberFinal = propNum;
 
             // Zap the property if it is there
             transactionService.getRetryingTransactionHelper().doInTransaction(
@@ -364,7 +177,7 @@ public class ConcurrentNodeServiceTest extends TestCase
 
                     // Loop, incrementing each time
                     // If we miss an update, then at the end it'll be obvious
-                    AuthenticationUtil.setFullyAuthenticatedUser(AuthenticationUtil.getSystemUserName());
+                    AuthenticationUtil.setRunAsUserSystem();
                     for (int i = 0; i < loops; i++)
                     {
                         RetryingTransactionCallback<Integer> callback = new RetryingTransactionCallback<Integer>()
@@ -381,14 +194,21 @@ public class ConcurrentNodeServiceTest extends TestCase
                                 }
                                 // Increment by one. Really should be this!
                                 current++;
-                                // Save the new value
                                 nodeService.setProperty(rootNodeRef, property, Integer.valueOf(current));
                                 return current;
                             }
                         };
+                        try
+                        {
                             RetryingTransactionHelper txnHelper = transactionService.getRetryingTransactionHelper();
                             txnHelper.setMaxRetries(loops);
-                        txnHelper.doInTransaction(callback, false, true);
+                            Integer newCount = txnHelper.doInTransaction(callback, false, true);
+                            System.out.println("Set value: " + Thread.currentThread().getName() + " " + newCount);
+                        }
+                        catch (Throwable e)
+                        {
+                            logger.error("Failed to set value: ", e);
+                        }
                     }
 
                     // Report us as finished
@@ -412,36 +232,30 @@ public class ConcurrentNodeServiceTest extends TestCase
             t.join();
         }
         
-        // Check each property in turn
+        // Force a clear to see if we are seeing old values
-        RetryingTransactionCallback<Void> checkCallback = new RetryingTransactionCallback<Void>()
+        SimpleCache nodesCache = (SimpleCache) ctx.getBean("node.nodesSharedCache");
-        {
+        nodesCache.clear();
-            @Override
+        SimpleCache propsCache = (SimpleCache) ctx.getBean("node.propertiesSharedCache");
-            public Void execute() throws Throwable
+        propsCache.clear();
-            {
-                HashMap<QName, Integer> values = new HashMap<QName, Integer>();
-                for(QName prop : properties)
-                {
-                    Object val = nodeService.getProperty(rootNodeRef, prop);
-                    Integer value = -1;
-                    if(val instanceof MLText)
-                    {
-                        value = Integer.valueOf( ((MLText)val).getValues().iterator().next() );
-                    }
-                    else
-                    {
-                        value = (Integer)val;
-                    }
-                    
-                    values.put(prop,value);
-                }
 
+        Map<QName, Serializable> nodeProperties = nodeService.getProperties(rootNodeRef);
         List<String> errors = new ArrayList<String>();
-                for(QName prop : properties)
+        for (int i =0; i < properties.length; i++)
         {
-                    Integer value = values.get(prop);
+            Integer value = (Integer) nodeProperties.get(properties[i]);
-                    if (value == null || !value.equals(new Integer(loops)))
+            if (value == null)
             {
-                        errors.add("\n   Prop " + prop + " : " + value);
+                errors.add("\n   Prop " + properties[i] + " : " + value);
+            }
+            if (!value.equals(new Integer(loops)))
+            {
+                // Check against the number of loops actually executed
+                if (propCounts[i] == value.intValue())
+                {
+                    System.out.println("Actually, not all counts done.");
+                }
+                errors.add("\n   Prop " + properties[i] + " : " + value);
+            }
         }
         if (errors.size() > 0)
         {
@@ -454,20 +268,65 @@ public class ConcurrentNodeServiceTest extends TestCase
             fail(sb.toString());
         }
     }
+    
+    /**
+     * Adds 'residual' aspects that are named according to the thread.  Multiple threads should all
+     * get their changes in. 
+     */
+    public void testMultithreaded_AspectWrites() throws Exception
+    {
+        final Thread[] threads = new Thread[2];
+        final int loops = 10;
+        
+        for (int i = 0; i < threads.length; i++)
+        {
+            final String name = "Thread-" + i + "-";
+            Runnable runnable = new Runnable()
+            {
+                @Override
+                public void run()
+                {
+                    AuthenticationUtil.setRunAsUserSystem();
+                    for (int loop = 0; loop < loops; loop++)
+                    {
+                        final String nameWithLoop = name + loop;
+                        RetryingTransactionCallback<Void> runCallback = new RetryingTransactionCallback<Void>()
+                        {
+                            @Override
+                            public Void execute() throws Throwable
+                            {
+                                // Add another aspect to the node
+                                QName qname = QName.createQName(NAMESPACE, nameWithLoop);
+                                nodeService.addAspect(rootNodeRef, qname, null);
                                 return null;
                             }
                         };
-        transactionService.getRetryingTransactionHelper().doInTransaction(checkCallback, true);
+                        transactionService.getRetryingTransactionHelper().doInTransaction(runCallback);
                     }
-
+                }
-    private NamespacePrefixResolver getNamespacePrefixResolver(String defaultURI)
+            };
+            threads[i] = new Thread(runnable, name);
+        }
+        // Start all the threads
+        for (int i = 0; i < threads.length; i++)
         {
-        DynamicNamespacePrefixResolver nspr = new DynamicNamespacePrefixResolver(null);
+            threads[i].start();
-        nspr.registerNamespace(NamespaceService.SYSTEM_MODEL_PREFIX, NamespaceService.SYSTEM_MODEL_1_0_URI);
+        }
-        nspr.registerNamespace(NamespaceService.CONTENT_MODEL_PREFIX, NamespaceService.CONTENT_MODEL_1_0_URI);
+        // Wait for them all to finish
-        nspr.registerNamespace(NamespaceService.APP_MODEL_PREFIX, NamespaceService.APP_MODEL_1_0_URI);
+        for (int i = 0; i < threads.length; i++)
-        nspr.registerNamespace("namespace", "namespace");
+        {
-        nspr.registerNamespace(NamespaceService.DEFAULT_PREFIX, defaultURI);
+            threads[i].join();
-        return nspr;
+        }
+        // Check the aspects
+        Set<QName> aspects = nodeService.getAspects(rootNodeRef);
+        for (int i = 0; i < threads.length; i++)
+        {
+            for (int j = 0; j < loops; j++)
+            {
+                String nameWithLoop = "Thread-" + i + "-" + j;
+                QName qname = QName.createQName(NAMESPACE, nameWithLoop);
+                assertTrue("Missing aspect: "+ nameWithLoop, aspects.contains(qname));
+            }
+        }
     }
 }