Bug 1421704 - Optimize NotifyVisited IPC to take batch r?mak draft
authorDoug Thayer <dothayer@mozilla.com>
Mon, 04 Dec 2017 09:45:29 -0800
changeset 711714 8ad502463573bd29a7f1e0487750dca5362a9404
parent 711178 defccba824aa91e8d4d820b1defaadfdca34bac7
child 743852 46a2b1136d7533ca672d279b7ad74fd3fc97aa7b
push id93119
push userbmo:dothayer@mozilla.com
push dateThu, 14 Dec 2017 15:43:22 +0000
reviewersmak
bugs1421704
milestone59.0a1
Bug 1421704 - Optimize NotifyVisited IPC to take batch r?mak During history import, sending NotifyVisited messages from the chrome process to the content processes in order to change link colors can take a significant portion of the parent process's main thread time. Batching it seems to have very significant results on jank time during history imports. MozReview-Commit-ID: BHAXpIMa7ly
dom/ipc/ContentChild.cpp
dom/ipc/ContentChild.h
dom/ipc/PContent.ipdl
toolkit/components/places/History.cpp
toolkit/components/places/History.h
--- a/dom/ipc/ContentChild.cpp
+++ b/dom/ipc/ContentChild.cpp
@@ -2447,25 +2447,27 @@ ContentChild::RecvNotifyAlertsObserver(c
     ++i;
   }
   return IPC_OK();
 }
 
 // NOTE: This method is being run in the SystemGroup, and thus cannot directly
 // touch pages. See GetSpecificMessageEventTarget.
 mozilla::ipc::IPCResult
-ContentChild::RecvNotifyVisited(const URIParams& aURI)
+ContentChild::RecvNotifyVisited(nsTArray<URIParams>&& aURIs)
 {
-  nsCOMPtr<nsIURI> newURI = DeserializeURI(aURI);
-  if (!newURI) {
-    return IPC_FAIL_NO_REASON(this);
-  }
-  nsCOMPtr<IHistory> history = services::GetHistoryService();
-  if (history) {
-    history->NotifyVisited(newURI);
+  for (const URIParams& uri : aURIs) {
+    nsCOMPtr<nsIURI> newURI = DeserializeURI(uri);
+    if (!newURI) {
+      return IPC_FAIL_NO_REASON(this);
+    }
+    nsCOMPtr<IHistory> history = services::GetHistoryService();
+    if (history) {
+      history->NotifyVisited(newURI);
+    }
   }
   return IPC_OK();
 }
 
 mozilla::ipc::IPCResult
 ContentChild::RecvLoadProcessScript(const nsString& aURL)
 {
   ProcessGlobal* global = ProcessGlobal::Get();
--- a/dom/ipc/ContentChild.h
+++ b/dom/ipc/ContentChild.h
@@ -357,17 +357,17 @@ public:
   virtual mozilla::ipc::IPCResult RecvSetOffline(const bool& offline) override;
 
   virtual mozilla::ipc::IPCResult RecvSetConnectivity(const bool& connectivity) override;
   virtual mozilla::ipc::IPCResult RecvSetCaptivePortalState(const int32_t& state) override;
 
   virtual mozilla::ipc::IPCResult RecvBidiKeyboardNotify(const bool& isLangRTL,
                                                          const bool& haveBidiKeyboards) override;
 
-  virtual mozilla::ipc::IPCResult RecvNotifyVisited(const URIParams& aURI) override;
+  virtual mozilla::ipc::IPCResult RecvNotifyVisited(nsTArray<URIParams>&& aURIs) override;
 
   // auto remove when alertfinished is received.
   nsresult AddRemoteAlertObserver(const nsString& aData, nsIObserver* aObserver);
 
   virtual mozilla::ipc::IPCResult RecvPreferenceUpdate(const Pref& aPref) override;
   virtual mozilla::ipc::IPCResult RecvVarUpdate(const GfxVarUpdate& pref) override;
 
   virtual mozilla::ipc::IPCResult RecvDataStoragePut(const nsString& aFilename,
--- a/dom/ipc/PContent.ipdl
+++ b/dom/ipc/PContent.ipdl
@@ -419,17 +419,17 @@ child:
     async RegisterChromeItem(ChromeRegistryItem item);
 
     async ClearImageCache(bool privateLoader, bool chrome);
 
     async SetOffline(bool offline);
     async SetConnectivity(bool connectivity);
     async SetCaptivePortalState(int32_t aState);
 
-    async NotifyVisited(URIParams uri);
+    async NotifyVisited(URIParams[] uri);
 
     async PreferenceUpdate(Pref pref);
     async VarUpdate(GfxVarUpdate var);
 
     async DataStoragePut(nsString aFilename, DataStorageItem aItem);
     async DataStorageRemove(nsString aFilename, nsCString aKey, DataStorageType aType);
     async DataStorageClear(nsString aFilename);
 
--- a/toolkit/components/places/History.cpp
+++ b/toolkit/components/places/History.cpp
@@ -570,16 +570,21 @@ public:
       mCallback->IsVisited(mURI, mIsVisited);
       return NS_OK;
     }
 
     if (mIsVisited) {
       History* history = History::GetService();
       NS_ENSURE_STATE(history);
       history->NotifyVisited(mURI);
+      AutoTArray<URIParams, 1> uris;
+      URIParams uri;
+      SerializeURI(mURI, uri);
+      uris.AppendElement(Move(uri));
+      history->NotifyVisitedParent(uris);
     }
 
     nsCOMPtr<nsIObserverService> observerService =
       mozilla::services::GetObserverService();
     if (observerService) {
       nsAutoString status;
       if (mIsVisited) {
         status.AssignLiteral(URI_VISITED);
@@ -638,16 +643,17 @@ public:
     , mHistory(History::GetService())
   {
     aPlaces.SwapElements(mPlaces);
   }
 
   nsresult NotifyVisit(nsNavHistory* aNavHistory,
                        nsCOMPtr<nsIObserverService>& aObsService,
                        PRTime aNow,
+                       nsTArray<URIParams>& aNotifyVisitedURIs,
                        VisitData aPlace) {
     nsCOMPtr<nsIURI> uri;
     MOZ_ALWAYS_SUCCEEDS(NS_NewURI(getter_AddRefs(uri), aPlace.spec));
     if (!uri) {
       return NS_ERROR_UNEXPECTED;
     }
     // Notify the visit.  Note that TRANSITION_EMBED visits are never added
     // to the database, thus cannot be queried and we don't notify them.
@@ -670,16 +676,20 @@ public:
       mHistory->AppendToRecentlyVisitedURIs(uri);
     }
     mHistory->NotifyVisited(uri);
 
     if (aPlace.titleChanged) {
       aNavHistory->NotifyTitleChange(uri, aPlace.title, aPlace.guid);
     }
 
+    URIParams serialized;
+    SerializeURI(uri, serialized);
+    aNotifyVisitedURIs.AppendElement(Move(serialized));
+
     return NS_OK;
   }
 
   NS_IMETHOD Run() override
   {
     MOZ_ASSERT(NS_IsMainThread(), "This should be called on the main thread");
 
     // We are in the main thread, no need to lock.
@@ -694,23 +704,27 @@ public:
       return NS_OK;
     }
 
     nsCOMPtr<nsIObserverService> obsService =
       mozilla::services::GetObserverService();
 
     PRTime now = PR_Now();
     if (mPlaces.Length() > 0) {
+      InfallibleTArray<URIParams> uris(mPlaces.Length());
       for (uint32_t i = 0; i < mPlaces.Length(); ++i) {
-        nsresult rv = NotifyVisit(navHistory, obsService, now, mPlaces[i]);
+        nsresult rv = NotifyVisit(navHistory, obsService, now, uris, mPlaces[i]);
         NS_ENSURE_SUCCESS(rv, rv);
       }
+      mHistory->NotifyVisitedParent(uris);
     } else {
-      nsresult rv = NotifyVisit(navHistory, obsService, now, mPlace);
+      AutoTArray<URIParams, 1> uris;
+      nsresult rv = NotifyVisit(navHistory, obsService, now, uris, mPlace);
       NS_ENSURE_SUCCESS(rv, rv);
+      mHistory->NotifyVisitedParent(uris);
     }
 
     return NS_OK;
   }
 private:
   nsTArray<VisitData> mPlaces;
   VisitData mPlace;
   RefPtr<History> mHistory;
@@ -1993,39 +2007,40 @@ GetLinkDocument(Link* aLink)
 {
   // NOTE: Theoretically GetElement should never return nullptr, but it does
   // in GTests because they use a mock_Link which returns null from this
   // method.
   Element* element = aLink->GetElement();
   return element ? element->OwnerDoc() : nullptr;
 }
 
+void
+History::NotifyVisitedParent(const nsTArray<URIParams>& aURIs)
+{
+  MOZ_ASSERT(XRE_IsParentProcess());
+  nsTArray<ContentParent*> cplist;
+  ContentParent::GetAll(cplist);
+
+  if (!cplist.IsEmpty()) {
+    for (uint32_t i = 0; i < cplist.Length(); ++i) {
+      Unused << cplist[i]->SendNotifyVisited(aURIs);
+    }
+  }
+}
+
 NS_IMETHODIMP
 History::NotifyVisited(nsIURI* aURI)
 {
   MOZ_ASSERT(NS_IsMainThread());
   NS_ENSURE_ARG(aURI);
   // NOTE: This can be run within the SystemGroup, and thus cannot directly
   // interact with webpages.
 
   nsAutoScriptBlocker scriptBlocker;
 
-  if (XRE_IsParentProcess()) {
-    nsTArray<ContentParent*> cplist;
-    ContentParent::GetAll(cplist);
-
-    if (!cplist.IsEmpty()) {
-      URIParams uri;
-      SerializeURI(aURI, uri);
-      for (uint32_t i = 0; i < cplist.Length(); ++i) {
-        Unused << cplist[i]->SendNotifyVisited(uri);
-      }
-    }
-  }
-
   // If we have no observers for this URI, we have nothing to notify about.
   KeyClass* key = mObservers.GetEntry(aURI);
   if (!key) {
     return NS_OK;
   }
   key->mVisited = true;
 
   // If we have a key, it should have at least one observer.
--- a/toolkit/components/places/History.h
+++ b/toolkit/components/places/History.h
@@ -10,16 +10,17 @@
 #include "mozilla/IHistory.h"
 #include "mozilla/MemoryReporting.h"
 #include "mozilla/Mutex.h"
 #include "mozIAsyncHistory.h"
 #include "nsIDownloadHistory.h"
 #include "Database.h"
 
 #include "mozilla/dom/Link.h"
+#include "mozilla/ipc/URIParams.h"
 #include "nsTHashtable.h"
 #include "nsString.h"
 #include "nsURIHashKey.h"
 #include "nsTObserverArray.h"
 #include "nsDeque.h"
 #include "nsIMemoryReporter.h"
 #include "nsIObserver.h"
 #include "mozIStorageConnection.h"
@@ -138,16 +139,17 @@ public:
   }
 
   /**
    * Helper function to append a new URI to mRecentlyVisitedURIs. See
    * mRecentlyVisitedURIs.
    */
   void AppendToRecentlyVisitedURIs(nsIURI* aURI);
 
+  void NotifyVisitedParent(const nsTArray<mozilla::ipc::URIParams>& aURIs);
 private:
   virtual ~History();
 
   void InitMemoryReporter();
 
   /**
    * Obtains a read-write database connection, initializing the connection
    * if needed. Must be invoked on the main thread.