1394710 - Changing priority on Window processes

Status: RESOLVED FIXED

(Core :: IPC, enhancement, P3)

Description

[Andrea Marchesini [:baku]]

+++ 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.

Comment 1

[Alex Thayer [:alexical] (she/her)]

I'd like to take a look at this and see if it has any impact on tab switch timings (among other things).

Comment 2

[Phabricator Automation]

Attachment: 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.

Comment 3

[Alex Thayer [:alexical] (she/her)]

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.)

Comment 4

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 1394710 - Set process priority class on Windows

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

Review commit: https://reviewboard.mozilla.org/r/257246/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/257246/

Comment 5

[(Away)]

Out of curiosity, are there any plans for followup work to use PROCESS_MODE_BACKGROUND_BEGIN/END?

Comment 6

[Alex Thayer [:alexical] (she/her)]

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

[(Away)]

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

[(No longer employed by Mozilla) Aaron Klotz]

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?

Comment 9

[Alex Thayer [:alexical] (she/her)]

(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.

Comment 10

[(No longer employed by Mozilla) Aaron Klotz]

Thanks.

Comment 11

[(No longer employed by Mozilla) Aaron Klotz]

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.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/73ed61535854
Set process priority class on Windows r=aklotz

Comment 13

[Alex Thayer [:alexical] (she/her)]

Arg. Embarrassing. Brain farted and didn't push to review before autolanding.

Comment 14

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8992385 [details]
Bug 1394710 - Set process priority class on Windows

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257246/diff/1-2/

Comment 15

[Cosmin Sabou [:CosminS]]

Backed out for braking Gecko Decision Task.

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=success&filter-searchStr=Gecko%20Decision%20Task%20opt%20Gecko%20Decision%20Task%20(D)&fromchange=73ed615358542b987c2c2a8d0730adf0b915a0f6&tochange=be3f6d08fb1715837b86d49c82efe6f776b3247a&selectedJob=188814248

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=188814248&repo=autoland&lineNumber=2267

Backout link: https://hg.mozilla.org/integration/autoland/rev/be3f6d08fb1715837b86d49c82efe6f776b3247a

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4e0b454aac6e
Set process priority class on Windows r=aklotz

Comment 17

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/4e0b454aac6e

Comment 18

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

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?

Comment 19

[Alex Thayer [:alexical] (she/her)]

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.

Comment 20

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

Ah, excellent, thanks!