1366356 - Decrease the process priority of content processes that are not running a foreground tab

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, enhancement)

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Having some API to notify processes when they have some foreground tabs or no foreground tabs at all would be really nice. We could for example try to do compacting GC a bit more aggressively on background processes.

Comment 2

[Gabor Krizsanits (INACTIVE)]

We have a dom/ipc/ProcessPriorityManager that is supposed to fiddle with process priorities but currently is pretty much useless dead code that does not do anything at all on platform. I was about to remove that... do you plan to use that code?

I'm not a big fan of process priority setting in general and would prefer a different solution to handle background tabs. We own every message/event loop and every threads in all content processes, so we should not wait for the OS to decide what to slow down and how, and let that be different on all platforms.

Comment 3

[Markus Stange [:mstange]]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #2)
> I'm not a big fan of process priority setting in general and would prefer a
> different solution to handle background tabs. We own every message/event
> loop and every threads in all content processes, so we should not wait for
> the OS to decide what to slow down and how, and let that be different on all
> platforms.

I don't really understand how this would work. You still want multiple content processes to be able to run code in parallel, don't you? And if those parallel executions compete for resources, you want to give the OS a hint about which one it should prioritize.

Comment 4

[Gabor Krizsanits (INACTIVE)]

(In reply to Markus Stange [:mstange] from comment #3)
> (In reply to Gabor Krizsanits [:krizsa :gabor] from comment #2)
> > I'm not a big fan of process priority setting in general and would prefer a
> > different solution to handle background tabs. We own every message/event
> > loop and every threads in all content processes, so we should not wait for
> > the OS to decide what to slow down and how, and let that be different on all
> > platforms.
> 
> I don't really understand how this would work. You still want multiple
> content processes to be able to run code in parallel, don't you? And if
> those parallel executions compete for resources, you want to give the OS a
> hint about which one it should prioritize.

The processes related to background tabs can go to sleep frequently for a short interval giving more CPU time for the process of the front tab doing his work. I think it should be a goal not to get into a situation where our browser hogs all the CPU's and we have to juggle which of the tabs starve less while we render the users machine useless. With multiple content processes where each process hosts multiple main threads I think it's essential to think about load balancing sooner than later and just setting the process priority at OS level won't be enough. I'm not saying though, that we should not do that too, that's why I asked Ehsan if he wants to keep the old process priority manager and transform it into something that is functional or wants to do a fresh start.

Comment 5

[(no longer active)]

I think starting with the existing code makes sense.  baku, is this something you would be interested to work on?

Comment 6

[Andrea Marchesini [:baku]]

Yes, I take this.

Comment 7

[Andrea Marchesini [:baku]]

Attachment: visibility.patch

After several cleaning steps, I think this class is finally ready to be used on desktop. It is still behind pref, but enabling it should be OK, because we don't have yet the SetProcessPriority() implementation for linux/mac/windows.

Doing some research, the next step is to use:

. on linux: setpriority. See https://linux.die.net/man/2/setpriority
. on mac: task_policy_set. See https://developer.apple.com/library/content/documentation/Darwin/Conceptual/KernelProgramming/scheduler/scheduler.html
. on windows: SetPriorityClass. See https://msdn.microsoft.com/en-us/library/windows/desktop/ms685100(v=vs.85).aspx

Comment 8

[Andrea Marchesini [:baku]]

Attachment: visibility.patch

Comment 9

[Gabriele Svelto [:gsvelto]]

(In reply to Andrea Marchesini [:baku] from comment #7)
> . on linux: setpriority. See https://linux.die.net/man/2/setpriority

You might want to check out cgroups instead, setpriority() has the severe limitation that it doesn't allow to change the priority of an entire process in a non-racy way. Changes applied to the process' pid affect only the first thread of that process (i.e. the one with the tid == pid), other threads are unaffected. So to reschedule an entire process you have to call setpriority() on every single thread which is inevitably racy.

Because of a kernel bug in early Android versions in B2G we were originally forced to use setpriority() which lead to this horrifying, racy contraption to change the priority of a process (bug 861441):

https://hg.mozilla.org/mozilla-central/file/51bf50daff2f/hal/gonk/GonkHal.cpp#l1065

We later switched to cgroups (bug 1081871) and never looked back.

You will most certainly need a different approach since Firefox doesn't have the same amount of control we had on the system in b2g, so I can't guarantee that cgroups are the right solution, but I'm pretty sure that setpriority() isn't the right one.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8873378 [details] [diff] [review]
visibility.patch

I'd rather store only active tabs in the hashtable. That way ComputePriority wouldn't be so slow when there are lots of tabs. It would be just
if (!mTabParentActivity.IsEmpty()) {
  return PROCESS_PRIORITY_FOREGROUND;
}

Though, better to rename to mActiveTabParents or so.

Comment 11

[Andrea Marchesini [:baku]]

Attachment: visibility.patch

Comment 12

[Andrea Marchesini [:baku]]

Attachment: part 2 - enabling

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8873733 [details] [diff] [review]
visibility.patch

># HG changeset patch
># User Andrea Marchesini <[e-mail]>
># Parent  8bb591801f9b3615d27ddfa6eb3788cb1296bc67
>
>diff --git a/dom/ipc/ProcessPriorityManager.cpp b/dom/ipc/ProcessPriorityManager.cpp
>--- a/dom/ipc/ProcessPriorityManager.cpp
>+++ b/dom/ipc/ProcessPriorityManager.cpp
>@@ -21,16 +21,17 @@
> #include "nsIObserverService.h"
> #include "StaticPtr.h"
> #include "nsIMozBrowserFrame.h"
> #include "nsIObserver.h"
> #include "nsITimer.h"
> #include "nsIPropertyBag2.h"
> #include "nsComponentManagerUtils.h"
> #include "nsCRT.h"
>+#include "nsTHashtable.h"
> 
> using namespace mozilla;
> using namespace mozilla::dom;
> using namespace mozilla::hal;
> 
> #ifdef XP_WIN
> #include <process.h>
> #define getpid _getpid
>@@ -144,16 +145,18 @@ public:
>     hal::ProcessPriority aOldPriority);
> 
>   /**
>    * Implements WakeLockObserver, used to monitor wake lock changes in the
>    * main process.
>    */
>   virtual void Notify(const WakeLockInformation& aInfo) override;
> 
>+  void TabActivityChanged(TabParent* aTabParent, bool aIsActive);
>+
>   /**
>    * Call ShutDown before destroying the ProcessPriorityManager because
>    * WakeLockObserver hols a strong reference to it.
>    */
>   void ShutDown();
> 
> private:
>   static bool sPrefsEnabled;
>@@ -262,16 +265,18 @@ public:
>     BACKGROUND_GRACE_PERIOD,
>   };
> 
>   void ScheduleResetPriority(TimeoutPref aTimeoutPref);
>   void ResetPriority();
>   void ResetPriorityNow();
>   void SetPriorityNow(ProcessPriority aPriority);
> 
>+  void TabActivityChanged(TabParent* aTabParent, bool aIsActive);
>+
>   void ShutDown();
> 
> private:
>   static uint32_t sBackgroundPerceivableGracePeriodMS;
>   static uint32_t sBackgroundGracePeriodMS;
> 
>   void FireTestOnlyObserverNotification(
>     const char* aTopic,
>@@ -288,16 +293,19 @@ private:
>   bool mHoldsHighPriorityWakeLock;
> 
>   /**
>    * Used to implement NameWithComma().
>    */
>   nsAutoCString mNameWithComma;
> 
>   nsCOMPtr<nsITimer> mResetPriorityTimer;
>+
>+  // This hashtable contains the list of active TabId for this process.
>+  nsTHashtable<nsUint64HashKey> mActiveTabParents;
> };
> 
> /* static */ bool ProcessPriorityManagerImpl::sInitialized = false;
> /* static */ bool ProcessPriorityManagerImpl::sPrefsEnabled = false;
> /* static */ bool ProcessPriorityManagerImpl::sRemoteTabsDisabled = true;
> /* static */ bool ProcessPriorityManagerImpl::sTestMode = false;
> /* static */ bool ProcessPriorityManagerImpl::sPrefListenersRegistered = false;
> /* static */ StaticRefPtr<ProcessPriorityManagerImpl>
>@@ -533,16 +541,30 @@ ProcessPriorityManagerImpl::Notify(const
>       mHighPriority = false;
>     }
> 
>     LOG("Got wake lock changed event. "
>         "Now mHighPriorityParent = %d\n", mHighPriority);
>   }
> }
> 
>+void
>+ProcessPriorityManagerImpl::TabActivityChanged(TabParent* aTabParent,
>+                                               bool aIsActive)
>+{
>+  ContentParent* cp = aTabParent->Manager()->AsContentParent();
>+  RefPtr<ParticularProcessPriorityManager> pppm =
>+    GetParticularProcessPriorityManager(cp);
>+  if (!pppm) {
>+    return;
>+  }
>+
>+  pppm->TabActivityChanged(aTabParent, aIsActive);
>+}
>+
> NS_IMPL_ISUPPORTS(ParticularProcessPriorityManager,
>                   nsIObserver,
>                   nsITimerCallback,
>                   nsISupportsWeakReference);
> 
> ParticularProcessPriorityManager::ParticularProcessPriorityManager(
>   ContentParent* aContentParent)
>   : mContentParent(aContentParent)
>@@ -719,16 +741,23 @@ ParticularProcessPriorityManager::OnTabP
>   nsCOMPtr<nsITabParent> tp = do_QueryInterface(aSubject);
>   NS_ENSURE_TRUE_VOID(tp);
> 
>   MOZ_ASSERT(XRE_IsParentProcess());
>   if (TabParent::GetFrom(tp)->Manager() != mContentParent) {
>     return;
>   }
> 
>+  uint64_t tabId;
>+  if (NS_WARN_IF(NS_FAILED(tp->GetTabId(&tabId)))) {
>+    return;
>+  }
>+
>+  mActiveTabParents.RemoveEntry(tabId);
>+
>   ResetPriority();
> }
> 
> void
> ParticularProcessPriorityManager::ResetPriority()
> {
>   ProcessPriority processPriority = ComputePriority();
>   if (mPriority == PROCESS_PRIORITY_UNKNOWN ||
>@@ -794,18 +823,19 @@ ProcessPriority
> ParticularProcessPriorityManager::CurrentPriority()
> {
>   return mPriority;
> }
> 
> ProcessPriority
> ParticularProcessPriorityManager::ComputePriority()
> {
>-  // TODO...
>-  return PROCESS_PRIORITY_FOREGROUND;
>+  if (!mActiveTabParents.IsEmpty()) {
>+    return PROCESS_PRIORITY_FOREGROUND;
>+  }
> 
>   if (mHoldsCPUWakeLock || mHoldsHighPriorityWakeLock) {
>     return PROCESS_PRIORITY_BACKGROUND_PERCEIVABLE;
>   }
> 
>   return PROCESS_PRIORITY_BACKGROUND;
> }
> 
>@@ -844,16 +874,31 @@ ParticularProcessPriorityManager::SetPri
>     Unused << mContentParent->SendNotifyProcessPriorityChanged(mPriority);
>   }
> 
>   FireTestOnlyObserverNotification("process-priority-set",
>     ProcessPriorityToString(mPriority));
> }
> 
> void
>+ParticularProcessPriorityManager::TabActivityChanged(TabParent* aTabParent,
>+                                                     bool aIsActive)
>+{
>+  MOZ_ASSERT(aTabParent);
>+
>+  if (!aIsActive) {
>+    mActiveTabParents.RemoveEntry(aTabParent->GetTabId());
>+  } else {
>+    mActiveTabParents.PutEntry(aTabParent->GetTabId());
>+  }
>+
>+  ResetPriority();
>+}
>+
>+void
> ParticularProcessPriorityManager::ShutDown()
> {
>   MOZ_ASSERT(mContentParent);
> 
>   UnregisterWakeLockObserver(this);
> 
>   if (mResetPriorityTimer) {
>     mResetPriorityTimer->Cancel();
>@@ -1018,9 +1063,24 @@ ProcessPriorityManager::SetProcessPriori
> 
> /* static */ bool
> ProcessPriorityManager::CurrentProcessIsForeground()
> {
>   return ProcessPriorityManagerChild::Singleton()->
>     CurrentProcessIsForeground();
> }
> 
>+/* static */ void
>+ProcessPriorityManager::TabActivityChanged(TabParent* aTabParent,
>+                                           bool aIsActive)
>+{
>+  MOZ_ASSERT(aTabParent);
>+
>+  ProcessPriorityManagerImpl* singleton =
>+    ProcessPriorityManagerImpl::GetSingleton();
>+  if (!singleton) {
>+    return;
>+  }
>+
>+  singleton->TabActivityChanged(aTabParent, aIsActive);
>+}
>+
> } // namespace mozilla
>diff --git a/dom/ipc/ProcessPriorityManager.h b/dom/ipc/ProcessPriorityManager.h
>--- a/dom/ipc/ProcessPriorityManager.h
>+++ b/dom/ipc/ProcessPriorityManager.h
>@@ -7,16 +7,17 @@
> #ifndef mozilla_ProcessPriorityManager_h_
> #define mozilla_ProcessPriorityManager_h_
> 
> #include "mozilla/HalTypes.h"
> 
> namespace mozilla {
> namespace dom {
> class ContentParent;
>+class TabParent;
> } // namespace dom
> 
> /**
>  * This class sets the priority of subprocesses in response to explicit
>  * requests and events in the system.
>  *
>  * A process's priority changes e.g. when it goes into the background via
>  * mozbrowser's setVisible(false).  Process priority affects CPU scheduling and
>@@ -63,16 +64,18 @@ public:
>    * Returns true iff this process's priority is FOREGROUND*.
>    *
>    * Note that because process priorities are set in the main process, it's
>    * possible for this method to return a stale value.  So be careful about
>    * what you use this for.
>    */
>   static bool CurrentProcessIsForeground();
> 
>+  static void TabActivityChanged(dom::TabParent* aTabParent, bool aIsActive);
>+
> private:
>   ProcessPriorityManager();
>   DISALLOW_EVIL_CONSTRUCTORS(ProcessPriorityManager);
> };
> 
> } // namespace mozilla
> 
> #endif
>diff --git a/dom/ipc/TabParent.cpp b/dom/ipc/TabParent.cpp
>--- a/dom/ipc/TabParent.cpp
>+++ b/dom/ipc/TabParent.cpp
>@@ -93,16 +93,17 @@
> #include "nsIAuthPrompt2.h"
> #include "gfxDrawable.h"
> #include "ImageOps.h"
> #include "UnitTransforms.h"
> #include <algorithm>
> #include "mozilla/WebBrowserPersistDocumentParent.h"
> #include "nsIGroupedSHistory.h"
> #include "PartialSHistory.h"
>+#include "ProcessPriorityManager.h"
> #include "nsString.h"
> 
> #ifdef XP_WIN
> #include "mozilla/plugins/PluginWidgetParent.h"
> #endif
> 
> #if defined(XP_WIN) && defined(ACCESSIBILITY)
> #include "mozilla/a11y/AccessibleWrap.h"
>@@ -2640,16 +2641,20 @@ TabParent::SetDocShellIsActive(bool isAc
>   // SetDocShellIsActive requests are ignored.
>   mLayerTreeEpoch++;
> 
>   // docshell is consider prerendered only if not active yet
>   mIsPrerendered &= !isActive;
>   mDocShellIsActive = isActive;
>   Unused << SendSetDocShellIsActive(isActive, mPreserveLayers, mLayerTreeEpoch);
> 
>+  // Let's inform the priority manager. This operation can end up with the
>+  // changing of the process priority.
>+  ProcessPriorityManager::TabActivityChanged(this, isActive);
>+
>   // Ask the child to repaint using the PHangMonitor channel/thread (which may
>   // be less congested).
>   if (isActive) {
>     ContentParent* cp = Manager()->AsContentParent();
>     cp->ForceTabPaint(this, mLayerTreeEpoch);
>   }
> 
>   return NS_OK;

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8873826 [details] [diff] [review]
part 2 - enabling

But do we have backend of this ready?

Comment 15

[Andrea Marchesini [:baku]]

I have something read-ish for linux. No mac, no windows. I'll submit a patch for linux probably on Monday.

Comment 16

[Andrea Marchesini [:baku]]

Attachment: part 2 - enabling

I uploaded the wrong patch. This is the right now for enabling this feature.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8874083 [details] [diff] [review]
part 2 - enabling

Why do we want to get rid of the pref?
And if we remove the pref, method name ProcessPriorityManagerImpl::PrefsEnabled()
doesn't make sense.
I think we could keep dom.ipc.processPriorityManager.enabled for now, at least for testing.

then
return hal::SetProcessPrioritySupported() && !sRemoteTabsDisabled;
->
return sPrefsEnabled && hal::SetProcessPrioritySupported() && !sRemoteTabsDisabled;

Comment 18

[Mike Conley (:mconley) (:⚙️)]

Is this patch ready / able to land, or are we waiting until after the soft freeze?

Comment 19

[Andrea Marchesini [:baku]]

These patches are ready to land. But we need the platform-specific part. I wrote the linux one. We need the windows and mac ones.

Comment 20

[(no longer active)]

(In reply to Andrea Marchesini [:baku] from comment #19)
> These patches are ready to land. But we need the platform-specific part. I
> wrote the linux one. We need the windows and mac ones.

I think I or dmajor may be able to help with the Windows parts.  I don't see a Linux specific patch here so I don't know which APIs you need.

For the Mac version you can ask help from mstange or spohl.  But I think this can land without Mac support initially, that's not really important.  Windows is the important platform to support.  We can support OSX opportunistically.

Comment 21

[Andrea Marchesini [:baku]]

Linux patch is still in testing. I want to file a separate bug for it.
About the API we should use, see comments 7 and 9.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8011068db5a
Decrease the process priority of content processes that are not running a foreground tab - part 1 - visibility logic, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/d95970dc9b7d
Decrease the process priority of content processes that are not running a foreground tab - part 2 - enabled if the platform specific code is ready, r=smaug

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a8011068db5a
https://hg.mozilla.org/mozilla-central/rev/d95970dc9b7d