Closed Bug 1529023 Opened 9 months ago Closed 5 months ago

Enable threads on macOS for GetProcInfo

Categories

(Toolkit :: Performance Monitoring, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla69
Tracking Status
firefox69 --- fixed

People

(Reporter: tarek, Assigned: tarek)

References

Details

Attachments

(2 files)

In order to do this we need to perform the GetProcInfo() calls via IPDL on the process itself because the Gnu Mach API task_info() can't be called outside the process unless you are root

Status: NEW → ASSIGNED

In order to solve this, I am thinking about making

https://searchfox.org/mozilla-central/source/toolkit/components/perfmonitoring/PerformanceMetricsCollector.h#87

reusable with ProcInfo items instead of PerformancInfo items, and also provide an customizable iterator for the processes
instead of one that just look at Content processes, https://searchfox.org/mozilla-central/source/toolkit/components/perfmonitoring/PerformanceMetricsCollector.cpp#221

Ehsan or Jed, do you think this should be done as template classes? Also, where do you think this piece of code should be ?
It seems to be an ipc broadcast mechanism that could be reusable elswhere. dom/ipc ?

the pattern is:

  • ask all processes something, under a specific uuid
  • collect all results for that uuid in a singleton (with an IPC timeout to deal with unresponsive processes)
  • resolve a promise with the aggregated results

(basically, Chromeutils.RequestPerformanceMetrics() pattern, but for any ipdl call that is broadcasted)

Flags: needinfo?(jld)
Flags: needinfo?(ehsan)

(In reply to Tarek Ziadé (:tarek) from comment #1)

the pattern is:

  • ask all processes something, under a specific uuid
  • collect all results for that uuid in a singleton (with an IPC timeout to deal with unresponsive processes)
  • resolve a promise with the aggregated results

(basically, Chromeutils.RequestPerformanceMetrics() pattern, but for any ipdl call that is broadcasted)

The memory reporter also has this general shape: send a request to every process and collect the results, with timeouts, and an identifier to distinguish requests. (It also has, or at least used to have when I was working on it, a limit on the number of processes with outstanding requests at once; that was important on B2G but might not matter in this case.) The profiler is also roughly similar, but has its own top-level protocol and dedicated threads so it can work even if the main thread is stuck or deadlocked.

Anyway, this is a harder problem than it seems. For all of these things that every (or almost every) child process uses, every process type has to declare the IPDL methods and implement them separately; I think we've spent several hours by now in IPC status meetings just discussing how to design a solution to this, and we still don't have a good answer. (Note that PGPU and PRDD have the parent/child roles reversed, so we can't even factor everything out into a common managed actor.) There's also the problem of iterating all of the relevant actor objects to send the messages, given that they're all distinct classes and have nothing in common besides IToplevelProtocol (which is subclassed by every actor, parent or child, that's at the root of the tree associated with any channel, not just the process's main actor).

Something that might help here is bug 1512990: the process actors (PContent, etc.) are all devirtualized, so the concrete classes can inherit implementations for Recv methods. If all of the process “parent” actors (ContentParent, GPUChild, etc.) had a common superclass (common to just them, not more general like IToplevelProtocol), then that class could maintain a global list of instances… but it would still need virtual method boilerplate to call the concrete classes' Send methods, unless that were handled by another level of superclass templated on the concrete class type to be able to static_cast itself and call the methods. Something along those lines.

(That still doesn't help with IPDL declarations; there are some ideas about letting protocols inherit/include other protocols, but nothing concrete. But copying/pasting those isn't the worst part of this.)

As for file locations, generally tools that apply to all of IPC and not just content processes go into ipc/glue (which should probably be renamed at some point, because it's no longer only glue code).

Flags: needinfo?(jld)
Flags: needinfo?(ehsan)
Priority: -- → P2

I worked on the patch and realized that even if we're doing the task_for_pid() call inside each child process, we get a permission error because of the way macOs's Mach framework deal with task_for_pid permissions.

The solution is to pass the Mach port via IPC as described in http://www.foldr.org/~michaelw/log/computers/macosx/task-info-fun-with-mach

I wrote a sample program using this explanation, that works, and would allow us to make this work without having to introduce a complex IPDL thing that would need ad-hoc implementation in each process type in Firefox.

The program I am attaching here as an example, gets a task port from the child process and allows us to inspect the child process threads. This technique is used for instance in Chromium where they have a similar feature to track child processes activity.

For us to have this feature working in Firefox, we need to do this IPC exchange everytime a child is forked from the parent process, and keep the child task in the parent process.

I can go ahead and work on this patch if people are OK introducing this new IPC exchange in Firefox (I just need to figure out where the child task object needs to live in the parent process)

Adding a few people in :ni to get some feedback on this :)

Flags: needinfo?(nfroyd)
Flags: needinfo?(mstange)
Flags: needinfo?(ehsan)

Looks like we're already getting the child_task here:

https://searchfox.org/mozilla-central/source/ipc/glue/GeckoChildProcessHost.cpp#974

I'll digg into ipc/glue/SharedMemoryBasic_mach.mm to see if there's a way from that API to get the a child_task, given a pid.

GeckoChildProcessHost::GetChildTask should be helpful here; it's what I used for my rough draft of bug 1480205.

To be completely honest I know next to nothing about the OSX mach APIs involved here, so I will leave this to others to comment on. :-)

Flags: needinfo?(ehsan)

Thanks Jed, this was very helpful!

It's now working with Content processes, I'll update the patch and look at other process types (RDD, GPU, etc.)
If you don't mind I will add you as a reviewer :)

Flags: needinfo?(nfroyd)
Flags: needinfo?(mstange)
Attachment #9055456 - Attachment description: Bug 1529023 - Enable threads on macOS for GetProcInfo → Bug 1529023 - Enable threads on macOS for GetProcInfo r?jld
Attachment #9055456 - Attachment description: Bug 1529023 - Enable threads on macOS for GetProcInfo r?jld → Bug 1529023 - Enable threads on macOS for GetProcInfo r?jld,mstange

(In reply to Tarek Ziadé (:tarek) from comment #5)

I worked on the patch and realized that even if we're doing the task_for_pid() call inside each child process, we get a permission error because of the way macOs's Mach framework deal with task_for_pid permissions.

I'm a bit confused by this statement - in the child process, you wouldn't need to call task_for_pid() at all, you'd just call mach_task_self(), no? But anyway, the alternative solution is good.

The solution is to pass the Mach port via IPC as described in http://www.foldr.org/~michaelw/log/computers/macosx/task-info-fun-with-mach

This is a great alternative because it doesn't require any new IPDL or new threads.

Pushed by tziade@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0635eeb2fad4
Enable threads on macOS for GetProcInfo r=jld,mstange
Flags: needinfo?(tarek)
Pushed by tziade@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5a608649393b
Enable threads on macOS for GetProcInfo r=jld,mstange
Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69
You need to log in before you can comment on or make changes to this bug.