1444765 - Add ability to quickly profile all threads for a given PID

Status: RESOLVED FIXED

(Core :: Gecko Profiler, enhancement, P3)

Description

[Kartikaya Gupta (email:[e-mail])]

Sometimes the browser slows to a crawl with high CPU usage and the OS task manager (e.g. Activity Monitor on macOS) can tell you which process is misbehaving. But running the profiler reveals that the main thread is mostly idle, some other thread must be eating the CPU. But finding that thread is hard. It might be easier if the profiler had an option to profile all the threads for the guilty pid, or at least all the ones that it knows about.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

This would help with e.g. bug 1404042 comment 157.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

For the record, right now if you leave the threads filter empty it will record all the threads across all the processes. Which is useful too. But I'd like to be able to enter something like "pid:<x>" into the thread filter field and have it record all the threads from that pid. From glancing around the code it looks like we would want to add the pid to the ThreadInfo structure being constructed at [1] and modify the ThreadSelected call at [2] to also pass the pid, and modify the implementation of that function to check for "pid:<x>" as one of the filters in mFilters and match against the pid instead. This shouldn't require any changes to the addon or perf-html.io frontend as far as I can tell.

[1] https://searchfox.org/mozilla-central/rev/cf464eabfeba64e866c1fa36b9fefd674dca9c51/tools/profiler/core/platform.cpp#2324
[2] https://searchfox.org/mozilla-central/rev/cf464eabfeba64e866c1fa36b9fefd674dca9c51/tools/profiler/core/platform.cpp#495

Comment 3

[Kartikaya Gupta (email:[e-mail])]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a377bc981865274f67d3eed72ffb010c0e76d80b

The patch was actually even simpler than I thought, since the ThreadSelected check can just pull the pid directly. Try push to make sure it compiles everywhere. I tested locally on macOS and it seems to have as desired.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Looks like there's infra issues ongoing, so not sure if/when the try jobs will be done. I'll put the patch up for review anyway for now. If there's compilation errors they should be easy enough to fix.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1444765 - Allow setting a pid:<pid> thread filter to capture all threads for a process.

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

Comment 6

[Nicholas Nethercote [inactive]]

Comment on attachment 8984275 [details]
Bug 1444765 - Allow setting a pid:<pid> thread filter to capture all threads for a process.

https://reviewboard.mozilla.org/r/250076/#review256456

Seems ok, though it should be documented somewhere, maybe in the comment on `profiler_start` in GeckoProfiler.h.

mstange, I seem to recall that you could put something like ',' into the filters in the profiler UI and it would match all threads, is that right? Maybe we should do something with that?

Comment 7

[Kartikaya Gupta (email:[e-mail])]

(In reply to Nicholas Nethercote [:njn] from comment #6)
> mstange, I seem to recall that you could put something like ',' into the
> filters in the profiler UI and it would match all threads, is that right?

If you leave the filter field empty it will match all the threads, but across all pids. (see comment 2)

Comment 8

[Nicholas Nethercote [inactive]]

Comment on attachment 8984275 [details]
Bug 1444765 - Allow setting a pid:<pid> thread filter to capture all threads for a process.

https://reviewboard.mozilla.org/r/250076/#review257232

Comment 9

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8984275 [details]
Bug 1444765 - Allow setting a pid:<pid> thread filter to capture all threads for a process.

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

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Thanks! I updated the comment on profiler_start like you suggested.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/035cac21d2b1
Allow setting a pid:<pid> thread filter to capture all threads for a process. r=njn

Comment 12

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/035cac21d2b1