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.
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
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 email@example.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/aa05515c1c87 Block the preallocated process manager while a content process is being launched.
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.
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/dbcc5752dea8 Block the preallocated process manager while a content process is being launched. r=mrbkap
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.
This is the patch we talked about during the e10s-multi meeting.
Can you elaborate on the need for this patch in 55, for someone who was not at that meeting?
Let's just put this request on hold. I want to investigate Bug 1368036, which seems to be related.
You need to log in before you can comment on or make changes to this bug.