Closed Bug 1444765 Opened 2 years ago Closed Last year

Add ability to quickly profile all threads for a given PID

Categories

(Core :: Gecko Profiler, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: kats, Assigned: kats)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.
Priority: -- → P3
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
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.
Assignee: nobody → bugmail
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 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?
Attachment #8984275 - Flags: feedback?(mstange)
(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 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
Attachment #8984275 - Flags: review?(n.nethercote) → review+
Thanks! I updated the comment on profiler_start like you suggested.
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/035cac21d2b1
Allow setting a pid:<pid> thread filter to capture all threads for a process. r=njn
https://hg.mozilla.org/mozilla-central/rev/035cac21d2b1
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.