From ef40e9d07216f998adad4e43060f1896f12b73d3 Mon Sep 17 00:00:00 2001 From: Gabor Krizsanits Date: Thu, 29 Jun 2017 21:44:40 +0200 Subject: [PATCH] Bug 1373660 - Block the preallocated process manager while a content process is being launched. We should not let the ppm to do work before the first paint in a new cp. This patch makes sure that we only let the ppm spawn a new process after the last process reached an idle state AND the main process becomes idle too. r=mrbkap --- dom/ipc/ContentParent.cpp | 12 ++-- dom/ipc/PreallocatedProcessManager.cpp | 94 ++++++++++++++++---------- dom/ipc/PreallocatedProcessManager.h | 27 ++------ 3 files changed, 71 insertions(+), 62 deletions(-) diff --git a/dom/ipc/ContentParent.cpp b/dom/ipc/ContentParent.cpp index 6b8d0203e551..c3cfe08a0f83 100644 --- a/dom/ipc/ContentParent.cpp +++ b/dom/ipc/ContentParent.cpp @@ -603,6 +603,8 @@ ContentParent::PreallocateProcess() new ContentParent(/* aOpener = */ nullptr, NS_LITERAL_STRING(DEFAULT_REMOTE_TYPE)); + PreallocatedProcessManager::AddBlocker(process); + if (!process->LaunchSubprocess(PROCESS_PRIORITY_PREALLOC)) { return nullptr; } @@ -880,6 +882,9 @@ ContentParent::GetNewOrUsedBrowserProcess(const nsAString& aRemoteType, // Create a new process from scratch. RefPtr p = new ContentParent(aOpener, aRemoteType); + // Until the new process is ready let's not allow to start up any preallocated processes. + PreallocatedProcessManager::AddBlocker(p); + if (!p->LaunchSubprocess(aPriority)) { return nullptr; } @@ -2727,10 +2732,9 @@ mozilla::ipc::IPCResult ContentParent::RecvFirstIdle() { // When the ContentChild goes idle, it sends us a FirstIdle message - // which we use as a good time to prelaunch another process. If we - // prelaunch any sooner than this, then we'll be competing with the - // child process and slowing it down. - PreallocatedProcessManager::AllocateAfterDelay(); + // which we use as a good time to signal the PreallocatedProcessManager + // that it can start allocating processes from now on. + PreallocatedProcessManager::RemoveBlocker(this); return IPC_OK(); } diff --git a/dom/ipc/PreallocatedProcessManager.cpp b/dom/ipc/PreallocatedProcessManager.cpp index 77fc8beb8269..a1f9a9ed0686 100644 --- a/dom/ipc/PreallocatedProcessManager.cpp +++ b/dom/ipc/PreallocatedProcessManager.cpp @@ -39,9 +39,8 @@ public: NS_DECL_NSIOBSERVER // See comments on PreallocatedProcessManager for these methods. - void AllocateAfterDelay(); - void AllocateOnIdle(); - void AllocateNow(); + void AddBlocker(ContentParent* aParent); + void RemoveBlocker(ContentParent* aParent); already_AddRefed Take(); bool Provide(ContentParent* aParent); @@ -54,6 +53,11 @@ private: void Init(); + bool CanAllocate(); + void AllocateAfterDelay(); + void AllocateOnIdle(); + void AllocateNow(); + void RereadPrefs(); void Enable(); void Disable(); @@ -64,6 +68,7 @@ private: bool mEnabled; bool mShutdown; RefPtr mPreallocatedProcess; + nsTHashtable mBlockers; }; /* static */ StaticRefPtr @@ -79,15 +84,6 @@ PreallocatedProcessManagerImpl::Singleton() ClearOnShutdown(&sSingleton); } - // First time when we init sSingleton, the pref database might not be in a - // reliable state (we are too early), so despite dom.ipc.processPrelaunch.enabled - // is set to true Preferences::GetBool will return false (it cannot find the pref). - // Since Init() above will be called only once, and the pref will not be changed, - // the manger will stay disabled. To prevent that let's re-read the pref each time - // someone accessing the manager singleton. This is a hack but this is not a hot code - // so it should be fine. - sSingleton->RereadPrefs(); - return sSingleton; } @@ -168,6 +164,11 @@ PreallocatedProcessManagerImpl::Take() return nullptr; } + if (mPreallocatedProcess) { + // The preallocated process is taken. Let's try to start up a new one soon. + AllocateOnIdle(); + } + return mPreallocatedProcess.forget(); } @@ -195,20 +196,46 @@ PreallocatedProcessManagerImpl::Enable() AllocateAfterDelay(); } +void +PreallocatedProcessManagerImpl::AddBlocker(ContentParent* aParent) +{ + uint64_t childID = aParent->ChildID(); + MOZ_ASSERT(!mBlockers.Contains(childID)); + mBlockers.PutEntry(childID); +} + +void +PreallocatedProcessManagerImpl::RemoveBlocker(ContentParent* aParent) +{ + uint64_t childID = aParent->ChildID(); + MOZ_ASSERT(mBlockers.Contains(childID)); + mBlockers.RemoveEntry(childID); + if (!mPreallocatedProcess && mBlockers.IsEmpty()) { + AllocateAfterDelay(); + } +} + +bool +PreallocatedProcessManagerImpl::CanAllocate() +{ + return mEnabled && + mBlockers.IsEmpty() && + !mPreallocatedProcess && + !mShutdown && + !ContentParent::IsMaxProcessCountReached(NS_LITERAL_STRING(DEFAULT_REMOTE_TYPE)); +} + void PreallocatedProcessManagerImpl::AllocateAfterDelay() { - if (!mEnabled || mPreallocatedProcess || mShutdown) { + if (!mEnabled) { return; } - // Originally AllocateOnIdle() was post here, but since the gecko parent - // message loop in practice never goes idle, that didn't work out well. - // Let's just launch the process after the delay. NS_DelayedDispatchToCurrentThread( - NewRunnableMethod("PreallocatedProcessManagerImpl::AllocateNow", + NewRunnableMethod("PreallocatedProcessManagerImpl::AllocateOnIdle", this, - &PreallocatedProcessManagerImpl::AllocateNow), + &PreallocatedProcessManagerImpl::AllocateOnIdle), Preferences::GetUint("dom.ipc.processPrelaunch.delayMs", DEFAULT_ALLOCATE_DELAY)); } @@ -216,7 +243,7 @@ PreallocatedProcessManagerImpl::AllocateAfterDelay() void PreallocatedProcessManagerImpl::AllocateOnIdle() { - if (!mEnabled || mPreallocatedProcess || mShutdown) { + if (!mEnabled) { return; } @@ -229,8 +256,11 @@ PreallocatedProcessManagerImpl::AllocateOnIdle() void PreallocatedProcessManagerImpl::AllocateNow() { - if (!mEnabled || mPreallocatedProcess || mShutdown || - ContentParent::IsMaxProcessCountReached(NS_LITERAL_STRING(DEFAULT_REMOTE_TYPE))) { + if (!CanAllocate()) { + if (mEnabled && !mPreallocatedProcess && !mBlockers.IsEmpty()) { + // If it's too early to allocate a process let's retry later. + AllocateAfterDelay(); + } return; } @@ -260,10 +290,6 @@ PreallocatedProcessManagerImpl::CloseProcess() void PreallocatedProcessManagerImpl::ObserveProcessShutdown(nsISupports* aSubject) { - if (!mPreallocatedProcess) { - return; - } - nsCOMPtr props = do_QueryInterface(aSubject); NS_ENSURE_TRUE_VOID(props); @@ -271,9 +297,11 @@ PreallocatedProcessManagerImpl::ObserveProcessShutdown(nsISupports* aSubject) props->GetPropertyAsUint64(NS_LITERAL_STRING("childID"), &childID); NS_ENSURE_TRUE_VOID(childID != CONTENT_PROCESS_ID_UNKNOWN); - if (childID == mPreallocatedProcess->ChildID()) { + if (mPreallocatedProcess && childID == mPreallocatedProcess->ChildID()) { mPreallocatedProcess = nullptr; } + + mBlockers.RemoveEntry(childID); } inline PreallocatedProcessManagerImpl* GetPPMImpl() @@ -282,21 +310,15 @@ inline PreallocatedProcessManagerImpl* GetPPMImpl() } /* static */ void -PreallocatedProcessManager::AllocateAfterDelay() +PreallocatedProcessManager::AddBlocker(ContentParent* aParent) { - GetPPMImpl()->AllocateAfterDelay(); + GetPPMImpl()->AddBlocker(aParent); } /* static */ void -PreallocatedProcessManager::AllocateOnIdle() +PreallocatedProcessManager::RemoveBlocker(ContentParent* aParent) { - GetPPMImpl()->AllocateOnIdle(); -} - -/* static */ void -PreallocatedProcessManager::AllocateNow() -{ - GetPPMImpl()->AllocateNow(); + GetPPMImpl()->RemoveBlocker(aParent); } /* static */ already_AddRefed diff --git a/dom/ipc/PreallocatedProcessManager.h b/dom/ipc/PreallocatedProcessManager.h index 0e2e005e8fca..d46377dc86df 100644 --- a/dom/ipc/PreallocatedProcessManager.h +++ b/dom/ipc/PreallocatedProcessManager.h @@ -33,31 +33,14 @@ class PreallocatedProcessManager final typedef mozilla::dom::ContentParent ContentParent; public: - /** - * Create a process after a delay. We wait for a period of time (specified - * by the dom.ipc.processPrelaunch.delayMs pref), then wait for this process - * to go idle, then allocate the new process. - * - * If the dom.ipc.processPrelaunch.enabled pref is false, or if we already - * have a preallocated process, this function does nothing. - */ - static void AllocateAfterDelay(); /** - * Create a process once this process goes idle. - * - * If the dom.ipc.processPrelaunch.enabled pref is false, or if we already - * have a preallocated process, this function does nothing. + * Before first paint we don't want to allocate any processes in the background. + * To avoid that, the PreallocatedProcessManager won't start up any processes while + * there is a blocker active. */ - static void AllocateOnIdle(); - - /** - * Create a process right now. - * - * If the dom.ipc.processPrelaunch.enabled pref is false, or if we already - * have a preallocated process, this function does nothing. - */ - static void AllocateNow(); + static void AddBlocker(ContentParent* aParent); + static void RemoveBlocker(ContentParent* aParent); /** * Take the preallocated process, if we have one. If we don't have one, this