Bug 1527505 - Part 4: Compute the type index in the recursive loop only once; r=nika

This patch alone decreases the runtime of the loop on my machine from about
1700 to 200-400ms ranges.  It turns out that computing the type index is
the most expensive part.  So perhaps we should look into improving that as
well.

The first thing that comes to mind is whether we can inline the loop in
GetTypeIndex().  The next part takes care of that, and it does help a bit.
But we need to do more still.

The next obvious thing is to optimize the memory access patterns.  Right
now we iterate over an array of dynamically allocated strings to compare
them, which amounts to pointer chasing to read a bit of memory, kind of
the worst possible way to access memory.  Then we look at replacing that
with fully sequential memory reads in the common cases.

Depends on D20231

Differential Revision: https://phabricator.services.mozilla.com/D20232

--HG--
extra : moz-landing-system : lando

extensions/cookie/nsPermissionManager.cpp

@@ -2252,41 +2252,45 @@ nsresult nsPermissionManager::RemoveAllInternal(bool aNotifyObservers) {
 NS_IMETHODIMP
 nsPermissionManager::TestExactPermission(nsIURI* aURI, const char* aType,
                                          uint32_t* aPermission) {
-  return CommonTestPermission(aURI, aType, aPermission, true, true);
+  return CommonTestPermission(aURI, -1, aType, aPermission, true, true);
 }
 
 NS_IMETHODIMP
 nsPermissionManager::TestExactPermissionFromPrincipal(nsIPrincipal* aPrincipal,
                                                       const char* aType,
                                                       uint32_t* aPermission) {
-  return CommonTestPermission(aPrincipal, aType, aPermission, true, true);
+  return CommonTestPermission(aPrincipal, -1, aType, aPermission, true, true);
 }
 
 NS_IMETHODIMP
 nsPermissionManager::TestExactPermanentPermission(nsIPrincipal* aPrincipal,
                                                   const char* aType,
                                                   uint32_t* aPermission) {
-  return CommonTestPermission(aPrincipal, aType, aPermission, true, false);
+  return CommonTestPermission(aPrincipal, -1, aType, aPermission, true, false);
 }
 
 NS_IMETHODIMP
 nsPermissionManager::TestPermission(nsIURI* aURI, const char* aType,
                                     uint32_t* aPermission) {
-  return CommonTestPermission(aURI, aType, aPermission, false, true);
+  return CommonTestPermission(aURI, -1, aType, aPermission, false, true);
 }
 
 NS_IMETHODIMP
 nsPermissionManager::TestPermissionOriginNoSuffix(
     const nsACString& aOriginNoSuffix, const char* aType,
     uint32_t* aPermission) {
-  auto preparationResult = CommonPrepareToTestPermission(nullptr, aPermission);
+  // Our caller isn't providing a type index hint, so we just pass -1 to force
+  // CommonPrepareToTestPermission to compute it for us based on aType.
+  auto preparationResult =
+      CommonPrepareToTestPermission(nullptr, -1, aType, aPermission);
   if (preparationResult.mShouldContinue == eDone) {
     return NS_OK;
   }
 
   MOZ_ASSERT(!preparationResult.mPrincipal);
 
-  return CommonTestPermissionInternal(nullptr, nullptr, aOriginNoSuffix, aType,
+  return CommonTestPermissionInternal(nullptr, nullptr, aOriginNoSuffix,
+                                      preparationResult.mTypeIndex, aType,
                                       aPermission, false, true);
 }
 
@@ -2309,7 +2313,7 @@ NS_IMETHODIMP
 nsPermissionManager::TestPermissionFromPrincipal(nsIPrincipal* aPrincipal,
                                                  const char* aType,
                                                  uint32_t* aPermission) {
-  return CommonTestPermission(aPrincipal, aType, aPermission, false, true);
+  return CommonTestPermission(aPrincipal, -1, aType, aPermission, false, true);
 }
 
 NS_IMETHODIMP
@@ -2380,8 +2384,8 @@ nsPermissionManager::GetPermissionObject(nsIPrincipal* aPrincipal,
 
 nsresult nsPermissionManager::CommonTestPermissionInternal(
     BasePrincipal* aPrincipal, nsIURI* aURI, const nsACString& aOriginNoSuffix,
-    const char* aType, uint32_t* aPermission, bool aExactHostMatch,
-    bool aIncludingSession) {
+    int32_t aTypeIndex, const char* aType, uint32_t* aPermission,
+    bool aExactHostMatch, bool aIncludingSession) {
   MOZ_ASSERT(aPrincipal || aURI || !aOriginNoSuffix.IsEmpty());
   MOZ_ASSERT_IF(aPrincipal, !aURI && aOriginNoSuffix.IsEmpty());
   MOZ_ASSERT_IF(aURI, !aPrincipal && aOriginNoSuffix.IsEmpty());
@@ -2408,8 +2412,8 @@ nsresult nsPermissionManager::CommonTestPermissionInternal(
     auto ep = aPrincipal->As<ExpandedPrincipal>();
     for (auto& prin : ep->AllowList()) {
       uint32_t perm;
-      nsresult rv = CommonTestPermission(prin, aType, &perm, aExactHostMatch,
-                                         aIncludingSession);
+      nsresult rv = CommonTestPermission(prin, aTypeIndex, aType, &perm,
+                                         aExactHostMatch, aIncludingSession);
       NS_ENSURE_SUCCESS(rv, rv);
       if (perm == nsIPermissionManager::ALLOW_ACTION) {
         *aPermission = perm;
@@ -2439,24 +2443,19 @@ nsresult nsPermissionManager::CommonTestPermissionInternal(
   }
 #endif
 
-  int32_t typeIndex = GetTypeIndex(aType, false);
-  // If type == -1, the type isn't known,
-  // so just return NS_OK
-  if (typeIndex == -1) return NS_OK;
-
   PermissionHashKey* entry =
-      aPrincipal ? GetPermissionHashKey(aPrincipal, typeIndex, aExactHostMatch)
-                 : GetPermissionHashKey(aURI, aOriginNoSuffix, typeIndex,
+      aPrincipal ? GetPermissionHashKey(aPrincipal, aTypeIndex, aExactHostMatch)
+                 : GetPermissionHashKey(aURI, aOriginNoSuffix, aTypeIndex,
                                         aExactHostMatch);
   if (!entry || (!aIncludingSession &&
-                 entry->GetPermission(typeIndex).mNonSessionExpireType ==
+                 entry->GetPermission(aTypeIndex).mNonSessionExpireType ==
                      nsIPermissionManager::EXPIRE_SESSION)) {
     return NS_OK;
   }
 
   *aPermission = aIncludingSession
-                     ? entry->GetPermission(typeIndex).mPermission
-                     : entry->GetPermission(typeIndex).mNonSessionPermission;
+                     ? entry->GetPermission(aTypeIndex).mPermission
+                     : entry->GetPermission(aTypeIndex).mNonSessionPermission;
 
   return NS_OK;
 }

extensions/cookie/nsPermissionManager.h

@@ -279,51 +279,83 @@ class nsPermissionManager final : public nsIPermissionManager,
   enum TestPreparationEnum { eContinue, eDone };
   struct TestPreparationResult {
     mozilla::BasePrincipal* mPrincipal;
+    int32_t mTypeIndex;
     TestPreparationEnum mShouldContinue;
   };
+  /**
+   * Perform the early steps of a permission check and determine whether we need
+   * to call CommonTestPermissionInternal() for the actual permission check.
+   *
+   * @param aPrincipal optional principal argument to check the permission for,
+   *                   can be nullptr if we aren't performing a principal-based
+   *                   check.
+   * @param aTypeIndex if the caller isn't sure what the index of the permission
+   *                   type to check for is in the mTypeArray member variable,
+   *                   it should pass -1, otherwise this would be the index of
+   *                   the type inside mTypeArray.  This would only be something
+   *                   other than -1 in recursive invocations of this function.
+   * @param aType      the permission type to test.
+   * @param aPermission out argument which will be a permission type that we
+   *                    will return from this function once the function is
+   *                    done.
+   */
   TestPreparationResult CommonPrepareToTestPermission(nsIPrincipal* aPrincipal,
+                                                      int32_t aTypeIndex,
+                                                      const char* aType,
                                                       uint32_t* aPermission) {
     auto* basePrin = mozilla::BasePrincipal::Cast(aPrincipal);
     if (basePrin && basePrin->IsSystemPrincipal()) {
       *aPermission = nsIPermissionManager::ALLOW_ACTION;
-      return {basePrin, eDone};
+      return {basePrin, -1, eDone};
     }
 
-    return {basePrin, eContinue};
+    *aPermission = nsIPermissionManager::UNKNOWN_ACTION;
+
+    int32_t typeIndex =
+        aTypeIndex == -1 ? GetTypeIndex(aType, false) : aTypeIndex;
+    // If type == -1, the type isn't known, just signal that we are done.
+    if (typeIndex == -1) {
+      return {basePrin, -1, eDone};
+    }
+
+    return {basePrin, typeIndex, eContinue};
   }
 
-  nsresult CommonTestPermission(nsIPrincipal* aPrincipal, const char* aType,
-                                uint32_t* aPermission, bool aExactHostMatch,
-                                bool aIncludingSession) {
-    auto preparationResult =
-        CommonPrepareToTestPermission(aPrincipal, aPermission);
+  // If aTypeIndex is passed -1, we try to inder the type index from aType.
+  nsresult CommonTestPermission(nsIPrincipal* aPrincipal, int32_t aTypeIndex,
+                                const char* aType, uint32_t* aPermission,
+                                bool aExactHostMatch, bool aIncludingSession) {
+    auto preparationResult = CommonPrepareToTestPermission(
+        aPrincipal, aTypeIndex, aType, aPermission);
     if (preparationResult.mShouldContinue == eDone) {
       return NS_OK;
     }
 
-    return CommonTestPermissionInternal(preparationResult.mPrincipal, nullptr,
-                                        EmptyCString(), aType, aPermission,
-                                        aExactHostMatch, aIncludingSession);
+    return CommonTestPermissionInternal(
+        preparationResult.mPrincipal, nullptr, EmptyCString(),
+        preparationResult.mTypeIndex, aType, aPermission, aExactHostMatch,
+        aIncludingSession);
   }
-  nsresult CommonTestPermission(nsIURI* aURI, const char* aType,
-                                uint32_t* aPermission, bool aExactHostMatch,
-                                bool aIncludingSession) {
+  // If aTypeIndex is passed -1, we try to inder the type index from aType.
+  nsresult CommonTestPermission(nsIURI* aURI, int32_t aTypeIndex,
+                                const char* aType, uint32_t* aPermission,
+                                bool aExactHostMatch, bool aIncludingSession) {
     auto preparationResult =
-        CommonPrepareToTestPermission(nullptr, aPermission);
+        CommonPrepareToTestPermission(nullptr, aTypeIndex, aType, aPermission);
     if (preparationResult.mShouldContinue == eDone) {
       return NS_OK;
     }
 
     MOZ_ASSERT(!preparationResult.mPrincipal);
 
-    return CommonTestPermissionInternal(nullptr, aURI, EmptyCString(), aType,
-                                        aPermission, aExactHostMatch,
-                                        aIncludingSession);
+    return CommonTestPermissionInternal(
+        nullptr, aURI, EmptyCString(), preparationResult.mTypeIndex, aType,
+        aPermission, aExactHostMatch, aIncludingSession);
   }
   // Only one of aPrincipal or aURI is allowed to be passed in.
   nsresult CommonTestPermissionInternal(
       mozilla::BasePrincipal* aPrincipal, nsIURI* aURI,
-      const nsACString& aOriginNoSuffix, const char* aType,
+      const nsACString& aOriginNoSuffix, int32_t aTypeIndex, const char* aType,
       uint32_t* aPermission, bool aExactHostMatch, bool aIncludingSession);
 
   nsresult OpenDatabase(nsIFile* permissionsFile);