Changing priority on Window processes

RESOLVED FIXED in Firefox 63

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
9 months ago

People

(Reporter: baku, Assigned: dthayer)

Tracking

(Depends on 1 bug, Blocks 3 bugs)

unspecified
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

+++ This bug was initially created as a clone of Bug #1372543 +++

Bug 1366356 implements the support for changing the process priority but it doesn't include any platform-specific implementation. Here, the windows one.
Blocks: 1394714
Priority: -- → P3
Blocks: 1366358
I'd like to take a look at this and see if it has any impact on tab switch timings (among other things).
Assignee: nobody → dothayer
Status: NEW → ASSIGNED
Fairly straightforward. Using an idle priority class for
background, which mirrors what I am observing of Chromium in
Process Explorer.
Hey Jim, just realized (this was my first and so far only use of phabricator) that this doesn't set r? flags on you. Have you seen this review request yet? Would you prefer it if I sent it through mozreview?

(Or let me know if you'd just prefer someone else to review this and I can find someone.)
Flags: needinfo?(jmathies)
Out of curiosity, are there any plans for followup work to use PROCESS_MODE_BACKGROUND_BEGIN/END?
I'm not aware of any - but in order to do that we'd need to send a message to the process in question to update its priority class itself, since we can't set those for a process other than our own. Also, the MSDN documentation is rather sparse on the details of PROCESS_MODE_BACKGROUND_BEGIN. Do you happen to have a better understanding of it - the chromium source[1] has this comment in the code talking about it, noting that it reduces the priority of IO:

  // Vista and above introduce a real background mode, which not only
  // sets the priority class on the threads but also on the IO generated
  // by it. Unfortunately it can only be set for the calling process.

And this SO question[2] suggests that it limits the process's working set to 32MB:

[1]: https://cs.chromium.org/chromium/src/base/process/process_win.cc?type=cs&q=PROCESS_MODE_BACKGROUND_BEGIN&sq=package:chromium&g=0&l=206
[2]: https://stackoverflow.com/a/30509372
I don't have any experience with it myself, I just noticed the caution on MSDN that lowering CPU priority alone may be ineffective if I/O and network traffic are still happening at standard priority.
I am not opposed to reviewing the patch as-is, but I would like to see a follow-up bug and a commitment to investigate the implications of PROCESS_MODE_BACKGROUND_BEGIN/END.

I don't want it to turn in to one of these, "Yeah, that'd be nice" bugs that languish in obscurity for all eternity.

Doug, do you think you can get a bug filed and prioritized for such an investigation?
Flags: needinfo?(dothayer)
Blocks: 1476365
(In reply to Aaron Klotz [:aklotz] from comment #8)
> I am not opposed to reviewing the patch as-is, but I would like to see a
> follow-up bug and a commitment to investigate the implications of
> PROCESS_MODE_BACKGROUND_BEGIN/END.
> 
> I don't want it to turn in to one of these, "Yeah, that'd be nice" bugs that
> languish in obscurity for all eternity.
> 
> Doug, do you think you can get a bug filed and prioritized for such an
> investigation?

Created, and flagged for fxperf triage.
Flags: needinfo?(dothayer)
Thanks.
Flags: needinfo?(jmathies)
Comment on attachment 8992385 [details]
Bug 1394710 - Set process priority class on Windows

https://reviewboard.mozilla.org/r/257246/#review264446

r=me with issues resolved.

::: hal/windows/WindowsProcessPriority.cpp:27
(Diff revision 1)
> +SetProcessPriority(int aPid, ProcessPriority aPriority)
> +{
> +  HAL_LOG("WindowsProcessPriority - SetProcessPriority(%d, %s)\n",
> +          aPid, ProcessPriorityToString(aPriority));
> +
> +  HANDLE hProcess = ::OpenProcess(PROCESS_SET_INFORMATION, false, aPid);

How about we use nsAutoHandle here, so we don't need to explicitly CloseHandle?

::: hal/windows/WindowsProcessPriority.cpp:27
(Diff revision 1)
> +SetProcessPriority(int aPid, ProcessPriority aPriority)
> +{
> +  HAL_LOG("WindowsProcessPriority - SetProcessPriority(%d, %s)\n",
> +          aPid, ProcessPriorityToString(aPriority));
> +
> +  HANDLE hProcess = ::OpenProcess(PROCESS_SET_INFORMATION, false, aPid);

s/false/FALSE/

::: hal/windows/WindowsProcessPriority.cpp:28
(Diff revision 1)
> +{
> +  HAL_LOG("WindowsProcessPriority - SetProcessPriority(%d, %s)\n",
> +          aPid, ProcessPriorityToString(aPriority));
> +
> +  HANDLE hProcess = ::OpenProcess(PROCESS_SET_INFORMATION, false, aPid);
> +  if (hProcess) {

Please MOZ_ASSERT this as well.
Attachment #8992385 - Flags: review?(aklotz) → review+
Pushed by dothayer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/73ed61535854
Set process priority class on Windows r=aklotz
Arg. Embarrassing. Brain farted and didn't push to review before autolanding.
Pushed by dothayer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4e0b454aac6e
Set process priority class on Windows r=aklotz
Flags: needinfo?(dothayer)
https://hg.mozilla.org/mozilla-central/rev/4e0b454aac6e
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Hey dthayer, is there a way we can make sure this stays on Nightly until we're sure it's able to ride the trains to release? Or is that already covered somehow?
Flags: needinfo?(dothayer)
Blocks: 1476981
That's already covered by the pref "dom.ipc.processPriorityManager.enabled", which is still disabled in Nightly. My plan is to write a patch turning it on in Nightly, and ask baku if he thinks the patch from bug 1366356 is ready for Nightly. This is being tracked in bug 1476981.
Flags: needinfo?(dothayer)
Ah, excellent, thanks!
Attachment #8984601 - Attachment is obsolete: true
Duplicate of this bug: 1476365
You need to log in before you can comment on or make changes to this bug.