Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(11)

Issue 549002: Mac: Other approach for IPCing child task_ts. (Closed)

Created:
9 years, 4 months ago by Nico
Modified:
8 years ago
Reviewers:
Mark Mentovai, jeremy
CC:
chromium-reviews_googlegroups.com
Visibility:
Public.

Description

Mac: Other approach for IPCing child task_ts. Also move mach_ipc_mac to base, where it's now used. Based on http://www.foldr.org/~michaelw/log/2009/03/13/ , but uses a named connection instead. Do the IPC right after fork-time, so that the sandbox is not yet in effect. See the codereview comments for a benchmark that proves that this shouldn't be expensive, and for pros and cons for using a named connection vs temporarily switching out the bootstrap port. Works for worker processes too and seems more reliable in general. Measured perf impact in http://src.chromium.org/viewvc/chrome?view=rev&revision=35888 , it's negligible. BUG=13156 TEST=(requires that one enables the task manager in browser.cc) 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". Also choose "Open all bookmarks" on the bookmarks bar with the task manager open and check that metrics for all tabs appear immediately. Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=35977

Patch Set 1 #

Patch Set 2 : rebase #

Patch Set 3 : compile #

Patch Set 4 : why must everything always suck so much #

Patch Set 5 : '' #

Patch Set 6 : '' #

Patch Set 7 : FFFFFFFFFFFFFFFFFFFUUUUUUUUUUUUUUUUUUUUUU #

Patch Set 8 : '' #

Patch Set 9 : minor cleanup 1 #

Patch Set 10 : mutex #

Patch Set 11 : ready for review #

Patch Set 12 : deallocate ports #

Patch Set 13 : '' #

Patch Set 14 : rebase #

Patch Set 15 : compile linux #

Patch Set 16 : better error logging #

Patch Set 17 : '' #

Patch Set 18 : '' #

Total comments: 1

Patch Set 19 : name-based #

Patch Set 20 : playing with it #

Total comments: 38

Patch Set 21 : comments #

Total comments: 4

Patch Set 22 : linux #

Patch Set 23 : '' #

Unified diffs Side-by-side diffs Delta from patch set Stats (+273 lines, -14 lines) Patch
M base/base.gypi View 1 chunk +2 lines, -0 lines 0 comments Download
A + base/mach_ipc_mac.h View 11 12 13 14 15 16 17 18 19 20 3 chunks +7 lines, -6 lines 0 comments Download
A + base/mach_ipc_mac.mm View 1 chunk +1 line, -1 line 0 comments Download
M base/process_util.h View 9 10 11 12 13 14 15 16 17 18 19 20 2 chunks +14 lines, -1 line 0 comments Download
M base/process_util_posix.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 2 chunks +143 lines, -1 line 0 comments Download
M chrome/browser/child_process_launcher.cc View 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 2 chunks +20 lines, -2 lines 0 comments Download
M chrome/browser/mach_broker_mac.h View 4 chunks +14 lines, -2 lines 0 comments Download
M chrome/browser/mach_broker_mac.cc View 12 13 14 15 16 17 18 19 20 3 chunks +72 lines, -1 line 0 comments Download

Messages

Total messages: 12 (0 generated)
Nico
My former approach ( http://codereview.chromium.org/460126 ) didn't work with worker and utility processes. Reason 1 ...
9 years, 4 months ago (2010-01-10 06:09:01 UTC) #1
Nico
FFFFFFFFFUUUUU…wrote a long reply, then codereview told me "invalid xsrf token", and now it's gone. ...
9 years, 4 months ago (2010-01-10 21:06:51 UTC) #2
Nico
This is ready for review. I'm pretty happy with it.
9 years, 4 months ago (2010-01-11 02:33:25 UTC) #3
Nico
On 2010/01/11 02:33:25, Nico wrote: > This is ready for review. I'm pretty happy with ...
9 years, 4 months ago (2010-01-11 15:50:25 UTC) #4
Nico
http://codereview.chromium.org/549002/diff/9024/6018 File base/process_util_posix.cc (right): http://codereview.chromium.org/549002/diff/9024/6018#newcode307 base/process_util_posix.cc:307: AutoLock lock(*Singleton<Lock>::get()); fail
9 years, 4 months ago (2010-01-11 16:12:24 UTC) #5
Nico
I switched this to a name-based connection scheme. This another 0.032 ms slower (for a ...
9 years, 4 months ago (2010-01-11 17:50:39 UTC) #6
Mark Mentovai
Hope I got the right one this time. I didn't have a chance to use ...
9 years, 4 months ago (2010-01-11 21:11:30 UTC) #7
Nico
Thank you, good sir. http://codereview.chromium.org/549002/diff/6023/9028 File base/process_util.h (right): http://codereview.chromium.org/549002/diff/6023/9028#newcode168 base/process_util.h:168: // Similar to above, but ...
9 years, 4 months ago (2010-01-11 22:32:26 UTC) #8
Mark Mentovai
This will do. LGTM if you look into whatever I ask about below. http://codereview.chromium.org/549002/diff/9033/9037 File ...
9 years, 4 months ago (2010-01-11 22:43:30 UTC) #9
Nico
http://codereview.chromium.org/549002/diff/9033/9037 File base/process_util.h (right): http://codereview.chromium.org/549002/diff/9033/9037#newcode170 base/process_util.h:170: // responsible for calling |vm_deallocate()| on the returned handle. ...
9 years, 4 months ago (2010-01-12 00:38:04 UTC) #10
Mark Mentovai
Thanks, Nico. That sounds fine to me.
9 years, 4 months ago (2010-01-12 00:50:59 UTC) #11
Nico
9 years, 4 months ago (2010-01-12 21:36:04 UTC) #12
The expert says:

"""
looks like we are talking about an inline port right (MACH_MSG_PORT_DESCRIPTOR)
and not anything out-of-line (like a port array) ...
if it's a single port right you are exchanging, mach_port_deallocate() is fine
"""

Powered by Google App Engine
This is Rietveld 408576698