Bug 1497007 - Replace custom for loops by range-based for and appropriate algorithms. r=ttung,asuth
authorSimon Giesecke <sgiesecke@mozilla.com>
Mon, 11 Nov 2019 08:24:53 +0000
changeset 501443 4ea3f8367672dfba56350d5447af359e00ea31b1
parent 501442 44af22f9c5f4f1b3a868c20c28616012f17f6f07
child 501444 9c38294bdc9482cbfeb319e80fa040c1615d3ea7
push id114170
push usermalexandru@mozilla.com
push dateTue, 12 Nov 2019 21:58:32 +0000
treeherdermozilla-inbound@9e3f44e87a1a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersttung, asuth
bugs1497007
milestone72.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1497007 - Replace custom for loops by range-based for and appropriate algorithms. r=ttung,asuth Differential Revision: https://phabricator.services.mozilla.com/D46948
dom/indexedDB/IDBIndex.cpp
dom/indexedDB/IDBObjectStore.cpp
dom/indexedDB/IDBTransaction.cpp
--- a/dom/indexedDB/IDBIndex.cpp
+++ b/dom/indexedDB/IDBIndex.cpp
@@ -83,34 +83,26 @@ void IDBIndex::AssertIsOnOwningThread() 
 }
 
 #endif  // DEBUG
 
 void IDBIndex::RefreshMetadata(bool aMayDelete) {
   AssertIsOnOwningThread();
   MOZ_ASSERT_IF(mDeletedMetadata, mMetadata == mDeletedMetadata);
 
-  const nsTArray<IndexMetadata>& indexes = mObjectStore->Spec().indexes();
-
-  bool found = false;
-
-  for (uint32_t count = indexes.Length(), index = 0; index < count; index++) {
-    const IndexMetadata& metadata = indexes[index];
-
-    if (metadata.id() == Id()) {
-      mMetadata = &metadata;
-
-      found = true;
-      break;
-    }
-  }
+  const auto& indexes = mObjectStore->Spec().indexes();
+  const auto foundIt = std::find_if(
+      indexes.cbegin(), indexes.cend(),
+      [id = Id()](const auto& metadata) { return metadata.id() == id; });
+  const bool found = foundIt != indexes.cend();
 
   MOZ_ASSERT_IF(!aMayDelete && !mDeletedMetadata, found);
 
   if (found) {
+    mMetadata = &*foundIt;
     MOZ_ASSERT(mMetadata != mDeletedMetadata);
     mDeletedMetadata = nullptr;
   } else {
     NoteDeletion();
   }
 }
 
 void IDBIndex::NoteDeletion() {
--- a/dom/indexedDB/IDBObjectStore.cpp
+++ b/dom/indexedDB/IDBObjectStore.cpp
@@ -2343,37 +2343,30 @@ void IDBObjectStore::RefreshSpec(bool aM
   AssertIsOnOwningThread();
   MOZ_ASSERT_IF(mDeletedSpec, mSpec == mDeletedSpec);
 
   const DatabaseSpec* dbSpec = mTransaction->Database()->Spec();
   MOZ_ASSERT(dbSpec);
 
   const nsTArray<ObjectStoreSpec>& objectStores = dbSpec->objectStores();
 
-  bool found = false;
-
-  for (uint32_t objCount = objectStores.Length(), objIndex = 0;
-       objIndex < objCount; objIndex++) {
-    const ObjectStoreSpec& objSpec = objectStores[objIndex];
-
-    if (objSpec.metadata().id() == Id()) {
-      mSpec = &objSpec;
-
-      for (uint32_t idxCount = mIndexes.Length(), idxIndex = 0;
-           idxIndex < idxCount; idxIndex++) {
-        mIndexes[idxIndex]->RefreshMetadata(aMayDelete);
-      }
-
-      for (uint32_t idxCount = mDeletedIndexes.Length(), idxIndex = 0;
-           idxIndex < idxCount; idxIndex++) {
-        mDeletedIndexes[idxIndex]->RefreshMetadata(false);
-      }
-
-      found = true;
-      break;
+  const auto foundIt = std::find_if(objectStores.cbegin(), objectStores.cend(),
+                                    [id = Id()](const auto& objSpec) {
+                                      return objSpec.metadata().id() == id;
+                                    });
+  const bool found = foundIt != objectStores.cend();
+  if (found) {
+    mSpec = &*foundIt;
+
+    for (auto& index : mIndexes) {
+      index->RefreshMetadata(aMayDelete);
+    }
+
+    for (auto& index : mDeletedIndexes) {
+      index->RefreshMetadata(false);
     }
   }
 
   MOZ_ASSERT_IF(!aMayDelete && !mDeletedSpec, found);
 
   if (found) {
     MOZ_ASSERT(mSpec != mDeletedSpec);
     mDeletedSpec = nullptr;
--- a/dom/indexedDB/IDBTransaction.cpp
+++ b/dom/indexedDB/IDBTransaction.cpp
@@ -26,16 +26,38 @@
 #include "nsServiceManagerUtils.h"
 #include "nsTHashtable.h"
 #include "ProfilerHelpers.h"
 #include "ReportInternalError.h"
 
 // Include this last to avoid path problems on Windows.
 #include "ActorsChild.h"
 
+namespace {
+// TODO: Move this to xpcom/ds.
+template <typename T, typename Range, typename Transformation>
+nsTHashtable<T> TransformToHashtable(const Range& aRange,
+                                     const Transformation& aTransformation) {
+  // TODO: Determining the size of the range is not syntactically necessary (and
+  // requires random access iterators if expressed this way). It is a
+  // performance optimization. We could resort to std::distance to support any
+  // iterator category, but this would lead to a double iteration of the range
+  // in case of non-random-access iterators. It is hard to determine in general
+  // if double iteration or reallocation is worse.
+  auto res = nsTHashtable<T>(aRange.cend() - aRange.cbegin());
+  // TOOD: std::transform could be used if nsTHashtable had an insert_iterator,
+  // and this would also allow a more generic version not depending on
+  // nsTHashtable at all.
+  for (const auto& item : aRange) {
+    res.PutEntry(aTransformation(item));
+  }
+  return res;
+}
+}  // namespace
+
 namespace mozilla {
 namespace dom {
 
 using namespace mozilla::dom::indexedDB;
 using namespace mozilla::ipc;
 
 bool IDBTransaction::HasTransactionChild() const {
   return (mMode == VERSION_CHANGE
@@ -91,31 +113,24 @@ IDBTransaction::IDBTransaction(IDBDataba
   ThreadLocal* idbThreadLocal = threadLocal->mIndexedDBThreadLocal;
   MOZ_ASSERT(idbThreadLocal);
 
   const_cast<int64_t&>(mLoggingSerialNumber) =
       idbThreadLocal->NextTransactionSN(aMode);
 
 #ifdef DEBUG
   if (!aObjectStoreNames.IsEmpty()) {
-    nsTArray<nsString> sortedNames(aObjectStoreNames);
-    sortedNames.Sort();
-
-    const uint32_t count = sortedNames.Length();
-    MOZ_ASSERT(count == aObjectStoreNames.Length());
-
     // Make sure the array is properly sorted.
-    for (uint32_t index = 0; index < count; index++) {
-      MOZ_ASSERT(aObjectStoreNames[index] == sortedNames[index]);
-    }
+    MOZ_ASSERT(
+        std::is_sorted(aObjectStoreNames.cbegin(), aObjectStoreNames.cend()));
 
     // Make sure there are no duplicates in our objectStore names.
-    for (uint32_t index = 0; index < count - 1; index++) {
-      MOZ_ASSERT(sortedNames[index] != sortedNames[index + 1]);
-    }
+    MOZ_ASSERT(aObjectStoreNames.cend() ==
+               std::adjacent_find(aObjectStoreNames.cbegin(),
+                                  aObjectStoreNames.cend()));
   }
 #endif
 
   mozilla::HoldJSObjects(this);
 }
 
 IDBTransaction::~IDBTransaction() {
   AssertIsOnOwningThread();
@@ -308,24 +323,22 @@ void IDBTransaction::OpenCursor(Backgrou
 
   // Balanced in BackgroundCursorChild::RecvResponse().
   OnNewRequest();
 }
 
 void IDBTransaction::RefreshSpec(const bool aMayDelete) {
   AssertIsOnOwningThread();
 
-  for (uint32_t count = mObjectStores.Length(), index = 0; index < count;
-       index++) {
-    mObjectStores[index]->RefreshSpec(aMayDelete);
+  for (auto& objectStore : mObjectStores) {
+    objectStore->RefreshSpec(aMayDelete);
   }
 
-  for (uint32_t count = mDeletedObjectStores.Length(), index = 0; index < count;
-       index++) {
-    mDeletedObjectStores[index]->RefreshSpec(false);
+  for (auto& objectStore : mDeletedObjectStores) {
+    objectStore->RefreshSpec(false);
   }
 }
 
 void IDBTransaction::OnNewRequest() {
   AssertIsOnOwningThread();
 
   if (!mPendingRequestCount) {
     MOZ_ASSERT(INITIAL == mReadyState);
@@ -464,22 +477,26 @@ already_AddRefed<IDBObjectStore> IDBTran
   AssertIsOnOwningThread();
   MOZ_ASSERT(aSpec.metadata().id());
   MOZ_ASSERT(VERSION_CHANGE == mMode);
   MOZ_ASSERT(mBackgroundActor.mVersionChangeBackgroundActor);
   MOZ_ASSERT(IsOpen());
 
 #ifdef DEBUG
   {
-    const nsString& name = aSpec.metadata().name();
-
-    for (uint32_t count = mObjectStores.Length(), index = 0; index < count;
-         index++) {
-      MOZ_ASSERT(mObjectStores[index]->Name() != name);
-    }
+    // TODO: Bind name outside of lambda capture as a workaround for GCC 7 bug
+    // https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66735.
+    const auto& name = aSpec.metadata().name();
+    // TODO: Use #ifdef and local variable as a workaround for Bug 1583449.
+    const bool objectStoreNameDoesNotYetExist =
+        std::all_of(mObjectStores.cbegin(), mObjectStores.cend(),
+                    [&name](const auto& objectStore) {
+                      return objectStore->Name() != name;
+                    });
+    MOZ_ASSERT(objectStoreNameDoesNotYetExist);
   }
 #endif
 
   MOZ_ALWAYS_TRUE(
       mBackgroundActor.mVersionChangeBackgroundActor->SendCreateObjectStore(
           aSpec.metadata()));
 
   RefPtr<IDBObjectStore> objectStore = IDBObjectStore::Create(this, aSpec);
@@ -496,30 +513,30 @@ void IDBTransaction::DeleteObjectStore(c
   MOZ_ASSERT(VERSION_CHANGE == mMode);
   MOZ_ASSERT(mBackgroundActor.mVersionChangeBackgroundActor);
   MOZ_ASSERT(IsOpen());
 
   MOZ_ALWAYS_TRUE(
       mBackgroundActor.mVersionChangeBackgroundActor->SendDeleteObjectStore(
           aObjectStoreId));
 
-  for (uint32_t count = mObjectStores.Length(), index = 0; index < count;
-       index++) {
-    RefPtr<IDBObjectStore>& objectStore = mObjectStores[index];
-
-    if (objectStore->Id() == aObjectStoreId) {
-      objectStore->NoteDeletion();
+  const auto foundIt =
+      std::find_if(mObjectStores.begin(), mObjectStores.end(),
+                   [aObjectStoreId](const auto& objectStore) {
+                     return objectStore->Id() == aObjectStoreId;
+                   });
+  if (foundIt != mObjectStores.end()) {
+    auto& objectStore = *foundIt;
+    objectStore->NoteDeletion();
 
-      RefPtr<IDBObjectStore>* deletedObjectStore =
-          mDeletedObjectStores.AppendElement();
-      deletedObjectStore->swap(mObjectStores[index]);
+    RefPtr<IDBObjectStore>* deletedObjectStore =
+        mDeletedObjectStores.AppendElement();
+    deletedObjectStore->swap(objectStore);
 
-      mObjectStores.RemoveElementAt(index);
-      break;
-    }
+    mObjectStores.RemoveElementAt(foundIt);
   }
 }
 
 void IDBTransaction::RenameObjectStore(const int64_t aObjectStoreId,
                                        const nsAString& aName) {
   AssertIsOnOwningThread();
   MOZ_ASSERT(aObjectStoreId);
   MOZ_ASSERT(VERSION_CHANGE == mMode);
@@ -605,56 +622,53 @@ void IDBTransaction::AbortInternal(const
 
     const nsTArray<ObjectStoreSpec>& specArray =
         mDatabase->Spec()->objectStores();
 
     if (specArray.IsEmpty()) {
       // This case is specially handled as a performance optimization, it is
       // equivalent to the else block.
       mObjectStores.Clear();
-      mDeletedObjectStores.Clear();
     } else {
-      nsTHashtable<nsUint64HashKey> validIds(specArray.Length());
+      const auto validIds = TransformToHashtable<nsUint64HashKey>(
+          specArray, [](const auto& spec) {
+            const int64_t objectStoreId = spec.metadata().id();
+            MOZ_ASSERT(objectStoreId);
+            return static_cast<uint64_t>(objectStoreId);
+          });
 
-      for (uint32_t specCount = specArray.Length(), specIndex = 0;
-           specIndex < specCount; specIndex++) {
-        const int64_t objectStoreId = specArray[specIndex].metadata().id();
-        MOZ_ASSERT(objectStoreId);
+      mObjectStores.RemoveElementsAt(
+          std::remove_if(mObjectStores.begin(), mObjectStores.end(),
+                         [&validIds](const auto& objectStore) {
+                           return !validIds.Contains(
+                               uint64_t(objectStore->Id()));
+                         }),
+          mObjectStores.end());
 
-        validIds.PutEntry(uint64_t(objectStoreId));
-      }
-
-      for (uint32_t objCount = mObjectStores.Length(), objIndex = 0;
-           objIndex < objCount;
-           /* incremented conditionally */) {
-        const int64_t objectStoreId = mObjectStores[objIndex]->Id();
+      // TODO: if nsTArray had an insert iterator, we could use the following
+      // instead:
+      // std::copy_if(std::make_move_iterator(mDeletedObjectStores.begin()),
+      //              std::make_move_iterator(mDeletedObjectStores.end()),
+      //              std::back_inserter(mObjectStores),
+      //              [&validIds](const auto& deletedObjectStore) {
+      //                const int64_t objectStoreId = deletedObjectStore->Id();
+      //                MOZ_ASSERT(objectStoreId);
+      //                return validIds.Contains(uint64_t(objectStoreId));
+      //              });
+      for (auto& deletedObjectStore : mDeletedObjectStores) {
+        const int64_t objectStoreId = deletedObjectStore->Id();
         MOZ_ASSERT(objectStoreId);
 
         if (validIds.Contains(uint64_t(objectStoreId))) {
-          objIndex++;
-        } else {
-          mObjectStores.RemoveElementAt(objIndex);
-          objCount--;
+          RefPtr<IDBObjectStore>* objectStore = mObjectStores.AppendElement();
+          objectStore->swap(deletedObjectStore);
         }
       }
-
-      if (!mDeletedObjectStores.IsEmpty()) {
-        for (uint32_t objCount = mDeletedObjectStores.Length(), objIndex = 0;
-             objIndex < objCount; objIndex++) {
-          const int64_t objectStoreId = mDeletedObjectStores[objIndex]->Id();
-          MOZ_ASSERT(objectStoreId);
-
-          if (validIds.Contains(uint64_t(objectStoreId))) {
-            RefPtr<IDBObjectStore>* objectStore = mObjectStores.AppendElement();
-            objectStore->swap(mDeletedObjectStores[objIndex]);
-          }
-        }
-        mDeletedObjectStores.Clear();
-      }
     }
+    mDeletedObjectStores.Clear();
   }
 
   // Fire the abort event if there are no outstanding requests. Otherwise the
   // abort event will be fired when all outstanding requests finish.
   if (needToSendAbort) {
     SendAbort(aAbortCode);
   }
 
@@ -868,46 +882,41 @@ already_AddRefed<IDBObjectStore> IDBTran
 
   const ObjectStoreSpec* spec = nullptr;
 
   if (IDBTransaction::VERSION_CHANGE == mMode ||
       mObjectStoreNames.Contains(aName)) {
     const nsTArray<ObjectStoreSpec>& objectStores =
         mDatabase->Spec()->objectStores();
 
-    for (uint32_t count = objectStores.Length(), index = 0; index < count;
-         index++) {
-      const ObjectStoreSpec& objectStore = objectStores[index];
-      if (objectStore.metadata().name() == aName) {
-        spec = &objectStore;
-        break;
-      }
+    const auto foundIt =
+        std::find_if(objectStores.cbegin(), objectStores.cend(),
+                     [&aName](const auto& objectStore) {
+                       return objectStore.metadata().name() == aName;
+                     });
+    if (foundIt != objectStores.cend()) {
+      spec = &*foundIt;
     }
   }
 
   if (!spec) {
     aRv.Throw(NS_ERROR_DOM_INDEXEDDB_NOT_FOUND_ERR);
     return nullptr;
   }
 
-  const int64_t desiredId = spec->metadata().id();
-
   RefPtr<IDBObjectStore> objectStore;
 
-  for (uint32_t count = mObjectStores.Length(), index = 0; index < count;
-       index++) {
-    RefPtr<IDBObjectStore>& existingObjectStore = mObjectStores[index];
-
-    if (existingObjectStore->Id() == desiredId) {
-      objectStore = existingObjectStore;
-      break;
-    }
-  }
-
-  if (!objectStore) {
+  const auto foundIt = std::find_if(
+      mObjectStores.cbegin(), mObjectStores.cend(),
+      [desiredId = spec->metadata().id()](const auto& existingObjectStore) {
+        return existingObjectStore->Id() == desiredId;
+      });
+  if (foundIt != mObjectStores.cend()) {
+    objectStore = *foundIt;
+  } else {
     objectStore = IDBObjectStore::Create(this, *spec);
     MOZ_ASSERT(objectStore);
 
     mObjectStores.AppendElement(objectStore);
   }
 
   return objectStore.forget();
 }