Closed
Bug 786631
Opened 12 years ago
Closed 12 years ago
Support preloading processes that are launched with INHERIT_PRIVILEGES
Categories
(Core :: General, defect)
Core
General
Tracking
()
People
(Reporter: cjones, Assigned: cjones)
References
Details
(Whiteboard: [FFOS_perf])
Attachments
(3 files, 2 obsolete files)
8.19 KB,
patch
|
dhylands
:
review+
|
Details | Diff | Splinter Review |
12.45 KB,
patch
|
justin.lebar+bug
:
review+
|
Details | Diff | Splinter Review |
19.94 KB,
patch
|
cjones
:
review+
overholt
:
approval-mozilla-b2g18+
|
Details | Diff | Splinter Review |
Bug 782456 added a hack to inherit permissions for apps that need them. This hack prevents using the prelaunched process for these inherit-permission processes, meaning that they will incur startup overhead. We can fix that by always launching the prelaunch process with INHERIT and then dropping privs after we know which app it's going to run.
Assignee | ||
Comment 4•12 years ago
|
||
We may need this to hit the targets in bug 817051 and bug 817116. We can make this work by deferring privilege dropping until we load the first TabChild "for real", here. That's not supported on all platforms though (linux supports it) and it makes me very squeamish security-wise. But it's not hard to implement. [1] http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp#147
Assignee | ||
Comment 5•12 years ago
|
||
It looks like we're going to need this to hit startup targets. Siiiigh.
Assignee: nobody → jones.chris.g
Assignee | ||
Comment 6•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #703198 -
Flags: review?(dhylands)
Assignee | ||
Comment 8•12 years ago
|
||
Comment on attachment 703198 [details] [diff] [review] part 1: Refactor privilege adjustment Botched something in patch upload, one minute.
Attachment #703198 -
Attachment is obsolete: true
Attachment #703198 -
Flags: review?(dhylands)
Assignee | ||
Updated•12 years ago
|
Attachment #703200 -
Attachment is obsolete: true
Attachment #703200 -
Flags: review?(justin.lebar+bug)
Assignee | ||
Updated•12 years ago
|
Attachment #703204 -
Flags: review? → review?(justin.lebar+bug)
Comment 11•12 years ago
|
||
Comment on attachment 703204 [details] [diff] [review] part 2: Make the prelaunch process totipotent and specialize when it's taken Looks great.
Attachment #703204 -
Flags: review?(justin.lebar+bug) → review+
Updated•12 years ago
|
Attachment #703202 -
Flags: review?(dhylands) → review+
Comment 12•12 years ago
|
||
(In reply to Chris Jones [:cjones] [:warhammer] from comment #0) > Bug 782456 added a hack to inherit permissions for apps that need them. > This hack prevents using the prelaunched process for these > inherit-permission processes, meaning that they will incur startup overhead. > We can fix that by always launching the prelaunch process with INHERIT and > then dropping privs after we know which app it's going to run. I think we should drop privs as soon as possible after launching, using seteuid/setegid. Then once we know which app we're going to be, we should elevate back to root (seteuid(0)) and finally do the permanent set (so no more elevation possible).
Assignee | ||
Comment 14•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/c26533f49aaf https://hg.mozilla.org/integration/mozilla-inbound/rev/8463ca8a191a
Assignee | ||
Comment 15•12 years ago
|
||
I seem to have grown approval powers, but it's bad form to get high on your own supply. This is not a zero-risk change, but it's relatively safe, easy to verify correct, and wins back startup time needed for product requirements that we can't get in any other way.
Attachment #703486 -
Flags: review+
Attachment #703486 -
Flags: approval-mozilla-b2g18?
Assignee | ||
Updated•12 years ago
|
blocking-b2g: --- → tef?
Assignee | ||
Comment 16•12 years ago
|
||
Backed out, win32 borkage https://hg.mozilla.org/integration/mozilla-inbound/rev/66f820124d4a https://hg.mozilla.org/integration/mozilla-inbound/rev/1e2f112e2de6 https://hg.mozilla.org/integration/mozilla-inbound/rev/4a8d8c2d97fd
Assignee | ||
Comment 17•12 years ago
|
||
Round two https://hg.mozilla.org/integration/mozilla-inbound/rev/a18d9b97ac9c https://hg.mozilla.org/integration/mozilla-inbound/rev/b67c3370a85f
Assignee | ||
Comment 18•12 years ago
|
||
(In reply to Chris Jones [:cjones] [:warhammer] from comment #15) > Created attachment 703486 [details] [diff] [review] > Rebased on gecko-18 (Imported the fix to this patch, but not reposting to avoid churn.)
Updated•12 years ago
|
blocking-b2g: tef? → tef+
Updated•12 years ago
|
Attachment #703486 -
Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
Comment 19•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a18d9b97ac9c https://hg.mozilla.org/mozilla-central/rev/b67c3370a85f
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Comment 20•12 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g18/rev/d9d299d9e6cf
status-b2g18:
--- → fixed
status-firefox19:
--- → wontfix
status-firefox20:
--- → wontfix
status-firefox21:
--- → fixed
Comment 21•12 years ago
|
||
Follow-up for bustage (the roll-up patch was missing an include in ContentChild.cpp). https://hg.mozilla.org/releases/mozilla-b2g18/rev/635fb4ce37af
Comment 22•12 years ago
|
||
(In reply to Chris Jones [:cjones] [:warhammer] from comment #18) > (In reply to Chris Jones [:cjones] [:warhammer] from comment #15) > > Created attachment 703486 [details] [diff] [review] > > Rebased on gecko-18 > > (Imported the fix to this patch, but not reposting to avoid churn.) Guess that should have been enough to keep me away. Backed out for win32 bustage. https://hg.mozilla.org/releases/mozilla-b2g18/rev/668e0cdfcc84
Comment 23•12 years ago
|
||
We did some subjective testing of this patch and it didn't seem to gain us much, so I run it through the profiler and discovered something weird. This is the Camera application starting up w/o the patch: http://people.mozilla.com/~bgirard/cleopatra/#report=2d352069167ff49b2f6239fea3ef1b98c3c97ded If you look at the phase which precedes the first ProcessNextNativeEvent::Wait pause it lasts for ~2.6s (256 samples), that's the app starting up from scratch. This on the other hand is a profile with the patch applied: http://people.mozilla.com/~bgirard/cleopatra/#report=fd3a57acc94e727a0eba4af5053d573d0ce0ddad The first phase which happens while the app is being preloaded takes ~0.6s (63 samples) then the real launch phase lasts ~2.4s (236 samples). So it looks like a ~0.4s slow down was introduced during the application launch phase which eats up most of the time saved by preloading it. Unfortunately just looking at the profiles I cannot easily tell where the extra delay is coming from. BTW all testing was done on the Otoro with this mornings gecko/gaia checkouts.
Comment 24•12 years ago
|
||
If I had to guess, I'd say that you're losing due to cold caches (the L1/L2 caches) and possibly (although seems like more of a stretch) cold file cache. The L1/L2 stuff is really hard to instrument without access to on-chip performance registers. If you could monitor cache-hits/cache-misses then you could compare those between the 2 runs.
Comment 25•12 years ago
|
||
Gabriele, do your results apply to other apps as well? That is, how much is preloading buying us for, say, the FM radio app? I think all you need to do to disable preloading is to set dom.ipc.processPrelaunch.enabled to false (b2g/app/b2g.js). > The L1/L2 stuff is really hard to instrument without access to on-chip performance > registers. Hopefully I'll get perf working soon, and then we can go to town. Bug 831611.
Comment 26•12 years ago
|
||
(In reply to Dave Hylands [:dhylands] from comment #24) > If I had to guess, I'd say that you're losing due to cold caches (the L1/L2 > caches) and possibly (although seems like more of a stretch) cold file cache. IMHO 400ms sound like a bit too much to be caused by cold caches/TLB, but your comment gave me an idea. I want to check if we're flushing some of our internal caches and how much I/O is involved in the process; I've run these tests on the Otoro so slow I/O might actually be a significant factor. (In reply to Justin Lebar [:jlebar] from comment #25) > Gabriele, do your results apply to other apps as well? That is, how much is > preloading buying us for, say, the FM radio app? > > I think all you need to do to disable preloading is to set > dom.ipc.processPrelaunch.enabled to false (b2g/app/b2g.js). I haven't tried yet but I will. I'll start with the Video app which should also be affected by this patch.
Comment 27•12 years ago
|
||
I tried profiling the Template app with and without preloading and I can reproduce the same issue: - This is without preloading the whole startup process takes ~1240ms http://people.mozilla.com/~bgirard/cleopatra/#report=a85b122c1f323253cb9ee322f28303111e6625b0 - This is with preloading, the preload phase takes ~620ms and the launch phase ~980ms so 1600ms in total http://people.mozilla.com/~bgirard/cleopatra/#report=15dc4e0c9ff20b64dc607c3329e34c48dd834bea I will try to experiment on another phone to see if it might be caused by slow I/O. Shall I open a separate bug for this?
Assignee | ||
Comment 29•12 years ago
|
||
(In reply to Ryan VanderMeulen from comment #22) > (In reply to Chris Jones [:cjones] [:warhammer] from comment #18) > > (In reply to Chris Jones [:cjones] [:warhammer] from comment #15) > > > Created attachment 703486 [details] [diff] [review] > > > Rebased on gecko-18 > > > > (Imported the fix to this patch, but not reposting to avoid churn.) > > Guess that should have been enough to keep me away. Backed out for win32 > bustage. > https://hg.mozilla.org/releases/mozilla-b2g18/rev/668e0cdfcc84 Sorry, should have announced comment 18 louder.
Comment 31•12 years ago
|
||
Backed out for bustage. https://hg.mozilla.org/releases/mozilla-b2g18/rev/f00db85a56da https://tbpl.mozilla.org/php/getParsedLog.php?id=19061198&tree=Mozilla-B2g18 /builds/slave/m-b2g18-osx64-g/build/dom/ipc/ContentChild.cpp:501:27: error: use of undeclared identifier 'GeckoChildProcessHost' GeckoChildProcessHost::DefaultChildPrivileges() :
Assignee | ||
Comment 32•12 years ago
|
||
(In reply to Ryan VanderMeulen from comment #31) > Backed out for bustage. > f00d Mmmmm best lunchtime backout evar. Thanks! Repushed with header fix, for what must have been new rottage https://hg.mozilla.org/releases/mozilla-b2g18/rev/e718cccd8ed8
Comment 33•12 years ago
|
||
Fwiw https://hg.mozilla.org/mozilla-central/rev/a18d9b97ac9c broke builds on bsd since SetCurrentProcessPrivileges() is not defined in process_util_bsd.cc. Filing a followup as soon as i have a fix.
Updated•12 years ago
|
Whiteboard: [perf_tsk]
Updated•12 years ago
|
Whiteboard: [perf_tsk] → [FFOS_perf]
Comment 34•12 years ago
|
||
Landed on mozilla-b2g18 prior to the 1/25 branching to mozilla-b2g18_v1_0_0, updating status-b2g-v1.0.0
status-b2g18-v1.0.0:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•