Bug 714256 - Remove locking from the transaction manager; r=roc
authorEhsan Akhgari <ehsan@mozilla.com>
Sun, 01 Jan 2012 17:35:11 -0500
changeset 84857 a4a07b81ad67f647edddc1a87e79b203802e241a
parent 84856 c2c6616eea53345bcfafbfa5f4764bc76bfee198
child 84858 9152791a6f9039f1be5534b1ea5d88dbec21cb20
push id805
push userakeybl@mozilla.com
push dateWed, 01 Feb 2012 18:17:35 +0000
treeherdermozilla-aurora@6fb3bf232436 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersroc
bugs714256
milestone12.0a1
Bug 714256 - Remove locking from the transaction manager; r=roc
editor/txmgr/src/nsTransactionManager.cpp
editor/txmgr/src/nsTransactionManager.h
--- a/editor/txmgr/src/nsTransactionManager.cpp
+++ b/editor/txmgr/src/nsTransactionManager.cpp
@@ -41,33 +41,23 @@
 #include "nsTransactionItem.h"
 #include "nsTransactionStack.h"
 #include "nsVoidArray.h"
 #include "nsTransactionManager.h"
 #include "nsTransactionList.h"
 #include "nsAutoPtr.h"
 #include "nsCOMPtr.h"
 
-#define LOCK_TX_MANAGER(mgr)    (mgr)->Lock()
-#define UNLOCK_TX_MANAGER(mgr)  (mgr)->Unlock()
-
-
 nsTransactionManager::nsTransactionManager(PRInt32 aMaxTransactionCount)
   : mMaxTransactionCount(aMaxTransactionCount)
 {
-  mMonitor = ::PR_NewMonitor();
 }
 
 nsTransactionManager::~nsTransactionManager()
 {
-  if (mMonitor)
-  {
-    ::PR_DestroyMonitor(mMonitor);
-    mMonitor = 0;
-  }
 }
 
 NS_IMPL_CYCLE_COLLECTION_CLASS(nsTransactionManager)
 
 NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(nsTransactionManager)
   NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMARRAY(mListeners)
   tmp->mDoStack.DoUnlink();
   tmp->mUndoStack.DoUnlink();
@@ -92,111 +82,95 @@ NS_IMPL_CYCLE_COLLECTING_RELEASE(nsTrans
 
 NS_IMETHODIMP
 nsTransactionManager::DoTransaction(nsITransaction *aTransaction)
 {
   nsresult result;
 
   NS_ENSURE_TRUE(aTransaction, NS_ERROR_NULL_POINTER);
 
-  LOCK_TX_MANAGER(this);
-
   bool doInterrupt = false;
 
   result = WillDoNotify(aTransaction, &doInterrupt);
 
   if (NS_FAILED(result)) {
-    UNLOCK_TX_MANAGER(this);
     return result;
   }
 
   if (doInterrupt) {
-    UNLOCK_TX_MANAGER(this);
     return NS_OK;
   }
 
   result = BeginTransaction(aTransaction);
 
   if (NS_FAILED(result)) {
     DidDoNotify(aTransaction, result);
-    UNLOCK_TX_MANAGER(this);
     return result;
   }
 
   result = EndTransaction();
 
   nsresult result2 = DidDoNotify(aTransaction, result);
 
   if (NS_SUCCEEDED(result))
     result = result2;
 
-  UNLOCK_TX_MANAGER(this);
-
   return result;
 }
 
 NS_IMETHODIMP
 nsTransactionManager::UndoTransaction()
 {
   nsresult result       = NS_OK;
   nsRefPtr<nsTransactionItem> tx;
 
-  LOCK_TX_MANAGER(this);
-
   // It is illegal to call UndoTransaction() while the transaction manager is
   // executing a  transaction's DoTransaction() method! If this happens,
   // the UndoTransaction() request is ignored, and we return NS_ERROR_FAILURE.
 
   result = mDoStack.Peek(getter_AddRefs(tx));
 
   if (NS_FAILED(result)) {
-    UNLOCK_TX_MANAGER(this);
     return result;
   }
 
   if (tx) {
-    UNLOCK_TX_MANAGER(this);
     return NS_ERROR_FAILURE;
   }
 
   // Peek at the top of the undo stack. Don't remove the transaction
   // until it has successfully completed.
   result = mUndoStack.Peek(getter_AddRefs(tx));
 
   if (NS_FAILED(result)) {
-    UNLOCK_TX_MANAGER(this);
     return result;
   }
 
   // Bail if there's nothing on the stack.
   if (!tx) {
-    UNLOCK_TX_MANAGER(this);
     return NS_OK;
   }
 
   nsCOMPtr<nsITransaction> t;
 
   result = tx->GetTransaction(getter_AddRefs(t));
 
   if (NS_FAILED(result)) {
-    UNLOCK_TX_MANAGER(this);
     return result;
   }
 
   bool doInterrupt = false;
 
   result = WillUndoNotify(t, &doInterrupt);
 
   if (NS_FAILED(result)) {
-    UNLOCK_TX_MANAGER(this);
     return result;
   }
 
   if (doInterrupt) {
-    UNLOCK_TX_MANAGER(this);
     return NS_OK;
   }
 
   result = tx->UndoTransaction(this);
 
   if (NS_SUCCEEDED(result)) {
     result = mUndoStack.Pop(getter_AddRefs(tx));
 
@@ -204,80 +178,69 @@ nsTransactionManager::UndoTransaction()
       result = mRedoStack.Push(tx);
   }
 
   nsresult result2 = DidUndoNotify(t, result);
 
   if (NS_SUCCEEDED(result))
     result = result2;
 
-  UNLOCK_TX_MANAGER(this);
-
   return result;
 }
 
 NS_IMETHODIMP
 nsTransactionManager::RedoTransaction()
 {
   nsresult result       = NS_OK;
   nsRefPtr<nsTransactionItem> tx;
 
-  LOCK_TX_MANAGER(this);
-
   // It is illegal to call RedoTransaction() while the transaction manager is
   // executing a  transaction's DoTransaction() method! If this happens,
   // the RedoTransaction() request is ignored, and we return NS_ERROR_FAILURE.
 
   result = mDoStack.Peek(getter_AddRefs(tx));
 
   if (NS_FAILED(result)) {
-    UNLOCK_TX_MANAGER(this);
     return result;
   }
 
   if (tx) {
-    UNLOCK_TX_MANAGER(this);
     return NS_ERROR_FAILURE;
   }
 
   // Peek at the top of the redo stack. Don't remove the transaction
   // until it has successfully completed.
   result = mRedoStack.Peek(getter_AddRefs(tx));
 
   if (NS_FAILED(result)) {
-    UNLOCK_TX_MANAGER(this);
     return result;
   }
 
   // Bail if there's nothing on the stack.
   if (!tx) {
-    UNLOCK_TX_MANAGER(this);
     return NS_OK;
   }
 
   nsCOMPtr<nsITransaction> t;
 
   result = tx->GetTransaction(getter_AddRefs(t));
 
   if (NS_FAILED(result)) {
-    UNLOCK_TX_MANAGER(this);
     return result;
   }
 
   bool doInterrupt = false;
 
   result = WillRedoNotify(t, &doInterrupt);
 
   if (NS_FAILED(result)) {
-    UNLOCK_TX_MANAGER(this);
     return result;
   }
 
   if (doInterrupt) {
-    UNLOCK_TX_MANAGER(this);
     return NS_OK;
   }
 
   result = tx->RedoTransaction(this);
 
   if (NS_SUCCEEDED(result)) {
     result = mRedoStack.Pop(getter_AddRefs(tx));
 
@@ -285,323 +248,266 @@ nsTransactionManager::RedoTransaction()
       result = mUndoStack.Push(tx);
   }
 
   nsresult result2 = DidRedoNotify(t, result);
 
   if (NS_SUCCEEDED(result))
     result = result2;
 
-  UNLOCK_TX_MANAGER(this);
-
   return result;
 }
 
 NS_IMETHODIMP
 nsTransactionManager::Clear()
 {
   nsresult result;
 
-  LOCK_TX_MANAGER(this);
-
   result = ClearRedoStack();
 
   if (NS_FAILED(result)) {
-    UNLOCK_TX_MANAGER(this);
     return result;
   }
 
   result = ClearUndoStack();
 
-  UNLOCK_TX_MANAGER(this);
-
   return result;
 }
 
 NS_IMETHODIMP
 nsTransactionManager::BeginBatch()
 {
   nsresult result;
 
   // We can batch independent transactions together by simply pushing
   // a dummy transaction item on the do stack. This dummy transaction item
   // will be popped off the do stack, and then pushed on the undo stack
   // in EndBatch().
 
-  LOCK_TX_MANAGER(this);
-
   bool doInterrupt = false;
 
   result = WillBeginBatchNotify(&doInterrupt);
 
   if (NS_FAILED(result)) {
-    UNLOCK_TX_MANAGER(this);
     return result;
   }
 
   if (doInterrupt) {
-    UNLOCK_TX_MANAGER(this);
     return NS_OK;
   }
 
   result = BeginTransaction(0);
   
   nsresult result2 = DidBeginBatchNotify(result);
 
   if (NS_SUCCEEDED(result))
     result = result2;
 
-  UNLOCK_TX_MANAGER(this);
-
   return result;
 }
 
 NS_IMETHODIMP
 nsTransactionManager::EndBatch()
 {
   nsRefPtr<nsTransactionItem> tx;
   nsCOMPtr<nsITransaction> ti;
   nsresult result;
 
-  LOCK_TX_MANAGER(this);
-
   // XXX: Need to add some mechanism to detect the case where the transaction
   //      at the top of the do stack isn't the dummy transaction, so we can
   //      throw an error!! This can happen if someone calls EndBatch() within
   //      the DoTransaction() method of a transaction.
   //
   //      For now, we can detect this case by checking the value of the
   //      dummy transaction's mTransaction field. If it is our dummy
   //      transaction, it should be NULL. This may not be true in the
   //      future when we allow users to execute a transaction when beginning
   //      a batch!!!!
 
   result = mDoStack.Peek(getter_AddRefs(tx));
 
   if (NS_FAILED(result)) {
-    UNLOCK_TX_MANAGER(this);
     return result;
   }
 
   if (tx)
     tx->GetTransaction(getter_AddRefs(ti));
 
   if (!tx || ti) {
-    UNLOCK_TX_MANAGER(this);
     return NS_ERROR_FAILURE;
   }
 
   bool doInterrupt = false;
 
   result = WillEndBatchNotify(&doInterrupt);
 
   if (NS_FAILED(result)) {
-    UNLOCK_TX_MANAGER(this);
     return result;
   }
 
   if (doInterrupt) {
-    UNLOCK_TX_MANAGER(this);
     return NS_OK;
   }
 
   result = EndTransaction();
 
   nsresult result2 = DidEndBatchNotify(result);
 
   if (NS_SUCCEEDED(result))
     result = result2;
 
-  UNLOCK_TX_MANAGER(this);
-
   return result;
 }
 
 NS_IMETHODIMP
 nsTransactionManager::GetNumberOfUndoItems(PRInt32 *aNumItems)
 {
-  nsresult result;
-
-  LOCK_TX_MANAGER(this);
-  result = mUndoStack.GetSize(aNumItems);
-  UNLOCK_TX_MANAGER(this);
-
-  return result;
+  return mUndoStack.GetSize(aNumItems);
 }
 
 NS_IMETHODIMP
 nsTransactionManager::GetNumberOfRedoItems(PRInt32 *aNumItems)
 {
-  nsresult result;
-
-  LOCK_TX_MANAGER(this);
-  result = mRedoStack.GetSize(aNumItems);
-  UNLOCK_TX_MANAGER(this);
-
-  return result;
+  return mRedoStack.GetSize(aNumItems);
 }
 
 NS_IMETHODIMP
 nsTransactionManager::GetMaxTransactionCount(PRInt32 *aMaxCount)
 {
   NS_ENSURE_TRUE(aMaxCount, NS_ERROR_NULL_POINTER);
 
-  LOCK_TX_MANAGER(this);
   *aMaxCount = mMaxTransactionCount;
-  UNLOCK_TX_MANAGER(this);
 
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsTransactionManager::SetMaxTransactionCount(PRInt32 aMaxCount)
 {
   PRInt32 numUndoItems  = 0, numRedoItems = 0, total = 0;
   nsRefPtr<nsTransactionItem> tx;
   nsresult result;
 
-  LOCK_TX_MANAGER(this);
-
   // It is illegal to call SetMaxTransactionCount() while the transaction
   // manager is executing a  transaction's DoTransaction() method because
   // the undo and redo stacks might get pruned! If this happens, the
   // SetMaxTransactionCount() request is ignored, and we return
   // NS_ERROR_FAILURE.
 
   result = mDoStack.Peek(getter_AddRefs(tx));
 
   if (NS_FAILED(result)) {
-    UNLOCK_TX_MANAGER(this);
     return result;
   }
 
   if (tx) {
-    UNLOCK_TX_MANAGER(this);
     return NS_ERROR_FAILURE;
   }
 
   // If aMaxCount is less than zero, the user wants unlimited
   // levels of undo! No need to prune the undo or redo stacks!
 
   if (aMaxCount < 0) {
     mMaxTransactionCount = -1;
-    UNLOCK_TX_MANAGER(this);
     return result;
   }
 
   result = mUndoStack.GetSize(&numUndoItems);
 
   if (NS_FAILED(result)) {
-    UNLOCK_TX_MANAGER(this);
     return result;
   }
 
   result = mRedoStack.GetSize(&numRedoItems);
 
   if (NS_FAILED(result)) {
-    UNLOCK_TX_MANAGER(this);
     return result;
   }
 
   total = numUndoItems + numRedoItems;
 
   // If aMaxCount is greater than the number of transactions that currently
   // exist on the undo and redo stack, there is no need to prune the
   // undo or redo stacks!
 
   if (aMaxCount > total ) {
     mMaxTransactionCount = aMaxCount;
-    UNLOCK_TX_MANAGER(this);
     return result;
   }
 
   // Try getting rid of some transactions on the undo stack! Start at
   // the bottom of the stack and pop towards the top.
 
   while (numUndoItems > 0 && (numRedoItems + numUndoItems) > aMaxCount) {
     result = mUndoStack.PopBottom(getter_AddRefs(tx));
 
     if (NS_FAILED(result) || !tx) {
-      UNLOCK_TX_MANAGER(this);
       return result;
     }
 
     --numUndoItems;
   }
 
   // If necessary, get rid of some transactions on the redo stack! Start at
   // the bottom of the stack and pop towards the top.
 
   while (numRedoItems > 0 && (numRedoItems + numUndoItems) > aMaxCount) {
     result = mRedoStack.PopBottom(getter_AddRefs(tx));
 
     if (NS_FAILED(result) || !tx) {
-      UNLOCK_TX_MANAGER(this);
       return result;
     }
 
     --numRedoItems;
   }
 
   mMaxTransactionCount = aMaxCount;
 
-  UNLOCK_TX_MANAGER(this);
-
   return result;
 }
 
 NS_IMETHODIMP
 nsTransactionManager::PeekUndoStack(nsITransaction **aTransaction)
 {
   nsRefPtr<nsTransactionItem> tx;
   nsresult result;
 
   NS_ENSURE_TRUE(aTransaction, NS_ERROR_NULL_POINTER);
 
   *aTransaction = 0;
 
-  LOCK_TX_MANAGER(this);
-
   result = mUndoStack.Peek(getter_AddRefs(tx));
 
   if (NS_FAILED(result) || !tx) {
-    UNLOCK_TX_MANAGER(this);
     return result;
   }
 
   result = tx->GetTransaction(aTransaction);
 
-  UNLOCK_TX_MANAGER(this);
-
   return result;
 }
 
 NS_IMETHODIMP
 nsTransactionManager::PeekRedoStack(nsITransaction **aTransaction)
 {
   nsRefPtr<nsTransactionItem> tx;
   nsresult result;
 
   NS_ENSURE_TRUE(aTransaction, NS_ERROR_NULL_POINTER);
 
   *aTransaction = 0;
 
-  LOCK_TX_MANAGER(this);
-
   result = mRedoStack.Peek(getter_AddRefs(tx));
 
   if (NS_FAILED(result) || !tx) {
-    UNLOCK_TX_MANAGER(this);
     return result;
   }
 
   result = tx->GetTransaction(aTransaction);
 
-  UNLOCK_TX_MANAGER(this);
-
   return result;
 }
 
 NS_IMETHODIMP
 nsTransactionManager::GetUndoList(nsITransactionList **aTransactionList)
 {
   NS_ENSURE_TRUE(aTransactionList, NS_ERROR_NULL_POINTER);
 
@@ -624,61 +530,37 @@ nsTransactionManager::GetRedoList(nsITra
   return (! *aTransactionList) ? NS_ERROR_OUT_OF_MEMORY : NS_OK;
 }
 
 NS_IMETHODIMP
 nsTransactionManager::AddListener(nsITransactionListener *aListener)
 {
   NS_ENSURE_TRUE(aListener, NS_ERROR_NULL_POINTER);
 
-  LOCK_TX_MANAGER(this);
-
-  nsresult rv = mListeners.AppendObject(aListener) ? NS_OK : NS_ERROR_FAILURE;
-
-  UNLOCK_TX_MANAGER(this);
-
-  return rv;
+  return mListeners.AppendObject(aListener) ? NS_OK : NS_ERROR_FAILURE;
 }
 
 NS_IMETHODIMP
 nsTransactionManager::RemoveListener(nsITransactionListener *aListener)
 {
   NS_ENSURE_TRUE(aListener, NS_ERROR_NULL_POINTER);
 
-  LOCK_TX_MANAGER(this);
-
-  nsresult rv = mListeners.RemoveObject(aListener) ? NS_OK : NS_ERROR_FAILURE;
-
-  UNLOCK_TX_MANAGER(this);
-
-  return rv;
+  return mListeners.RemoveObject(aListener) ? NS_OK : NS_ERROR_FAILURE;
 }
 
 nsresult
 nsTransactionManager::ClearUndoStack()
 {
-  nsresult result;
-
-  LOCK_TX_MANAGER(this);
-  result = mUndoStack.Clear();
-  UNLOCK_TX_MANAGER(this);
-
-  return result;
+  return mUndoStack.Clear();
 }
 
 nsresult
 nsTransactionManager::ClearRedoStack()
 {
-  nsresult result;
-
-  LOCK_TX_MANAGER(this);
-  result = mRedoStack.Clear();
-  UNLOCK_TX_MANAGER(this);
-
-  return result;
+  return mRedoStack.Clear();
 }
 
 nsresult
 nsTransactionManager::WillDoNotify(nsITransaction *aTransaction, bool *aInterrupt)
 {
   nsresult result = NS_OK;
   for (PRInt32 i = 0, lcount = mListeners.Count(); i < lcount; i++)
   {
@@ -907,19 +789,16 @@ nsTransactionManager::DidMergeNotify(nsI
   return result;
 }
 
 nsresult
 nsTransactionManager::BeginTransaction(nsITransaction *aTransaction)
 {
   nsresult result = NS_OK;
 
-  // No need for LOCK/UNLOCK_TX_MANAGER() calls since the calling routine
-  // should have done this already!
-
   // XXX: POSSIBLE OPTIMIZATION
   //      We could use a factory that pre-allocates/recycles transaction items.
   nsRefPtr<nsTransactionItem> tx = new nsTransactionItem(aTransaction);
 
   if (!tx) {
     return NS_ERROR_OUT_OF_MEMORY;
   }
 
@@ -941,19 +820,16 @@ nsTransactionManager::BeginTransaction(n
 
 nsresult
 nsTransactionManager::EndTransaction()
 {
   nsCOMPtr<nsITransaction> tint;
   nsRefPtr<nsTransactionItem> tx;
   nsresult result              = NS_OK;
 
-  // No need for LOCK/UNLOCK_TX_MANAGER() calls since the calling routine
-  // should have done this already!
-
   result = mDoStack.Pop(getter_AddRefs(tx));
 
   if (NS_FAILED(result) || !tx)
     return result;
 
   result = tx->GetTransaction(getter_AddRefs(tint));
 
   if (NS_FAILED(result)) {
@@ -1072,26 +948,8 @@ nsTransactionManager::EndTransaction()
   if (NS_FAILED(result)) {
     // XXX: What do we do in the case where a clear fails?
     //      Remove the transaction from the stack, and release it?
   }
 
   return result;
 }
 
-nsresult
-nsTransactionManager::Lock()
-{
-  if (mMonitor)
-    PR_EnterMonitor(mMonitor);
-
-  return NS_OK;
-}
-
-nsresult
-nsTransactionManager::Unlock()
-{
-  if (mMonitor)
-    PR_ExitMonitor(mMonitor);
-
-  return NS_OK;
-}
-
--- a/editor/txmgr/src/nsTransactionManager.h
+++ b/editor/txmgr/src/nsTransactionManager.h
@@ -33,17 +33,16 @@
  * the provisions above, a recipient may use your version of this file under
  * the terms of any one of the MPL, the GPL or the LGPL.
  *
  * ***** END LICENSE BLOCK ***** */
 
 #ifndef nsTransactionManager_h__
 #define nsTransactionManager_h__
 
-#include "prmon.h"
 #include "nsWeakReference.h"
 #include "nsITransactionManager.h"
 #include "nsCOMArray.h"
 #include "nsITransactionListener.h"
 #include "nsCycleCollectionParticipant.h"
 
 class nsITransaction;
 class nsITransactionListener;
@@ -60,18 +59,16 @@ class nsTransactionManager : public nsIT
 private:
 
   PRInt32                mMaxTransactionCount;
   nsTransactionStack     mDoStack;
   nsTransactionStack     mUndoStack;
   nsTransactionRedoStack mRedoStack;
   nsCOMArray<nsITransactionListener> mListeners;
 
-  PRMonitor              *mMonitor;
-
 public:
 
   /** The default constructor.
    */
   nsTransactionManager(PRInt32 aMaxTransactionCount=-1);
 
   /** The default destructor.
    */
@@ -107,13 +104,11 @@ public:
                                   bool aDidMerge,
                                   nsresult aMergeResult);
 
 private:
 
   /* nsTransactionManager specific private methods. */
   virtual nsresult BeginTransaction(nsITransaction *aTransaction);
   virtual nsresult EndTransaction(void);
-  virtual nsresult Lock(void);
-  virtual nsresult Unlock(void);
 };
 
 #endif // nsTransactionManager_h__