Bug 1471025: Part 6 - Optimize preference lookups while dispatching callbacks. r=njn draft
authorKris Maglione <maglione.k@gmail.com>
Sat, 07 Jul 2018 12:47:34 -0700
changeset 815379 8bd27d0b04812c453e759ed0397330be61ec8aef
parent 815378 cf66a73448f52b17b0ec773da30b4945b1793e21
child 815380 3a0b14b81e335dd1514cf075fe3d525cc0a88bd2
push id115503
push usermaglione.k@gmail.com
push dateSat, 07 Jul 2018 19:50:37 +0000
reviewersnjn
bugs1471025
milestone63.0a1
Bug 1471025: Part 6 - Optimize preference lookups while dispatching callbacks. r=njn Since lookups in the snapshotted hashtable are currently O(log n) rather than O(1), they're expected to be slightly more expensive than the previous purely-dynamic lookups. In general, I expect this not to be a major issue. The main concern is pref cache lookups while initializing the database. Initialization in the parent process will still always use only a dynamic hashtable, so the performance characteristics there won't change. In the child process, though, we'll still need to notify observers of preferences in the snapshot when they have user values, which could require any number of lookups at startup (though in practice probably will not). This patch solves that problem by caching the wrapper for the current known preference value when dispatching callbacks, and optimizing lookups for that value when it is present. MozReview-Commit-ID: 2CAn0rM0bE9
modules/libpref/Preferences.cpp
--- a/modules/libpref/Preferences.cpp
+++ b/modules/libpref/Preferences.cpp
@@ -1466,17 +1466,23 @@ public:
 
   Elem end() { return { *this, true }; }
 };
 
 static Pref*
 pref_HashTableLookup(const char* aPrefName);
 
 static void
-NotifyCallbacks(const char* aPrefName);
+NotifyCallbacks(const char* aPrefName, const PrefWrapper* aPref = nullptr);
+
+static void
+NotifyCallbacks(const char* aPrefName, const PrefWrapper& aPref)
+{
+  NotifyCallbacks(aPrefName, &aPref);
+}
 
 #define PREF_HASHTABLE_INITIAL_LENGTH 1024
 
 static PrefSaveData
 pref_savePrefs()
 {
   MOZ_ASSERT(NS_IsMainThread());
 
@@ -1526,24 +1532,35 @@ pref_HashTableLookup(const char* aPrefNa
   PrefEntry* entry = pref_HashTableLookupInner(aPrefName);
   if (!entry) {
     return nullptr;
   }
 
   return entry->mPref;
 }
 
+// While notifying preference callbacks, this holds the wrapper for the
+// preference being notified, in order to optimize lookups.
+//
+// Note: Callbacks and lookups only happen on the main thread, so this is safe
+// to use without locking.
+static const PrefWrapper* gCallbackPref;
+
 Maybe<PrefWrapper>
 pref_Lookup(const char* aPrefName)
 {
   Maybe<PrefWrapper> result;
 
+  MOZ_ASSERT(NS_IsMainThread());
+
   AddAccessCount(aPrefName);
 
-  if (Pref* pref = pref_HashTableLookup(aPrefName)) {
+  if (gCallbackPref && strcmp(aPrefName, gCallbackPref->Name()) == 0) {
+    result.emplace(*gCallbackPref);
+  } else if (Pref* pref = pref_HashTableLookup(aPrefName)) {
     result.emplace(pref);
   }
 
   return result;
 }
 
 static nsresult
 pref_SetPref(const char* aPrefName,
@@ -1592,17 +1609,17 @@ pref_SetPref(const char* aPrefName,
 
     return rv;
   }
 
   if (valueChanged) {
     if (aKind == PrefValueKind::User && XRE_IsParentProcess()) {
       Preferences::HandleDirty();
     }
-    NotifyCallbacks(aPrefName);
+    NotifyCallbacks(aPrefName, PrefWrapper(pref));
   }
 
   return NS_OK;
 }
 
 // Removes |node| from callback list. Returns the node after the deleted one.
 static CallbackNode*
 pref_RemoveCallbackNode(CallbackNode* aNode, CallbackNode* aPrevNode)
@@ -1620,20 +1637,23 @@ pref_RemoveCallbackNode(CallbackNode* aN
   if (gLastPriorityNode == aNode) {
     gLastPriorityNode = aPrevNode;
   }
   delete aNode;
   return next_node;
 }
 
 static void
-NotifyCallbacks(const char* aPrefName)
+NotifyCallbacks(const char* aPrefName, const PrefWrapper* aPref)
 {
   bool reentered = gCallbacksInProgress;
 
+  gCallbackPref = aPref;
+  auto cleanup = MakeScopeExit([]() { gCallbackPref = nullptr; });
+
   // 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;
 
   nsDependentCString prefName(aPrefName);
 
@@ -4118,17 +4138,17 @@ Preferences::SetPreference(const dom::Pr
   if (!pref->HasDefaultValue() && !pref->HasUserValue()) {
     gHashTable->RemoveEntry(entry);
   }
 
   // Note: we don't have to worry about HandleDirty() because we are setting
   // prefs in the content process that have come from the parent process.
 
   if (valueChanged) {
-    NotifyCallbacks(prefName);
+    NotifyCallbacks(prefName, PrefWrapper(pref));
   }
 }
 
 /* static */ void
 Preferences::GetPreference(dom::Pref* aDomPref)
 {
   MOZ_ASSERT(XRE_IsParentProcess());
 
@@ -4937,17 +4957,17 @@ Preferences::Lock(const char* aPrefName)
 
   Pref* pref = pref_HashTableLookup(aPrefName);
   if (!pref) {
     return NS_ERROR_UNEXPECTED;
   }
 
   if (!pref->IsLocked()) {
     pref->SetIsLocked(true);
-    NotifyCallbacks(aPrefName);
+    NotifyCallbacks(aPrefName, PrefWrapper(pref));
   }
 
   return NS_OK;
 }
 
 /* static */ nsresult
 Preferences::Unlock(const char* aPrefName)
 {
@@ -4956,17 +4976,17 @@ Preferences::Unlock(const char* aPrefNam
 
   Pref* pref = pref_HashTableLookup(aPrefName);
   if (!pref) {
     return NS_ERROR_UNEXPECTED;
   }
 
   if (pref->IsLocked()) {
     pref->SetIsLocked(false);
-    NotifyCallbacks(aPrefName);
+    NotifyCallbacks(aPrefName, PrefWrapper(pref));
   }
 
   return NS_OK;
 }
 
 /* static */ bool
 Preferences::IsLocked(const char* aPrefName)
 {