Changing priority on Window processes

RESOLVED FIXED in Firefox 63

Status

()

P3
normal
RESOLVED FIXED
a year ago
12 days ago

People

(Reporter: baku, Assigned: dthayer)

Tracking

(Depends on: 1 bug, Blocks: 4 bugs)

unspecified
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

a year ago
+++ 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.
(Reporter)

Updated

a year ago
Blocks: 1394714

Updated

a year ago
Priority: -- → P3

Updated

7 months ago
Blocks: 1366358
(Assignee)

Comment 1

5 months ago
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
Created attachment 8984601 [details]
Bug 1394710 - Set process priority class on Windows r?jmathies

Fairly straightforward. Using an idle priority class for
background, which mirrors what I am observing of Chromium in
Process Explorer.
(Assignee)

Comment 3

4 months ago
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)
Comment hidden (mozreview-request)

Comment 5

3 months ago
Out of curiosity, are there any plans for followup work to use PROCESS_MODE_BACKGROUND_BEGIN/END?
(Assignee)

Comment 6

3 months ago
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

Comment 7

3 months ago
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.

Comment 8

3 months ago
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)
(Assignee)

Updated

3 months ago
Blocks: 1476365
(Assignee)

Comment 9

3 months ago
(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 11

3 months ago
mozreview-review
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+

Comment 12

3 months ago
Pushed by dothayer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/73ed61535854
Set process priority class on Windows r=aklotz
(Assignee)

Comment 13

3 months ago
Arg. Embarrassing. Brain farted and didn't push to review before autolanding.
Comment hidden (mozreview-request)

Comment 16

3 months ago
Pushed by dothayer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4e0b454aac6e
Set process priority class on Windows r=aklotz
(Assignee)

Updated

3 months ago
Flags: needinfo?(dothayer)

Comment 17

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4e0b454aac6e
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
status-firefox63: --- → fixed
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)
(Assignee)

Updated

3 months ago
Blocks: 1476981
(Assignee)

Comment 19

3 months ago
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!

Updated

3 months ago
Attachment #8984601 - Attachment is obsolete: true

Updated

12 days ago
Duplicate of this bug: 1476365
You need to log in before you can comment on or make changes to this bug.