From ef76693c356a93e15ae0884b7693abe147cd902e Mon Sep 17 00:00:00 2001 From: Ehsan Akhgari Date: Thu, 21 Feb 2019 22:54:12 +0000 Subject: [PATCH] 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 | 41 +++++++------- extensions/cookie/nsPermissionManager.h | 68 +++++++++++++++++------ 2 files changed, 70 insertions(+), 39 deletions(-) diff --git a/extensions/cookie/nsPermissionManager.cpp b/extensions/cookie/nsPermissionManager.cpp index b2ab0cbb4004..b4951fa17e2d 100644 --- a/extensions/cookie/nsPermissionManager.cpp +++ b/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(); 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; } diff --git a/extensions/cookie/nsPermissionManager.h b/extensions/cookie/nsPermissionManager.h index a94edecf9043..e9d329f8a8b7 100644 --- a/extensions/cookie/nsPermissionManager.h +++ b/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);