1710473 - Make it easier to test the process priority manager

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, task, P2)

Description

[Andrew McCreight [:mccr8]]

The process priority manager is not enabled on Linux or OSX, because the OS integration work hasn't been done there. However, for testing purposes, there's no actual reason to restrict it, because all of the tracking of which processes are active is OS agnostic. I did all of the development of my patch for bug 1618547 on OSX, for instance.

To get the priority manager to work, you have to set dom.ipc.processPriorityManager.enabled to true, which seems reasonable, but that is not sufficient.

The reason for that is that this is the actual code to decide if the priority manager is enabled or not:

bool ProcessPriorityManagerImpl::PrefsEnabled() {
  return StaticPrefs::dom_ipc_processPriorityManager_enabled() &&
         hal::SetProcessPrioritySupported() &&
         !StaticPrefs::dom_ipc_tabs_disabled();
}

You'll note that this will return false if hal::SetProcessPrioritySupported() returns false. On Android and Windows, that function always returns true, but otherwise it always returns false.

For my local debugging, I changed the FallbackProcessPriority's version of this method to always return true, but I think we should just delete this method. It feels like the worst that will happen is that somebody will flip the pref and then nothing will happen, which doesn't seem like a big deal to me.

Maybe I'm missing something. I looked over bug 1366356 where it was added, but I'm not sure why it was added.

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: Bug 1710473 - Remove hal::SetProcessPrioritySupported().

As far as I can see, all this does is protect the user from
running some useless code if they manually enable the priority
manager using a pref on an OS that doesn't support it. The
upside of allowing this is that it makes it possible to debug
the priority manager on OSX and Linux with just a pref flip.

Comment 2

[Gabriele Svelto [:gsvelto]]

It's fine, there's no value in having that redundant check. I don't remember why it was put in place, it was a long time ago, so there might have been a reason but there's a good chance that was FxOS-specific so we shouldn't worry about it.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8c25a9e850c1
Remove hal::SetProcessPrioritySupported(). r=gsvelto,geckoview-reviewers,aklotz

Comment 4

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/8c25a9e850c1