Closed
Bug 1373660
Opened 7 years ago
Closed 7 years ago
Preallocated content process is launching before first paint
Categories
(Core :: DOM: Content Processes, defect)
Core
DOM: Content Processes
Tracking
()
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: mconley, Assigned: gkrizsanits)
References
Details
Attachments
(2 files)
11.14 KB,
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
11.11 KB,
patch
|
Details | Diff | Splinter Review |
Here's a slice from a start-up profile: https://perfht.ml/2sj3xwY The preallocation of the content process is occurring before first paint. We should probably defer that until sometime after first paint - perhaps done when the user is idle.
Reporter | ||
Updated•7 years ago
|
Whiteboard: [qf]
Assignee | ||
Comment 1•7 years ago
|
||
Right, I think I've made a mistake. So the intention was to do something like that: http://searchfox.org/mozilla-central/rev/20d16dadd336e0c6b97e3e19dc4ff907744b5734/dom/ipc/ContentParent.cpp#2719 but then I seem to forgot to prevent PreallocatedProcessManagerImpl::Enable() to launch it earlier. I can take this.
Assignee: nobody → gkrizsanits
Updated•7 years ago
|
Whiteboard: [qf] → [qf:p1]
Assignee | ||
Comment 3•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5bd4d42849d0559ceefb3a00056810463eb33603
Assignee | ||
Comment 4•7 years ago
|
||
I've added some description in the patch comment. I think all the test failures from the last try run are known intermittents, and it was just an unlucky push. We'll see...
Attachment #8881827 -
Flags: review?(mrbkap)
Comment 5•7 years ago
|
||
Comment on attachment 8881827 [details] [diff] [review] ppm blocker. v1 Review of attachment 8881827 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/ipc/PreallocatedProcessManager.cpp @@ +67,5 @@ > > bool mEnabled; > bool mShutdown; > RefPtr<ContentParent> mPreallocatedProcess; > + std::set<uint64_t> mBlockers; This should probably use a Mozilla datastructure.
Attachment #8881827 -
Flags: review?(mrbkap) → review+
Pushed by gkrizsanits@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/aa05515c1c87 Block the preallocated process manager while a content process is being launched.
Comment 7•7 years ago
|
||
Backed out for frequently timing out in toolkit/components/extensions/test/xpcshell/test_ext_i18n.js on OS X 10.10 opt: https://hg.mozilla.org/integration/mozilla-inbound/rev/7efddd72ba59c19ab21efc7b21c57cd13e123aa7 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=7efddd72ba59c19ab21efc7b21c57cd13e123aa7&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-resultStatus=success&filter-searchStr=xpcshell+10.10 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=110891540&repo=mozilla-inbound 15:35:19 INFO - (xpcshell/head.js) | test run_next_test 2 finished (2) 15:35:19 INFO - "CONSOLE_MESSAGE: (error) [JavaScript Error: "DataError: Data provided to an operation does not meet requirements."] 15:35:19 INFO - wrapMethods/cls.prototype[method]@resource://gre/modules/IndexedDB.jsm:103:47 15:35:19 INFO - get@resource://gre/modules/ExtensionParent.jsm:1341:22 15:35:19 INFO - async*parseManifest@resource://gre/modules/Extension.jsm:873:12 15:35:19 INFO - loadManifest@resource://gre/modules/Extension.jsm:543:7 15:35:19 INFO - async*loadManifest@resource://gre/modules/Extension.jsm:878:12 15:35:19 INFO - _startup@resource://gre/modules/Extension.jsm:1055:42 15:35:19 INFO - async*startup@resource://gre/modules/Extension.jsm:1031:27 15:35:19 INFO - startup@resource://testing-common/ExtensionXPCShellUtils.jsm:281:27 15:35:19 INFO - test_get_ui_language@/Users/cltbld/tasks/task_1498774118/build/tests/xpcshell/tests/toolkit/components/extensions/test/xpcshell/test_ext_i18n.js:290:9 15:35:19 INFO - async*asyncFunction@resource://gre/modules/Task.jsm:241:18 15:35:19 INFO - Task_spawn@resource://gre/modules/Task.jsm:166:12 15:35:19 INFO - _run_next_test@/Users/cltbld/tasks/task_1498774118/build/tests/xpcshell/head.js:1544:9 15:35:19 INFO - run@/Users/cltbld/tasks/task_1498774118/build/tests/xpcshell/head.js:706:9 15:35:19 INFO - _do_main@/Users/cltbld/tasks/task_1498774118/build/tests/xpcshell/head.js:220:3 15:35:19 INFO - _execute_test@/Users/cltbld/tasks/task_1498774118/build/tests/xpcshell/head.js:549:5 15:35:19 INFO - @-e:1:1 15:35:19 INFO - " 15:35:19 INFO - TEST-PASS | xpcshell-remote.ini:toolkit/components/extensions/test/xpcshell/test_ext_i18n.js | test_get_ui_language - [test_get_ui_language : 243] Got expected getUILanguage result in background - Expected: en_US, Actual: en_US - true == true 15:35:19 INFO - TEST-PASS | xpcshell-remote.ini:toolkit/components/extensions/test/xpcshell/test_ext_i18n.js | test_get_ui_language - [test_get_ui_language : 243] Got expected getMessage result in background - Expected: en_US, Actual: en_US - true == true 15:35:19 INFO - <<<<<<<
Flags: needinfo?(gkrizsanits)
Assignee | ||
Comment 8•7 years ago
|
||
The failure you linked is in non-e10s mode where my code is inactive so I doubt it's related. And the backout did not seem to fix the problem. Was this the only reason for the backout? This is an important patch so if there isn't any other reason for the backout I'm going to repush it.
Flags: needinfo?(gkrizsanits) → needinfo?(aryx.bugmail)
Comment 9•7 years ago
|
||
This seems to have been unrelated, please reland it.
Flags: needinfo?(aryx.bugmail)
Comment 10•7 years ago
|
||
Pushed by gkrizsanits@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/dbcc5752dea8 Block the preallocated process manager while a content process is being launched. r=mrbkap
Comment 11•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/dbcc5752dea8
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Assignee | ||
Comment 12•7 years ago
|
||
Comment on attachment 8881827 [details] [diff] [review] ppm blocker. v1 Approval Request Comment [Feature/Bug causing the regression]: It's not a regression. It's an optimization on the preallocated process manager. [User impact if declined]: We might start the preallocated content process at the wrong time, delaying firstpaint. [Is this code covered by automated tests?]: The code is used by all browser tests, and some other test suits, so it's heavily tested. There is also a probe related to this feature, that would show regression if something were not OK. But it looks OK. [Has the fix been verified in Nightly?]: Yes. [Needs manual test from QE? If yes, steps to reproduce]: No. [List of other uplifts needed for the feature/fix]: None. [Is the change risky?]: Low risk. [Why is the change risky/not risky?]: The patch does not do anything new, it just makes sure that we're more careful about when we start up the preallocated process. [String changes made/needed]: None.
Attachment #8881827 -
Flags: approval-mozilla-beta?
Assignee | ||
Comment 13•7 years ago
|
||
Assignee | ||
Comment 14•7 years ago
|
||
This is the patch we talked about during the e10s-multi meeting.
Flags: needinfo?(lhenry)
Comment 15•7 years ago
|
||
Can you elaborate on the need for this patch in 55, for someone who was not at that meeting?
Flags: needinfo?(gkrizsanits)
Assignee | ||
Comment 16•7 years ago
|
||
Let's just put this request on hold. I want to investigate Bug 1368036, which seems to be related.
Flags: needinfo?(lhenry)
Flags: needinfo?(gkrizsanits)
Assignee | ||
Updated•7 years ago
|
Attachment #8881827 -
Flags: approval-mozilla-beta?
Updated•2 years ago
|
Performance Impact: --- → P1
Whiteboard: [qf:p1]
You need to log in
before you can comment on or make changes to this bug.
Description
•