Mac: Proof-of-concept task manager

Mac: Proof-of-concept task manager

Superseded by http://codereview.chromium.org/549002.

I landed the IPC parts of this temporarily in r34146 to look at the perf impact. Results are inconclusive; maybe it makes startup a tiny bit slower, but it's within noise level.

Once this is nice and tidy, I will break this up into smaller CLs, send those out for review and submit this in pieces (by now, this happened for several of the pieces).

Outstanding issues:
* IPC part should be part of the channel infrastructure
* Doesn't work for worker and utility processes yet

* Polish stuff: Maybe sample every 2s to better match Activity Monitor, maybe call this "Activity Monitor" instead of "Task Manager" on OS X, add icons to UI, and other small UI stuff.

Done:
* Support more child process types (currently, this only works for renderers. Missing are plugins, extensions, maybe helper/utility processes, webworkers)
* This currently leaks ports (but not in a harmful way as far as I can tell, and maybe that's not even caused by this patch)

BUG=13156
TEST=
1.) Open one tab that plays a youtube video
2.) Open a second and visit http://www.whatwg.org/demos/workers/primes/page.html
3.) Install e.g. the gmail checker extension
4.) Open the task manager
It should report metrics for
* one browser process
* two renderer processes
* one plugin process
* one worker process
* one extension process
Check that %cpu etc more or less match what Activity Monitor displays if you filter for "Chromium".

TODO: check same for utility processes, nacl, profile importer

Process types:
* Renderer
* Extension
* Utility
* NaCL
* Plugin
* Profile importer
* Shared worker
* (Zygote)

[Nico, 2009-12-09 08:41:41]

Still more a request for comments than a review.

There's a MachBroker class that stores a pid->mach_port_t mapping. The renderer
opens a mach ipc connection on a background thread (renderer_main change) and
sends its task and host, and the browser opens a mach ipc connection on a
background thread to read these. The normal ipc connection name is used for the
mach name as well.

Naming is suboptimal, and I probably want to move my new threads and tasks into
their own files (and I want to use fewer debug printfs, and I might be leaking
mach ports atm), but modulo that, is this ok? Please also look at the perf
graphs.

A slightly different approach would be to send a "please ipc me mach ports"
message to the renderer through normal ipc and then do this dance there.

Jeremy: I wasn't sure what you meant with "integrate with existing channel" on
the last CL, since this needs to open a different channel.

[jeremy, 2009-12-09 10:38:10]

Take a look at how FDs are handled in ipc_channl_posix.cc .  I mean something
like that - add out of bound message passing via Mach ports.

This would provide a more flexible solution in that in the future we could move
other data types over Mach IPC e.g. zero copy memory buffers.  If it makes
sense.

The current solution is a bit more minimal but lacks the flexibility of hooking
into the existing ipc system.

[Nico, 2009-12-09 17:26:16]

On 2009/12/09 10:38:10, jeremy wrote:
> Take a look at how FDs are handled in ipc_channl_posix.cc .  I mean something
> like that - add out of bound message passing via Mach ports.

I looked there last week, but I didn't get it. FDs can be sent over sockets.
Mach ports can't. Do you mean the ipc channel should have both a posix socket
connection and a mach connection? What am I missing?

> 
> This would provide a more flexible solution in that in the future we could
move
> other data types over Mach IPC e.g. zero copy memory buffers.  If it makes
> sense.
> 
> The current solution is a bit more minimal but lacks the flexibility of
hooking
> into the existing ipc system.

[Mark Mentovai, 2009-12-09 17:39:55]

I think there's a disconnect between what Nico's trying to achieve and
what Jeremy thinks is possible.

Nico's change is intended to get us around the "we can't call
task_for_pid" problem.  It's very purpose-specific.

Jeremy's asking for something more general: hooking up a Mach port
between processes as, it seems to me, another communication channel on
the same IPC Channel (capital C, as in ipc_channel_posix.cc).  We'd
then have the ability to pass the mach_port from a child's
task_for_pid across IPC.  We'd also be able to use the IPC Mach port
for other IPC tasks.

I always thought that a Mach-based IPC implementation would be an
interesting way to go, and Jeremy's prototypes did seem to be working
well.  I'm not sure if we'd want to go whole-hog or use some sort of
hybrid model or even if it would ultimately wind up making sense at
all given the way that we use the IPC architecture*.  Regardless,
setting up the infrastructure to use Mach IPC may not be worthwhile
right now unless we're actually planning on using it - if we just need
to solve the task_for_pid problem, I don't think there's anything
tying us to pulling it through our existing IPC mechanism if other
better solutions are available.  (Better doesn't have to mean
"technically superior" or "ultimately more flexible," it can just be
"simpler" and "easier to implement right now.")

Does that sound like a fair characterization?

Now you kids can duke it out.

Mark

* "the way that we use IPC" - mostly very small messages and a lot of
shared memory.  With that in mind, we definitely have seen cases where
we hit "IPC message is too big," and that's pretty much a dead-end.
http://crbug.com/26822 is an example.  Whether or not we choose to
have an upper bound on the size of what can be sent over IPC, if we're
going to have big IPC messages, sending them between processes as
copy-on-write could be a winner.

thakis@chromium.org wrote:
> On 2009/12/09 10:38:10, jeremy wrote:
>>
>> Take a look at how FDs are handled in ipc_channl_posix.cc .  I mean
>> something
>> like that - add out of bound message passing via Mach ports.
>
> I looked there last week, but I didn't get it. FDs can be sent over sockets.
> Mach ports can't. Do you mean the ipc channel should have both a posix
> socket
> connection and a mach connection? What am I missing?
>
>
>> This would provide a more flexible solution in that in the future we could
>
> move
>>
>> other data types over Mach IPC e.g. zero copy memory buffers.  If it makes
>> sense.
>
>> The current solution is a bit more minimal but lacks the flexibility of
>
> hooking
>>
>> into the existing ipc system.
>
>
>
> http://codereview.chromium.org/460126
>

[jam, 2009-12-09 18:40:01]

http://codereview.chromium.org/460126/diff/43/3001
File chrome/browser/child_process_launcher.cc (right):

http://codereview.chromium.org/460126/diff/43/3001#newcode258
chrome/browser/child_process_launcher.cc:258: // (then again, the fds duping
stuff is here too, so maybe it's ok)
this functionality doesn't seem related to ChildProcessLauncher, whose purpose
is to launch a process asynchronously.  It seems that this code better belongs
in the users of this class, when they get OnChannelConnected.

[Mark Mentovai, 2010-01-11 20:22:46]

I think the principles here are sound enough, but it was hardish to follow some
bits with all of the turdlets floating around.  I'll take another look after you
do a cleanup pass.

http://codereview.chromium.org/460126/diff/20024/22001
File chrome/browser/child_process_launcher.cc (right):

http://codereview.chromium.org/460126/diff/20024/22001#newcode41
chrome/browser/child_process_launcher.cc:41: // TODO(thakis): Move some place
central
Wanna do this now?

http://codereview.chromium.org/460126/diff/20024/22001#newcode43
chrome/browser/child_process_launcher.cc:43: std::string channel =
kMachChannelPrefix + channel_name;
Unless channel_name begins with a dot, this will give us names like
"com.Google.ChromeMyNewAwesomeChannelName".

http://codereview.chromium.org/460126/diff/20024/22001#newcode45
chrome/browser/child_process_launcher.cc:45: MachBroker::instance();  // make
sure registrations are registered on the right thread.
80

http://codereview.chromium.org/460126/diff/20024/22001#newcode49
chrome/browser/child_process_launcher.cc:49: printf("Creating receive port
%s\n", channel.c_str());
Debugging turdlet?

(I leave my debugging turdlets flush with the left margin too, to make 'em
easier to spot when I'm done debugging.)

http://codereview.chromium.org/460126/diff/20024/22001#newcode55
chrome/browser/child_process_launcher.cc:55: const int kMachPortMessageID = 57;
Magic?

http://codereview.chromium.org/460126/diff/20024/22001#newcode64
chrome/browser/child_process_launcher.cc:64: CHECK(kMachPortMessageID ==
message.GetMessageID());
CHECK_EQ?

http://codereview.chromium.org/460126/diff/20024/22001#newcode65
chrome/browser/child_process_launcher.cc:65: CHECK(1 ==
message.GetDescriptorCount());
CHECK_EQ?

http://codereview.chromium.org/460126/diff/20024/22001#newcode67
chrome/browser/child_process_launcher.cc:67: printf("yay\n");
Debugging turdlet?

http://codereview.chromium.org/460126/diff/20024/22001#newcode71
chrome/browser/child_process_launcher.cc:71: printf("nay\n");
LOG?

http://codereview.chromium.org/460126/diff/20024/22001#newcode80
chrome/browser/child_process_launcher.cc:80: class MachTask2 : public Task {
Class-level comments would be nice for this and for MachTask, so that a casual
reader can tell the difference between the two classes.

http://codereview.chromium.org/460126/diff/20024/22001#newcode82
chrome/browser/child_process_launcher.cc:82: MachTask2(const mach_port_t& task,
base::ProcessHandle pid)  // must be refs, might be 0 at call time and only be
set later on the other thread
80

http://codereview.chromium.org/460126/diff/20024/22001#newcode86
chrome/browser/child_process_launcher.cc:86: if (task_)
You need {} here because this thing spills over a bunch of lines.

http://codereview.chromium.org/460126/diff/20024/22001#newcode98
chrome/browser/child_process_launcher.cc:98: MachThread() :
base::Thread("MachThread"), task_(0) {}
task_(MACH_PORT_NULL) ?

http://codereview.chromium.org/460126/diff/20024/22001#newcode106
chrome/browser/child_process_launcher.cc:106: void DoIt2(base::ProcessHandle
pid) {
Oh, so is this just to avoid the ban on overloading?

DoItChannelName and DoItPid would be better names, then.

"Do it.  Do it.  DO IT."

http://codereview.chromium.org/460126/diff/20024/22001#newcode255
chrome/browser/child_process_launcher.cc:255: bool is_extension =
cmd_line->GetSwitchValueASCII(switches::kProcessType) ==
Fix indent?

This whole section (253-256) should be broken up somehow to make it a little
more readable.  For example, factor out the GetSwitchValueASCII call?

http://codereview.chromium.org/460126/diff/20024/22001#newcode258
chrome/browser/child_process_launcher.cc:258: for (size_t ii = 0; ii <
cmd_line->argv().size(); ++ii)
Debugging turdlets?

http://codereview.chromium.org/460126/diff/20024/22001#newcode262
chrome/browser/child_process_launcher.cc:262: MachThread mach_thread;
Fix the indent in here too?

http://codereview.chromium.org/460126/diff/20024/22001#newcode263
chrome/browser/child_process_launcher.cc:263: if (true || is_renderer ||
is_extension) {
Remove "true" - debugging turdlet?

http://codereview.chromium.org/460126/diff/20024/22001#newcode276
chrome/browser/child_process_launcher.cc:276: if (true || is_renderer ||
is_extension) {
true

http://codereview.chromium.org/460126/diff/20024/22001#newcode395
chrome/browser/child_process_launcher.cc:395: //  DCHECK(!context_->starting_);
?

http://codereview.chromium.org/460126/diff/20024/22002
File chrome/browser/mach_broker_mac.cc (right):

http://codereview.chromium.org/460126/diff/20024/22002#newcode14
chrome/browser/mach_broker_mac.cc:14: CHECK(NotificationService::current() !=
NULL)
CHECK_NE?

http://codereview.chromium.org/460126/diff/20024/22002#newcode48
chrome/browser/mach_broker_mac.cc:48: fprintf(stderr, "invalidating pid %d\n",
(int)pid);
Turdlet!

http://codereview.chromium.org/460126/diff/20024/22002#newcode58
chrome/browser/mach_broker_mac.cc:58: LOG(WARNING) << "Failed to deallocate mach
task " << it->second.mach_task_
This can be a LOG_IF.

http://codereview.chromium.org/460126/diff/20024/22002#newcode95
chrome/browser/mach_broker_mac.cc:95: Invalidate(handle);
If handle is 0, you know you don't need to waste time calling Invalidate.

http://codereview.chromium.org/460126/diff/20024/22004
File chrome/chrome_common.gypi (right):

http://codereview.chromium.org/460126/diff/20024/22004#newcode59
chrome/chrome_common.gypi:59: 'common/mach_ipc_mac.h',
Sort, mach < main.

http://codereview.chromium.org/460126/diff/20024/22005
File chrome/common/child_process.cc (right):

http://codereview.chromium.org/460126/diff/20024/22005#newcode31
chrome/common/child_process.cc:31: const int kMachPortMessageID = 57;
This line and the next are “duplicatious.”  I know English isn’t your first
language, but don’t bother looking that up, I invented it myself.

Heed the good advice of your TODO from line 30.

http://codereview.chromium.org/460126/diff/20024/22005#newcode32
chrome/common/child_process.cc:32: const std::string kMachChannelPrefix =
"com.Google.Chrome";
Same thing here applies about the dots.

http://codereview.chromium.org/460126/diff/20024/22005#newcode36
chrome/common/child_process.cc:36: printf("Creating send port %s\n",
channel_name.c_str());
t’let

http://codereview.chromium.org/460126/diff/20024/22005#newcode40
chrome/common/child_process.cc:40: // add some ports to be translated for us
Comments ought to be complete sentences with capital letters and periods and
occasionally humor.

http://codereview.chromium.org/460126/diff/20024/22005#newcode41
chrome/common/child_process.cc:41: //   
message.AddDescriptor(MachMsgPortDescriptor(mach_task_self()));
Huh?

http://codereview.chromium.org/460126/diff/20024/22005#newcode42
chrome/common/child_process.cc:42:
message.AddDescriptor(MachMsgPortDescriptor(mach_task_self(),
MACH_MSG_TYPE_COPY_SEND));
80

http://codereview.chromium.org/460126/diff/20024/22005#newcode49
chrome/common/child_process.cc:49: fprintf(stderr, "send result: 0x%lx\n",
(unsigned long)result);
Observe the TODO from 47, and don't printf in any case.  :)

http://codereview.chromium.org/460126/diff/20024/22006
File chrome/common/mach_ipc_mac.h (right):

http://codereview.chromium.org/460126/diff/20024/22006#newcode89
chrome/common/mach_ipc_mac.h:89: explicit MachMsgPortDescriptor(mach_port_t
in_name) {
Good catch.

http://codereview.chromium.org/460126/diff/20024/22007
File chrome/common/mach_ipc_mac.mm (right):

http://codereview.chromium.org/460126/diff/20024/22007#newcode189
chrome/common/mach_ipc_mac.mm:189: fprintf(stderr, "failed to allocate send
port\n");
What's this decoration about?

http://codereview.chromium.org/460126/diff/20024/22007#newcode199
chrome/common/mach_ipc_mac.mm:199: fprintf(stderr, "failed to insert send
right\n");
Same

http://codereview.chromium.org/460126/diff/20024/22007#newcode205
chrome/common/mach_ipc_mac.mm:205: if (![[NSMachBootstrapServer sharedInstance]
registerPort:ns_port name:port_name]) {
80

http://codereview.chromium.org/460126/diff/20024/22007#newcode206
chrome/common/mach_ipc_mac.mm:206: NSLog(@"failed to register ort %@ with name
%@", ns_port, port_name);
NSLog?

And you misspelled port.  :)

http://codereview.chromium.org/460126/diff/20024/22007#newcode220
chrome/common/mach_ipc_mac.mm:220: fprintf(stderr, "failed to allocate send
port2\n");
t’let

http://codereview.chromium.org/460126/diff/20024/22007#newcode253
chrome/common/mach_ipc_mac.mm:253: fprintf(stderr, "failed to insert send
right2\n");
…

http://codereview.chromium.org/460126/diff/20024/22007#newcode283
chrome/common/mach_ipc_mac.mm:283: fprintf(stderr, "receiving bootstrap port
failed\n");
…

http://codereview.chromium.org/460126/diff/20024/22007#newcode307
chrome/common/mach_ipc_mac.mm:307: fprintf(stderr, "bootstrap lookup failed\n");
…

[Mark Mentovai, 2010-01-11 20:23:32]

If the turdlets are intentional and you want to keep them until you're done
refactoring, that's OK too.  I'll wait to take another look until you've moved
the pieces around.

[Mark Mentovai, 2010-01-11 20:24:52]

codereview screwed me.