1652000 - about:processes uses too much CPU

Status: RESOLVED FIXED

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

Description

[David Teller [:Yoric] - still alive but not very active]

Apparently, about:processes takes 25-30% on some machines. Let's fix that.

Ryan, could you tell us more? Which OS are you using?

Comment 1

[Ryan VanderMeulen [:RyanVM]]

Attachment: about:support

Comment 2

[Florian Quèze [:florian]]

Here's a profile of it: https://share.firefox.dev/2W5mXWw

Comment 3

[David Teller [:Yoric] - still alive but not very active]

Thanks.

So, it looks like calls to Thread32Next are expensive and we're calling it quite often. Looking at it a bit more in detail, we're doing the following:

• For each process
  • Take a snapshot of the entire OS
  • Walk through all the threads of this snapshot
    • Discard the threads that do not match the current process

Looks like we could be quite a bit more efficient by taking a single snapshot and iterating through it once. This would mean reworking the calling code in ChromeUtils to avoid assuming that we handle each process separately.

I might also move this code to ProcessToolsService, to be determined.

Comment 4

[David Teller [:Yoric] - still alive but not very active]

So, I'd say:

1. Rework ProcInfo::GetProcInfo to take as argument an array of partially-filled ProcInfo instead of a single ProcInfo.
2. Rework the calling code from ChromeUtils::RequestProcInfo accordingly.

Comment 5

[David Teller [:Yoric] - still alive but not very active]

Attachment: Bug 1652000 - Refactoring ChromeUtils::RequestProcInfo to place a single request to GetProcInfo;r?tarek!

The current implementation of GetProcInfo is inefficient under Windows as it takes
one snapshot of the entire OS per process, then needs to walk each of these global
snapshots looking for information on the single process.

This patch changes the API and client caller RequestProcInfo to be able to share
a single snapshot.

Comment 6

[David Teller [:Yoric] - still alive but not very active]

Attachment: Bug 1652000 - Refactoring ChromeUtils::RequestProcInfo to place a single request to GetProcInfo;r?tarek

GetProcInfo relies upon Thread32First/Thread32Next, both of which are quite CPU-expensive.
This patch refactors GetProcInfo to decrease the number of calls to Thread32Next from
~O(T * P) to ~O(T), where T is the number of threads on the OS and P is the number of Firefox
processes launched.

Depends on D83335

Comment 7

[David Teller [:Yoric] - still alive but not very active]

Attachment: Bug 1652000 - A little lifetime documentation;r?nika!

Comment 8

[David Teller [:Yoric] - still alive but not very active]

Attachment: Bug 1652000 - Refactor Linux implementation of GetProcInfo to use a single snapshot;r?tarek

Depends on D83336

Comment 9

[David Teller [:Yoric] - still alive but not very active]

Attachment: Bug 1652000 - Refactor macOS implementation of GetProcInfo to use a single snapshot;r?tarek!

Depends on D83371

Comment 10

[David Teller [:Yoric] - still alive but not very active]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=829d1a546cd375411367aeab2f5c6764bcd710a5

Comment 11

[David Teller [:Yoric] - still alive but not very active]

Florian, Ryan, once this patch has landed, could you confirm that it improves CPU usage on your Windows machines?

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/22bb2a5032b7
Refactoring ChromeUtils::RequestProcInfo to place a single request to GetProcInfo;r=tarek
https://hg.mozilla.org/integration/autoland/rev/5f02a357671c
A little lifetime documentation;r=nika

Comment 13

[Narcis Beleuzu [:NarcisB]]

Backed out for bustages on ChromeUtils.cpp

Backout link: https://hg.mozilla.org/integration/autoland/rev/bc330da2b11b420c6907b136db6b7342bc50ea3e
Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=310189138&repo=autoland&lineNumber=44969

Comment 14

[(No longer employed by Mozilla) Aaron Klotz]

I hope we're taking the toolhelp32 snapshot off main thread as well; that call is super expensive.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0a6cb920a247
Refactoring ChromeUtils::RequestProcInfo to place a single request to GetProcInfo;r=tarek,geckoview-reviewers,agi
https://hg.mozilla.org/integration/autoland/rev/69dbf38f4071
A little lifetime documentation;r=nika

Comment 16

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/0a6cb920a247
https://hg.mozilla.org/mozilla-central/rev/69dbf38f4071

Comment 17

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fd92c0395206
Backed out 2 changesets for causing Bug 1653749.

Comment 18

[Cosmin Sabou [:CosminS]]

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&searchStr=os%2Cx%2C10.14%2Cdebug%2Cmochitests%2Ctest-macosx1014-64%2Fdebug-mochitest-browser-chrome-e10s%2Cbc13&revision=69dbf38f40712351b668a82e7880649e6d89fd26&selectedTaskRun=GcODwbCNTuiaxPpDI48vJg.0

Failure logs: https://treeherder.mozilla.org/logviewer.html#?job_id=310279082&repo=autoland
https://treeherder.mozilla.org/logviewer.html#?job_id=310279090&repo=autoland
https://treeherder.mozilla.org/logviewer.html#?job_id=310279093&repo=autoland

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

Comment 19

[David Teller [:Yoric] - still alive but not very active]

Thanks for the backout!

Comment 20

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backout merged to central: https://hg.mozilla.org/mozilla-central/rev/fd92c0395206

Comment 21

[David Teller [:Yoric] - still alive but not very active]

So, tests confirm that if I remove the 3-liner that was supposed to fix bug 1642257, we don't crash anymore.

Let's land the part that works and deal with the other bug in the other bug.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d52e4d4a3788
Refactoring ChromeUtils::RequestProcInfo to place a single request to GetProcInfo;r=tarek,geckoview-reviewers,agi
https://hg.mozilla.org/integration/autoland/rev/4603073c15cb
A little lifetime documentation;r=nika

Comment 23

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/d52e4d4a3788
https://hg.mozilla.org/mozilla-central/rev/4603073c15cb

Comment 24

[David Teller [:Yoric] - still alive but not very active]

So we had a tid of 0 under Windows once.

Odd.

Comment 25

[Florian Quèze [:florian]]

(In reply to David Teller [:Yoric] (please use "needinfo") from comment #11)

Florian, Ryan, once this patch has landed, could you confirm that it improves CPU usage on your Windows machines?

CPU usage now seems to be 2-4% of one core. Example profile: https://share.firefox.dev/2ZScj7G

Comment 26

[Ryan VanderMeulen [:RyanVM]]

CPU usage seems better for me too.

Comment 27

[David Teller [:Yoric] - still alive but not very active]

We'll call this a victory, then!