Bug 1472523: Part 3 - Use the same nsCString for pref callback/observer objects. r=njn
authorKris Maglione <maglione.k@gmail.com>
Sun, 01 Jul 2018 10:39:10 -0700
changeset 425135 506cb0a8e4e278ef54f6e741ca226b8e32d86af9
parent 425134 4e2ddd1d83f11fd222f26a64c6b7c7eea9e6ff93
child 425136 c430c64b47d64406108819996da3e1e00da91cf4
push id104980
push usermaglione.k@gmail.com
push dateThu, 05 Jul 2018 03:19:58 +0000
treeherdermozilla-inbound@c430c64b47d6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersnjn
bugs1472523
milestone63.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 1472523: Part 3 - Use the same nsCString for pref callback/observer objects. r=njn This reduced the additional string duplication that we currently do every time we add a preference observer. It changes the string that we store in the observer objects to be absolute, rather than relative to the branch, but keeps the semantics the same, by resolving the full preference name in the places we were previously matching by relative string. This actually has the effect of simplifying a lot of code, since the absolute preference name is usually what we want. MozReview-Commit-ID: 10WjHb0tNGB
modules/libpref/Preferences.cpp
--- a/modules/libpref/Preferences.cpp
+++ b/modules/libpref/Preferences.cpp
@@ -1459,31 +1459,31 @@ public:
   static PLDHashNumber HashKey(const PrefCallback* aKey)
   {
     uint32_t hash = mozilla::HashString(aKey->mDomain);
     return mozilla::AddToHash(hash, aKey->mCanonical);
   }
 
 public:
   // Create a PrefCallback with a strong reference to its observer.
-  PrefCallback(const char* aDomain,
+  PrefCallback(const nsACString& aDomain,
                nsIObserver* aObserver,
                nsPrefBranch* aBranch)
     : mDomain(aDomain)
     , mBranch(aBranch)
     , mWeakRef(nullptr)
     , mStrongRef(aObserver)
   {
     MOZ_COUNT_CTOR(PrefCallback);
     nsCOMPtr<nsISupports> canonical = do_QueryInterface(aObserver);
     mCanonical = canonical;
   }
 
   // Create a PrefCallback with a weak reference to its observer.
-  PrefCallback(const char* aDomain,
+  PrefCallback(const nsACString& aDomain,
                nsISupportsWeakReference* aObserver,
                nsPrefBranch* aBranch)
     : mDomain(aDomain)
     , mBranch(aBranch)
     , mWeakRef(do_GetWeakReference(aObserver))
     , mStrongRef(nullptr)
   {
     MOZ_COUNT_CTOR(PrefCallback);
@@ -1629,28 +1629,40 @@ private:
     PrefName& operator=(const PrefName&) = delete;
 
     struct PtrMatcher
     {
       static const char* match(const char* aVal) { return aVal; }
       static const char* match(const nsCString& aVal) { return aVal.get(); }
     };
 
+    struct CStringMatcher
+    {
+      // Note: This is a reference, not an instance. It's used to pass our outer
+      // method argument through to our matcher methods.
+      nsACString& mStr;
+
+      void match(const char* aVal) { mStr.Assign(aVal); }
+      void match(const nsCString& aVal) { mStr.Assign(aVal); }
+    };
+
     struct LenMatcher
     {
       static size_t match(const char* aVal) { return strlen(aVal); }
       static size_t match(const nsCString& aVal) { return aVal.Length(); }
     };
 
     const char* get() const
     {
       static PtrMatcher m;
       return match(m);
     }
 
+    void get(nsACString& aStr) const { match(CStringMatcher{ aStr }); }
+
     size_t Length() const
     {
       static LenMatcher m;
       return match(m);
     }
   };
 
   virtual ~nsPrefBranch();
@@ -2372,49 +2384,51 @@ nsPrefBranch::AddObserver(const char* aD
                           nsIObserver* aObserver,
                           bool aHoldWeak)
 {
   PrefCallback* pCallback;
 
   NS_ENSURE_ARG(aDomain);
   NS_ENSURE_ARG(aObserver);
 
+  nsCString prefName;
+  GetPrefName(aDomain).get(prefName);
+
   // Hold a weak reference to the observer if so requested.
   if (aHoldWeak) {
     nsCOMPtr<nsISupportsWeakReference> weakRefFactory =
       do_QueryInterface(aObserver);
     if (!weakRefFactory) {
       // The caller didn't give us a object that supports weak reference...
       // tell them.
       return NS_ERROR_INVALID_ARG;
     }
 
     // Construct a PrefCallback with a weak reference to the observer.
-    pCallback = new PrefCallback(aDomain, weakRefFactory, this);
+    pCallback = new PrefCallback(prefName, weakRefFactory, this);
 
   } else {
     // Construct a PrefCallback with a strong reference to the observer.
-    pCallback = new PrefCallback(aDomain, aObserver, this);
+    pCallback = new PrefCallback(prefName, aObserver, this);
   }
 
   auto p = mObservers.LookupForAdd(pCallback);
   if (p) {
     NS_WARNING("Ignoring duplicate observer.");
     delete pCallback;
     return NS_OK;
   }
 
   p.OrInsert([&pCallback]() { return pCallback; });
 
   // We must pass a fully qualified preference name to the callback
   // aDomain == nullptr is the only possible failure, and we trapped it with
   // NS_ENSURE_ARG above.
-  const PrefName& pref = GetPrefName(aDomain);
   Preferences::RegisterCallback(NotifyObserver,
-                                pref.get(),
+                                prefName,
                                 pCallback,
                                 Preferences::PrefixMatch,
                                 /* isPriority */ false);
 
   return NS_OK;
 }
 
 NS_IMETHODIMP
@@ -2434,24 +2448,24 @@ nsPrefBranch::RemoveObserver(const char*
   // break the iteration in FreeObserverList.
   if (mFreeingObserverList) {
     return NS_OK;
   }
 
   // Remove the relevant PrefCallback from mObservers and get an owning pointer
   // to it. Unregister the callback first, and then let the owning pointer go
   // out of scope and destroy the callback.
-  PrefCallback key(aDomain, aObserver, this);
+  nsCString prefName;
+  GetPrefName(aDomain).get(prefName);
+  PrefCallback key(prefName, aObserver, this);
   nsAutoPtr<PrefCallback> pCallback;
   mObservers.Remove(&key, &pCallback);
   if (pCallback) {
-    // aDomain == nullptr is the only possible failure, trapped above.
-    const PrefName& pref = GetPrefName(aDomain);
     rv = Preferences::UnregisterCallback(
-      NotifyObserver, pref.get(), pCallback, Preferences::PrefixMatch);
+      NotifyObserver, prefName, pCallback, Preferences::PrefixMatch);
   }
 
   return rv;
 }
 
 NS_IMETHODIMP
 nsPrefBranch::Observe(nsISupports* aSubject,
                       const char* aTopic,
@@ -2475,17 +2489,17 @@ nsPrefBranch::NotifyObserver(const char*
     // The observer has expired.  Let's remove this callback.
     pCallback->GetPrefBranch()->RemoveExpiredCallback(pCallback);
     return;
   }
 
   // Remove any root this string may contain so as to not confuse the observer
   // by passing them something other than what they passed us as a topic.
   uint32_t len = pCallback->GetPrefBranch()->GetRootLength();
-  nsAutoCString suffix(aNewPref + len);
+  nsDependentCString suffix(aNewPref + len);
 
   observer->Observe(static_cast<nsIPrefBranch*>(pCallback->GetPrefBranch()),
                     NS_PREFBRANCH_PREFCHANGE_TOPIC_ID,
                     NS_ConvertASCIItoUTF16(suffix).get());
 }
 
 size_t
 nsPrefBranch::SizeOfIncludingThis(MallocSizeOf aMallocSizeOf) const
@@ -2508,20 +2522,18 @@ nsPrefBranch::FreeObserverList()
 {
   // We need to prevent anyone from modifying mObservers while we're iterating
   // over it. In particular, some clients will call RemoveObserver() when
   // they're removed and destructed via the iterator; we set
   // mFreeingObserverList to keep those calls from touching mObservers.
   mFreeingObserverList = true;
   for (auto iter = mObservers.Iter(); !iter.Done(); iter.Next()) {
     nsAutoPtr<PrefCallback>& callback = iter.Data();
-    nsPrefBranch* prefBranch = callback->GetPrefBranch();
-    const PrefName& pref = prefBranch->GetPrefName(callback->GetDomain().get());
     Preferences::UnregisterCallback(nsPrefBranch::NotifyObserver,
-                                    pref.get(),
+                                    callback->GetDomain(),
                                     callback,
                                     Preferences::PrefixMatch);
     iter.Remove();
   }
   mFreeingObserverList = false;
 }
 
 void
@@ -3038,40 +3050,37 @@ PreferenceServiceReporter::CollectReport
   size_t numWeakAlive = 0;
   size_t numWeakDead = 0;
   nsTArray<nsCString> suspectPreferences;
   // Count of the number of referents for each preference.
   nsDataHashtable<nsCStringHashKey, uint32_t> prefCounter;
 
   for (auto iter = rootBranch->mObservers.Iter(); !iter.Done(); iter.Next()) {
     nsAutoPtr<PrefCallback>& callback = iter.Data();
-    nsPrefBranch* prefBranch = callback->GetPrefBranch();
-    const auto& pref = prefBranch->GetPrefName(callback->GetDomain().get());
 
     if (callback->IsWeak()) {
       nsCOMPtr<nsIObserver> callbackRef = do_QueryReferent(callback->mWeakRef);
       if (callbackRef) {
         numWeakAlive++;
       } else {
         numWeakDead++;
       }
     } else {
       numStrong++;
     }
 
-    nsDependentCString prefString(pref.get());
     uint32_t oldCount = 0;
-    prefCounter.Get(prefString, &oldCount);
+    prefCounter.Get(callback->GetDomain(), &oldCount);
     uint32_t currentCount = oldCount + 1;
-    prefCounter.Put(prefString, currentCount);
+    prefCounter.Put(callback->GetDomain(), currentCount);
 
     // Keep track of preferences that have a suspiciously large number of
     // referents (a symptom of a leak).
     if (currentCount == kSuspectReferentCount) {
-      suspectPreferences.AppendElement(prefString);
+      suspectPreferences.AppendElement(callback->GetDomain());
     }
   }
 
   for (uint32_t i = 0; i < suspectPreferences.Length(); i++) {
     nsCString& suspect = suspectPreferences[i];
     uint32_t totalReferentCount = 0;
     prefCounter.Get(suspect, &totalReferentCount);