Bug 1689967 - Simplify CleanupTemporaryStorage and the functions it calls. r=dom-workers-and-storage-reviewers,janv
☠☠ backed out by 49fff125656d ☠ ☠
authorSimon Giesecke <sgiesecke@mozilla.com>
Tue, 02 Feb 2021 10:10:47 +0000
changeset 565614 129b45a961fb5bf9a71a217da71ac8079fbc4657
parent 565613 d354b3d5312c0bfe245e34ad880336abeb60ebfa
child 565615 06c045f7240a7f168e904e1e278e08cf1d3ed928
push id135529
push usersgiesecke@mozilla.com
push dateTue, 02 Feb 2021 14:30:08 +0000
treeherderautoland@129b45a961fb [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdom-workers-and-storage-reviewers, janv
bugs1689967
milestone87.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 1689967 - Simplify CleanupTemporaryStorage and the functions it calls. r=dom-workers-and-storage-reviewers,janv Instead of collection both origins exceeding their group limit and the global limit before clearing any origin, do that separately. This removes the need to adjust the global usage while determining which origins to evict simulating the effects of evicting the origins exceeding their group limit. This requires moving the lock into the GetOrigin* functions (thus removing "Locked" from their names). Also, the arguments to GetOriginInfosExceedingGlobalLimit are removed. As an effect, the predicate passed into MaybeInsertOriginInfos is the same in both uses, and just checks the IsPersisted property, so the predicate argument is removed and the function is renamed to MaybeInsertNonPersistedOriginInfos. Differential Revision: https://phabricator.services.mozilla.com/D103629
dom/quota/ActorsParent.cpp
dom/quota/QuotaManager.h
--- a/dom/quota/ActorsParent.cpp
+++ b/dom/quota/ActorsParent.cpp
@@ -7084,23 +7084,24 @@ already_AddRefed<OriginInfo> QuotaManage
     if (groupInfo) {
       return groupInfo->LockedGetOriginInfo(aGroupAndOrigin.mOrigin);
     }
   }
 
   return nullptr;
 }
 
-template <typename Iterator, typename Pred>
-void QuotaManager::MaybeInsertOriginInfos(
+template <typename Iterator>
+void QuotaManager::MaybeInsertNonPersistedOriginInfos(
     Iterator aDest, const RefPtr<GroupInfo>& aTemporaryGroupInfo,
-    const RefPtr<GroupInfo>& aDefaultGroupInfo, Pred&& aPred) {
-  const auto copy = [&aDest, &aPred](const GroupInfo& groupInfo) {
-    std::copy_if(groupInfo.mOriginInfos.cbegin(), groupInfo.mOriginInfos.cend(),
-                 aDest, aPred);
+    const RefPtr<GroupInfo>& aDefaultGroupInfo) {
+  const auto copy = [&aDest](const GroupInfo& groupInfo) {
+    std::copy_if(
+        groupInfo.mOriginInfos.cbegin(), groupInfo.mOriginInfos.cend(), aDest,
+        [](const auto& originInfo) { return !originInfo->LockedPersisted(); });
   };
 
   if (aTemporaryGroupInfo) {
     MOZ_ASSERT(PERSISTENCE_TYPE_TEMPORARY ==
                aTemporaryGroupInfo->GetPersistenceType());
 
     copy(*aTemporaryGroupInfo);
   }
@@ -7125,18 +7126,18 @@ QuotaManager::CollectLRUOriginInfosUntil
                                     std::forward<Pred>(aPred));
 
   originInfos.TruncateLength(foundIt - originInfos.cbegin());
 
   return originInfos;
 }
 
 QuotaManager::OriginInfosNestedTraversable
-QuotaManager::LockedGetOriginInfosExceedingGroupLimit() const {
-  mQuotaMutex.AssertCurrentThreadOwns();
+QuotaManager::GetOriginInfosExceedingGroupLimit() const {
+  MutexAutoLock lock(mQuotaMutex);
 
   OriginInfosNestedTraversable originInfos;
 
   for (const auto& entry : mGroupInfoPairs) {
     const auto& pair = entry.GetData();
 
     MOZ_ASSERT(!entry.GetKey().IsEmpty());
     MOZ_ASSERT(pair);
@@ -7157,101 +7158,63 @@ QuotaManager::LockedGetOriginInfosExceed
 
     if (groupUsage > 0) {
       QuotaManager* quotaManager = QuotaManager::Get();
       MOZ_ASSERT(quotaManager, "Shouldn't be null!");
 
       if (groupUsage > quotaManager->GetGroupLimit()) {
         originInfos.AppendElement(CollectLRUOriginInfosUntil(
             [&temporaryGroupInfo, &defaultGroupInfo](auto inserter) {
-              MaybeInsertOriginInfos(std::move(inserter), temporaryGroupInfo,
-                                     defaultGroupInfo,
-                                     [](const auto& originInfo) {
-                                       return !originInfo->LockedPersisted();
-                                     });
+              MaybeInsertNonPersistedOriginInfos(
+                  std::move(inserter), temporaryGroupInfo, defaultGroupInfo);
             },
             [&groupUsage, quotaManager](const auto& originInfo) {
               groupUsage -= originInfo->LockedUsage();
 
               return groupUsage <= quotaManager->GetGroupLimit();
             }));
       }
     }
   }
 
   return originInfos;
 }
 
-QuotaManager::OriginInfosFlatTraversable
-QuotaManager::LockedGetOriginInfosExceedingGlobalLimit(
-    const OriginInfosNestedTraversable& aAlreadyDoomedOriginInfos,
-    const uint64_t aAlreadyDoomedUsage) const {
-  mQuotaMutex.AssertCurrentThreadOwns();
-
-  return CollectLRUOriginInfosUntil(
-      [this, &aAlreadyDoomedOriginInfos](auto inserter) {
+QuotaManager::OriginInfosNestedTraversable
+QuotaManager::GetOriginInfosExceedingGlobalLimit() const {
+  MutexAutoLock lock(mQuotaMutex);
+
+  QuotaManager::OriginInfosNestedTraversable res;
+  res.AppendElement(CollectLRUOriginInfosUntil(
+      // XXX The lambda only needs to capture this, but due to Bug 1421435 it
+      // can't.
+      [&](auto inserter) {
         for (const auto& entry : mGroupInfoPairs) {
           const auto& pair = entry.GetData();
 
           MOZ_ASSERT(!entry.GetKey().IsEmpty());
           MOZ_ASSERT(pair);
 
-          MaybeInsertOriginInfos(
+          MaybeInsertNonPersistedOriginInfos(
               inserter, pair->LockedGetGroupInfo(PERSISTENCE_TYPE_TEMPORARY),
-              pair->LockedGetGroupInfo(PERSISTENCE_TYPE_DEFAULT),
-              [&aAlreadyDoomedOriginInfos](const auto& originInfo) {
-                return !std::any_of(aAlreadyDoomedOriginInfos.cbegin(),
-                                    aAlreadyDoomedOriginInfos.cend(),
-                                    // XXX This should capture originInfo by
-                                    // value, but it can't due to Bug 1421435.
-                                    [&originInfo](const auto& array) {
-                                      return array.Contains(originInfo);
-                                    }) &&
-                       !originInfo->LockedPersisted();
-              });
+              pair->LockedGetGroupInfo(PERSISTENCE_TYPE_DEFAULT));
         }
       },
-      [temporaryStorageUsage = mTemporaryStorageUsage - aAlreadyDoomedUsage,
+      [temporaryStorageUsage = mTemporaryStorageUsage,
        temporaryStorageLimit = mTemporaryStorageLimit,
        doomedUsage = uint64_t{0}](const auto& originInfo) mutable {
         if (temporaryStorageUsage - doomedUsage <= temporaryStorageLimit) {
           return true;
         }
 
         doomedUsage += originInfo->LockedUsage();
         return false;
-      });
-}
-
-QuotaManager::OriginInfosNestedTraversable
-QuotaManager::GetOriginInfosExceedingLimits() const {
-  MutexAutoLock lock(mQuotaMutex);
-
-  auto originInfos = LockedGetOriginInfosExceedingGroupLimit();
-
-  const uint64_t doomedUsage = std::accumulate(
-      originInfos.cbegin(), originInfos.cend(), uint64_t(0),
-      [](uint64_t oldValue, const auto& currentOriginInfos) {
-        return std::accumulate(currentOriginInfos.cbegin(),
-                               currentOriginInfos.cend(), oldValue,
-                               [](uint64_t oldValue, const auto& originInfo) {
-                                 return oldValue + originInfo->LockedUsage();
-                               });
-      });
-
-  // Evicting origins that exceed their group limit also affects the global
-  // temporary storage usage. If the global temporary storage limit would still
-  // be exceeded after evicting the origins that were already selected, we need
-  // to specifically evict origins to get below the global limit.
-  if (mTemporaryStorageUsage - doomedUsage > mTemporaryStorageLimit) {
-    originInfos.AppendElement(
-        LockedGetOriginInfosExceedingGlobalLimit(originInfos, doomedUsage));
-  }
-
-  return originInfos;
+      }));
+
+  return res;
 }
 
 void QuotaManager::ClearOrigins(
     const OriginInfosNestedTraversable& aDoomedOriginInfos) {
   AssertIsOnIOThread();
 
   // XXX Does this need to be done a) in order and/or b) sequentially?
   for (const auto& doomedOriginInfo :
@@ -7296,17 +7259,21 @@ void QuotaManager::ClearOrigins(
     OriginClearCompleted(clearedOrigin.mPersistenceType, clearedOrigin.mOrigin,
                          Nullable<Client::Type>());
   }
 }
 
 void QuotaManager::CleanupTemporaryStorage() {
   AssertIsOnIOThread();
 
-  ClearOrigins(GetOriginInfosExceedingLimits());
+  // Evicting origins that exceed their group limit also affects the global
+  // temporary storage usage, so these steps have to be taken sequentially.
+  // Combining them doesn't seem worth the added complexity.
+  ClearOrigins(GetOriginInfosExceedingGroupLimit());
+  ClearOrigins(GetOriginInfosExceedingGlobalLimit());
 
   if (mTemporaryStorageUsage > mTemporaryStorageLimit) {
     // If disk space is still low after origin clear, notify storage pressure.
     NotifyStoragePressure(mTemporaryStorageUsage);
   }
 }
 
 void QuotaManager::DeleteFilesForOrigin(PersistenceType aPersistenceType,
--- a/dom/quota/QuotaManager.h
+++ b/dom/quota/QuotaManager.h
@@ -559,23 +559,19 @@ class QuotaManager final : public Backgr
                             nsIFile* aDirectory);
 
   using OriginInfosFlatTraversable =
       nsTArray<NotNull<RefPtr<const OriginInfo>>>;
 
   using OriginInfosNestedTraversable =
       nsTArray<nsTArray<NotNull<RefPtr<const OriginInfo>>>>;
 
-  OriginInfosNestedTraversable LockedGetOriginInfosExceedingGroupLimit() const;
+  OriginInfosNestedTraversable GetOriginInfosExceedingGroupLimit() const;
 
-  OriginInfosFlatTraversable LockedGetOriginInfosExceedingGlobalLimit(
-      const OriginInfosNestedTraversable& aAlreadyDoomedOriginInfos,
-      uint64_t aAlreadyDoomedUsage) const;
-
-  OriginInfosNestedTraversable GetOriginInfosExceedingLimits() const;
+  OriginInfosNestedTraversable GetOriginInfosExceedingGlobalLimit() const;
 
   void ClearOrigins(const OriginInfosNestedTraversable& aDoomedOriginInfos);
 
   void CleanupTemporaryStorage();
 
   void DeleteFilesForOrigin(PersistenceType aPersistenceType,
                             const nsACString& aOrigin);
 
@@ -593,20 +589,20 @@ class QuotaManager final : public Backgr
 
   bool IsSanitizedOriginValid(const nsACString& aSanitizedOrigin);
 
   int64_t GenerateDirectoryLockId();
 
   void MaybeRecordShutdownStep(Maybe<Client::Type> aClientType,
                                const nsACString& aStepDescription);
 
-  template <typename Iterator, typename Pred>
-  static void MaybeInsertOriginInfos(
+  template <typename Iterator>
+  static void MaybeInsertNonPersistedOriginInfos(
       Iterator aDest, const RefPtr<GroupInfo>& aTemporaryGroupInfo,
-      const RefPtr<GroupInfo>& aDefaultGroupInfo, Pred&& aPred);
+      const RefPtr<GroupInfo>& aDefaultGroupInfo);
 
   template <typename Collect, typename Pred>
   static OriginInfosFlatTraversable CollectLRUOriginInfosUntil(
       Collect&& aCollect, Pred&& aPred);
 
   // Thread on which IO is performed.
   LazyInitializedOnceNotNull<const nsCOMPtr<nsIThread>> mIOThread;