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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Performance Impact high
Tracking Status
firefox56 --- fixed

People

(Reporter: mconley, Assigned: gkrizsanits)

References

Details

Attachments

(2 files)

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.
Whiteboard: [qf]
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
Bug 1348361 seems related too...
See Also: → 1348361
Whiteboard: [qf] → [qf:p1]
Attached patch ppm blocker. v1Splinter Review
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 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.
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)
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)
This seems to have been unrelated, please reland it.
Flags: needinfo?(aryx.bugmail)
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
https://hg.mozilla.org/mozilla-central/rev/dbcc5752dea8
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
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?
Attached patch rebased for betaSplinter Review
This is the patch we talked about during the e10s-multi meeting.
Flags: needinfo?(lhenry)
Can you elaborate on the need for this patch in 55, for someone who was not at that meeting?
Flags: needinfo?(gkrizsanits)
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)
Attachment #8881827 - Flags: approval-mozilla-beta?
Performance Impact: --- → P1
Whiteboard: [qf:p1]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: