Bug 1414188 - Remove ValueObserver. r=glandium.
authorNicholas Nethercote <nnethercote@mozilla.com>
Thu, 02 Nov 2017 20:38:34 +1100
changeset 443953 891b6831d29bdfc80fc14434185bfd7c975e45dd
parent 443868 90a7bc300af375c80574266857e3ce46c30c6697
child 443954 e05d3e232865dbb2533966879f50db54daa7340e
push id1618
push userCallek@gmail.com
push dateThu, 11 Jan 2018 17:45:48 +0000
treeherdermozilla-release@882ca853e05a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersglandium
bugs1414188
milestone58.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 1414188 - Remove ValueObserver. r=glandium. libpref has callbacks. You can register a callback function against a particular pref (or pref prefix), and it'll be called when any matching pref changes. This is implemented in two layers. The lower layer is the list of CallbackNode objects, pointed to by gFirstCallback. The upper layer involves gObserverTable, which is a hash table of ValueObserver objects. It is used for callbacks registered via Preferences::RegisterCallback() and Preferences::Add*VarCache(), but not for observers registered with Preferences::Add{Weak,Strong}Observer(). If multiple callbacks with identical prefnames, callback functions and MatchKinds occur, they are commoned up into a single ValueObserver, which is then wrapped by a single CallbackNode. (The callbacks within a single ValueObserver can have different void* aClosure args; ValueObserver keeps those in an nsTArray.) Note also that gObserverTable is very inelegant, with duplication of data between the keys and the values due it being a nsRefPtrHashtable<ValueObserverHashKey, ValueObserver> and ValueObserver being a subclass of ValueObserverHashKey(!) This extra layer might make sense if there were a lot of commoning up happening, but there's not. Across all process kinds there is an average of between 1.2 and 1.7 closures per ValueObserver. The vast majority have 1 closure, and there are a handful that have multiple; the highest number I've seen is 71. (Note that this two-layer design probably seemed more natural back when libpref was spread across multiple files.) This patch removes the ValueObserver layer. The only tricky part is that there is a notion of a MatchKind -- ExactMatch or PrefixMatch -- which exists in the ValueObserverLayer but not in the CallbackNode layer. So the patch moves the MatchKind into the CallbackNode layer, which is straightforward. On Linux64 this reduces memory usage by about 40 KiB in the parent process and about 30 KiB in each content processes. The performance implications are minor. - The list of CallbackNodes is a bit longer, so when a pref is changed we must check more nodes. But we avoid possibly doing prefname matching twice. - Callback registration is much faster, just a simple list prepend, without any hash table insertion required. - Callback unregistration is probably not much different. There is a longer list to traverse, but no hash table removal. - Pref lookup is by far the hottest libpref operation, and it's unchanged. For perf to be genuinely affected would require (a) a *lot* more almost-duplicate callbacks that would have been commoned up, and (b) frequent changing of the pref those callbacks are observing. This seems highly unlikely. MozReview-Commit-ID: 6xo4xmytOf3
modules/libpref/Preferences.cpp
--- a/modules/libpref/Preferences.cpp
+++ b/modules/libpref/Preferences.cpp
@@ -293,16 +293,17 @@ struct CallbackNode
 {
   const char* mDomain;
 
   // If someone attempts to remove the node from the callback list while
   // pref_DoCallback is running, |func| is set to nullptr. Such nodes will
   // be removed at the end of pref_DoCallback.
   PrefChangedFunc mFunc;
   void* mData;
+  Preferences::MatchKind mMatchKind;
   CallbackNode* mNext;
 };
 
 static PLDHashTable* gHashTable;
 
 static ArenaAllocator<8192, 4> gPrefNameArena;
 
 // The callback list contains all the priority callbacks followed by the
@@ -1070,25 +1071,27 @@ PREF_PrefIsLocked(const char* aPrefName)
 }
 
 // Adds a node to the callback list; the position depends on aIsPriority. The
 // callback function will be called if anything below that node is modified.
 static void
 PREF_RegisterCallback(const char* aPrefNode,
                       PrefChangedFunc aCallback,
                       void* aData,
+                      Preferences::MatchKind aMatchKind,
                       bool aIsPriority)
 {
   NS_PRECONDITION(aPrefNode, "aPrefNode must not be nullptr");
   NS_PRECONDITION(aCallback, "aCallback must not be nullptr");
 
   auto node = (CallbackNode*)moz_xmalloc(sizeof(CallbackNode));
   node->mDomain = moz_xstrdup(aPrefNode);
   node->mFunc = aCallback;
   node->mData = aData;
+  node->mMatchKind = aMatchKind;
 
   if (aIsPriority) {
     // Add to the start of the list.
     node->mNext = gFirstCallback;
     gFirstCallback = node;
     if (!gLastPriorityNode) {
       gLastPriorityNode = node;
     }
@@ -1129,24 +1132,26 @@ pref_RemoveCallbackNode(CallbackNode* aN
   return next_node;
 }
 
 // Deletes a node from the callback list or marks it for deletion. Succeeds if
 // a callback was found that matched all the parameters.
 static nsresult
 PREF_UnregisterCallback(const char* aPrefNode,
                         PrefChangedFunc aCallback,
-                        void* aData)
+                        void* aData,
+                        Preferences::MatchKind aMatchKind)
 {
   nsresult rv = NS_ERROR_FAILURE;
   CallbackNode* node = gFirstCallback;
   CallbackNode* prev_node = nullptr;
 
   while (node != nullptr) {
     if (node->mFunc == aCallback && node->mData == aData &&
+        node->mMatchKind == aMatchKind &&
         strcmp(node->mDomain, aPrefNode) == 0) {
       if (gCallbacksInProgress) {
         // postpone the node removal until after
         // callbacks enumeration is finished.
         node->mFunc = nullptr;
         gShouldCleanupDeadNodes = true;
         prev_node = node;
         node = node->mNext;
@@ -1171,20 +1176,25 @@ pref_DoCallback(const char* aChangedPref
   bool reentered = gCallbacksInProgress;
 
   // Nodes must not be deleted while gCallbacksInProgress is true.
   // Nodes that need to be deleted are marked for deletion by nulling
   // out the |func| pointer. We release them at the end of this function
   // if we haven't reentered.
   gCallbacksInProgress = true;
 
-  for (node = gFirstCallback; node != nullptr; node = node->mNext) {
-    if (node->mFunc &&
-        PL_strncmp(aChangedPref, node->mDomain, strlen(node->mDomain)) == 0) {
-      (*node->mFunc)(aChangedPref, node->mData);
+  for (node = gFirstCallback; node; node = node->mNext) {
+    if (node->mFunc) {
+      bool matches =
+        node->mMatchKind == Preferences::ExactMatch
+          ? strcmp(node->mDomain, aChangedPref) == 0
+          : strncmp(node->mDomain, aChangedPref, strlen(node->mDomain)) == 0;
+      if (matches) {
+        (node->mFunc)(aChangedPref, node->mData);
+      }
     }
   }
 
   gCallbacksInProgress = reentered;
 
   if (gShouldCleanupDeadNodes && !gCallbacksInProgress) {
     CallbackNode* prev_node = nullptr;
     node = gFirstCallback;
@@ -2878,18 +2888,22 @@ nsPrefBranch::AddObserver(const char* aD
   }
 
   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);
-  PREF_RegisterCallback(
-    pref.get(), NotifyObserver, pCallback, /* isPriority */ false);
+  PREF_RegisterCallback(pref.get(),
+                        NotifyObserver,
+                        pCallback,
+                        Preferences::PrefixMatch,
+                        /* isPriority */ false);
+
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsPrefBranch::RemoveObserver(const char* aDomain, nsIObserver* aObserver)
 {
   NS_ENSURE_ARG(aDomain);
   NS_ENSURE_ARG(aObserver);
@@ -2911,17 +2925,18 @@ nsPrefBranch::RemoveObserver(const char*
   // 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);
   nsAutoPtr<PrefCallback> pCallback;
   mObservers.Remove(&key, &pCallback);
   if (pCallback) {
     // aDomain == nullptr is the only possible failure, trapped above.
     const PrefName& pref = GetPrefName(aDomain);
-    rv = PREF_UnregisterCallback(pref.get(), NotifyObserver, pCallback);
+    rv = PREF_UnregisterCallback(
+      pref.get(), NotifyObserver, pCallback, Preferences::PrefixMatch);
   }
 
   return rv;
 }
 
 NS_IMETHODIMP
 nsPrefBranch::Observe(nsISupports* aSubject,
                       const char* aTopic,
@@ -2973,17 +2988,20 @@ nsPrefBranch::FreeObserverList()
   // 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());
-    PREF_UnregisterCallback(pref.get(), nsPrefBranch::NotifyObserver, callback);
+    PREF_UnregisterCallback(pref.get(),
+                            nsPrefBranch::NotifyObserver,
+                            callback,
+                            Preferences::PrefixMatch);
     iter.Remove();
   }
   mFreeingObserverList = false;
 }
 
 void
 nsPrefBranch::RemoveExpiredCallback(PrefCallback* aCallback)
 {
@@ -3184,120 +3202,16 @@ static const char kPrefFileHeader[] =
 // clang-format on
 
 StaticRefPtr<Preferences> Preferences::sPreferences;
 bool Preferences::sShutdown = false;
 
 // This globally enables or disables OMT pref writing, both sync and async.
 static int32_t sAllowOMTPrefWrite = -1;
 
-class ValueObserverHashKey : public PLDHashEntryHdr
-{
-public:
-  typedef ValueObserverHashKey* KeyType;
-  typedef const ValueObserverHashKey* KeyTypePointer;
-
-  static const ValueObserverHashKey* KeyToPointer(ValueObserverHashKey* aKey)
-  {
-    return aKey;
-  }
-
-  static PLDHashNumber HashKey(const ValueObserverHashKey* aKey)
-  {
-    PLDHashNumber hash = HashString(aKey->mPrefName);
-    hash = AddToHash(hash, aKey->mMatchKind);
-    return AddToHash(hash, aKey->mCallback);
-  }
-
-  ValueObserverHashKey(const char* aPref,
-                       PrefChangedFunc aCallback,
-                       Preferences::MatchKind aMatchKind)
-    : mPrefName(aPref)
-    , mCallback(aCallback)
-    , mMatchKind(aMatchKind)
-  {
-  }
-
-  explicit ValueObserverHashKey(const ValueObserverHashKey* aOther)
-    : mPrefName(aOther->mPrefName)
-    , mCallback(aOther->mCallback)
-    , mMatchKind(aOther->mMatchKind)
-  {
-  }
-
-  bool KeyEquals(const ValueObserverHashKey* aOther) const
-  {
-    return mCallback == aOther->mCallback && mPrefName == aOther->mPrefName &&
-           mMatchKind == aOther->mMatchKind;
-  }
-
-  ValueObserverHashKey* GetKey() const
-  {
-    return const_cast<ValueObserverHashKey*>(this);
-  }
-
-  enum
-  {
-    ALLOW_MEMMOVE = true
-  };
-
-  nsCString mPrefName;
-  PrefChangedFunc mCallback;
-  Preferences::MatchKind mMatchKind;
-};
-
-class ValueObserver final
-  : public nsIObserver
-  , public ValueObserverHashKey
-{
-  ~ValueObserver() = default;
-
-public:
-  NS_DECL_ISUPPORTS
-  NS_DECL_NSIOBSERVER
-
-  ValueObserver(const char* aPref,
-                PrefChangedFunc aCallback,
-                Preferences::MatchKind aMatchKind)
-    : ValueObserverHashKey(aPref, aCallback, aMatchKind)
-  {
-  }
-
-  void AppendClosure(void* aClosure) { mClosures.AppendElement(aClosure); }
-
-  void RemoveClosure(void* aClosure) { mClosures.RemoveElement(aClosure); }
-
-  bool HasNoClosures() { return mClosures.Length() == 0; }
-
-  nsTArray<void*> mClosures;
-};
-
-NS_IMPL_ISUPPORTS(ValueObserver, nsIObserver)
-
-NS_IMETHODIMP
-ValueObserver::Observe(nsISupports* aSubject,
-                       const char* aTopic,
-                       const char16_t* aData)
-{
-  NS_ASSERTION(!nsCRT::strcmp(aTopic, NS_PREFBRANCH_PREFCHANGE_TOPIC_ID),
-               "invalid topic");
-
-  NS_ConvertUTF16toUTF8 data(aData);
-  if (mMatchKind == Preferences::ExactMatch &&
-      !mPrefName.EqualsASCII(data.get())) {
-    return NS_OK;
-  }
-
-  for (uint32_t i = 0; i < mClosures.Length(); i++) {
-    mCallback(data.get(), mClosures.ElementAt(i));
-  }
-
-  return NS_OK;
-}
-
 // Write the preference data to a file.
 class PreferencesWriter final
 {
 public:
   PreferencesWriter() = default;
 
   static nsresult Write(nsIFile* aFile, PrefSaveData& aPrefs)
   {
@@ -3436,18 +3350,16 @@ struct CacheData
     float mDefaultValueFloat;
   };
 };
 
 // gCacheDataDesc holds information about prefs startup. It's being used for
 // diagnosing prefs startup problems in bug 1276488.
 static const char* gCacheDataDesc = "untouched";
 static nsTArray<nsAutoPtr<CacheData>>* gCacheData = nullptr;
-static nsRefPtrHashtable<ValueObserverHashKey, ValueObserver>* gObserverTable =
-  nullptr;
 
 #ifdef DEBUG
 static bool
 HaveExistingCacheFor(void* aPtr)
 {
   MOZ_ASSERT(NS_IsMainThread());
   if (gCacheData) {
     for (size_t i = 0, count = gCacheData->Length(); i < count; ++i) {
@@ -3503,24 +3415,16 @@ Preferences::SizeOfIncludingThisAndOther
 
   if (gCacheData) {
     n += gCacheData->ShallowSizeOfIncludingThis(aMallocSizeOf);
     for (uint32_t i = 0, count = gCacheData->Length(); i < count; ++i) {
       n += aMallocSizeOf((*gCacheData)[i]);
     }
   }
 
-  if (gObserverTable) {
-    n += gObserverTable->ShallowSizeOfIncludingThis(aMallocSizeOf);
-    for (auto iter = gObserverTable->Iter(); !iter.Done(); iter.Next()) {
-      n += iter.Key()->mPrefName.SizeOfExcludingThisIfUnshared(aMallocSizeOf);
-      n += iter.Data()->mClosures.ShallowSizeOfExcludingThis(aMallocSizeOf);
-    }
-  }
-
   if (sPreferences->mRootBranch) {
     n += static_cast<nsPrefBranch*>(sPreferences->mRootBranch.get())
            ->SizeOfIncludingThis(aMallocSizeOf);
   }
 
   if (sPreferences->mDefaultRootBranch) {
     n += static_cast<nsPrefBranch*>(sPreferences->mDefaultRootBranch.get())
            ->SizeOfIncludingThis(aMallocSizeOf);
@@ -3685,18 +3589,16 @@ Preferences::GetInstanceForService()
     sPreferences = nullptr;
     gCacheDataDesc = res.unwrapErr();
     return nullptr;
   }
 
   gCacheData = new nsTArray<nsAutoPtr<CacheData>>();
   gCacheDataDesc = "set by GetInstanceForService()";
 
-  gObserverTable = new nsRefPtrHashtable<ValueObserverHashKey, ValueObserver>();
-
   // Preferences::GetInstanceForService() can be called from GetService(), and
   // RegisterStrongMemoryReporter calls GetService(nsIMemoryReporter).  To
   // avoid a potential recursive GetService() call, we can't register the
   // memory reporter here; instead, do it off a runnable.
   RefPtr<AddPreferencesMemoryReporterRunnable> runnable =
     new AddPreferencesMemoryReporterRunnable();
   NS_DispatchToMainThread(runnable);
 
@@ -3743,19 +3645,16 @@ Preferences::Preferences()
   , mDefaultRootBranch(new nsPrefBranch("", true))
 {
 }
 
 Preferences::~Preferences()
 {
   MOZ_ASSERT(!sPreferences);
 
-  delete gObserverTable;
-  gObserverTable = nullptr;
-
   delete gCacheData;
   gCacheData = nullptr;
 
   PREF_Cleanup();
 }
 
 //
 // nsISupports Implementation
@@ -4920,72 +4819,42 @@ Preferences::RemoveObservers(nsIObserver
 
   for (uint32_t i = 0; aPrefs[i]; i++) {
     nsresult rv = RemoveObserver(aObserver, aPrefs[i]);
     NS_ENSURE_SUCCESS(rv, rv);
   }
   return NS_OK;
 }
 
-static void
-NotifyObserver(const char* aPref, void* aClosure)
-{
-  nsCOMPtr<nsIObserver> observer = static_cast<nsIObserver*>(aClosure);
-  observer->Observe(nullptr,
-                    NS_PREFBRANCH_PREFCHANGE_TOPIC_ID,
-                    NS_ConvertASCIItoUTF16(aPref).get());
-}
-
-static void
-RegisterCallbackHelper(PrefChangedFunc aCallback,
-                       const char* aPref,
-                       void* aClosure,
-                       Preferences::MatchKind aMatchKind,
-                       bool aIsPriority)
-{
-  ValueObserverHashKey hashKey(aPref, aCallback, aMatchKind);
-  RefPtr<ValueObserver> observer;
-  gObserverTable->Get(&hashKey, getter_AddRefs(observer));
-  if (observer) {
-    observer->AppendClosure(aClosure);
-    return;
-  }
-
-  observer = new ValueObserver(aPref, aCallback, aMatchKind);
-  observer->AppendClosure(aClosure);
-  PREF_RegisterCallback(
-    aPref, NotifyObserver, static_cast<nsIObserver*>(observer), aIsPriority);
-  gObserverTable->Put(observer, observer);
-}
-
 // RegisterVarCacheCallback uses high priority callbacks to ensure that cache
 // observers are called prior to ordinary pref observers. Doing this ensures
 // that ordinary observers will never get stale values from cache variables.
 static void
 RegisterVarCacheCallback(PrefChangedFunc aCallback,
                          const char* aPref,
                          void* aClosure)
 {
   MOZ_ASSERT(Preferences::IsServiceAvailable());
 
-  RegisterCallbackHelper(
-    aCallback, aPref, aClosure, Preferences::ExactMatch, /* isPriority */ true);
+  PREF_RegisterCallback(
+    aPref, aCallback, aClosure, Preferences::ExactMatch, /* isPriority */ true);
 }
 
 /* static */ nsresult
 Preferences::RegisterCallback(PrefChangedFunc aCallback,
                               const char* aPref,
                               void* aClosure,
                               MatchKind aMatchKind)
 {
   MOZ_ASSERT(aCallback);
   NS_ENSURE_TRUE(InitStaticMembers(), NS_ERROR_NOT_AVAILABLE);
 
-  RegisterCallbackHelper(
-    aCallback, aPref, aClosure, aMatchKind, /* isPriority */ false);
+  PREF_RegisterCallback(
+    aPref, aCallback, aClosure, aMatchKind, /* isPriority */ false);
+
   return NS_OK;
 }
 
 /* static */ nsresult
 Preferences::RegisterCallbackAndCall(PrefChangedFunc aCallback,
                                      const char* aPref,
                                      void* aClosure,
                                      MatchKind aMatchKind)
@@ -5006,32 +4875,17 @@ Preferences::UnregisterCallback(PrefChan
                                 MatchKind aMatchKind)
 {
   MOZ_ASSERT(aCallback);
   if (!sPreferences && sShutdown) {
     return NS_OK; // Observers have been released automatically.
   }
   NS_ENSURE_TRUE(sPreferences, NS_ERROR_NOT_AVAILABLE);
 
-  ValueObserverHashKey hashKey(aPref, aCallback, aMatchKind);
-  RefPtr<ValueObserver> observer;
-  gObserverTable->Get(&hashKey, getter_AddRefs(observer));
-  if (!observer) {
-    return NS_OK;
-  }
-
-  observer->RemoveClosure(aClosure);
-  if (observer->HasNoClosures()) {
-    // Delete the callback since its list of closures is empty.
-    MOZ_ALWAYS_SUCCEEDS(
-      PREF_UnregisterCallback(aPref, NotifyObserver, observer));
-
-    gObserverTable->Remove(observer);
-  }
-  return NS_OK;
+  return PREF_UnregisterCallback(aPref, aCallback, aClosure, aMatchKind);
 }
 
 static void
 BoolVarChanged(const char* aPref, void* aClosure)
 {
   CacheData* cache = static_cast<CacheData*>(aClosure);
   *static_cast<bool*>(cache->mCacheLocation) =
     Preferences::GetBool(aPref, cache->mDefaultValueBool);