Closed Bug 1488993 Opened 2 years ago Closed 1 year ago
Process Manager “blocker” logic around preallocated processes and errors
46 bytes, text/x-phabricator-request
|Details | Review|
I found some issues with preallocated process blockers while working on bug 1446161: * Preallocated processes themselves don't need to be blockers; PreallocatedProcessManager currently supports at most one process, and if we support multiple concurrent launches in the future, it's not something we'd do accidentally * RemoveBlocker isn't called in some error cases: if the process fails to launch, or if it's destroyed before reaching RecvFirstIdle. In that case a blocker would be leaked and we'd never preallocate afterwards. I have a patch for this.
This makes sure to release blockers (so that content process preallocation can resume) in error cases, and stops making preallocated processes themselves blockers, because it's unnecessary (we don't currently support multiple preallocated processes) and not doing it means not having to handle those error cases as well. (Also, in the future we might want to allow the possibility of multiple concurrent launches if the hardware can support it with acceptable performance.)
Comment on attachment 9008597 [details] Bug 1488993 - Fix PreallocatedProcessManager blocker handling Mike Conley (:mconley) (:⚙️) has approved the revision.
Attachment #9008597 - Flags: review+
Attachment #9008597 - Attachment description: Bug 1488993 - Fix PreallocatedProcessManager "blocker" handling → Bug 1488993 - Fix PreallocatedProcessManager blocker handling
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/933bd1aed20f Fix PreallocatedProcessManager blocker handling r=mconley
So there are a couple of things wrong with this patch, which I found while investigating bug 1491817: 1. We *do* clean up blockers in ActorDestroy — indirectly, via the ipc:content-shutdown observable. See https://searchfox.org/mozilla-central/rev/6c82481caa506a240a626bb44a2b8cbe0eedb3a0/dom/ipc/PreallocatedProcessManager.cpp#310 2. Previously, the preallocated process would remain a blocker until it had created its first TabChild and then reached an idle state; with this patch, the next preallocation is allowed as soon as the process is taken. However, there's at least a 1s delay before it will actually try (parent main thread idle followed by 1s timer), so that may not matter in practice. I was going to back the patch out while I reconsider it, but there are infra problems (bug 1491948).
(In reply to Jed Davis [:jld] (⏰UTC-6) from comment #5) > 2. Previously, the preallocated process would remain a blocker until it had > created its first TabChild and then reached an idle state; with this patch, > the next preallocation is allowed as soon as the process is taken. However, > there's at least a 1s delay before it will actually try (parent main thread > idle followed by 1s timer), so that may not matter in practice. I profiled on Linux; from the first PBrowser constructor to the following main thread idle was 98ms, of which 45ms was spent in nsLookAndFeel::EnsureInit asking GTK about themes. So this might not make a difference in practice.
Backout by firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/b86d2f48f89c Backed out changeset 933bd1aed20f
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I tried another approach to this, leaving as much of the existing behavior intact as I could without breaking the future async launch change,by moving the AddBlocker calls to immediately after launching for normal launch, or after PreallocatedProcessManager::Take for preallocated. (We don't need to have a blocker *while* launching, because this is all on the main thread.) That almost works, and it almost means that the assertion in RemoveBlocker could stay, but it failed intermittently on Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d0b78ff9026c1ac72cc22ab3eac9f5a09278928a The failured are because the preallocated process was already registered as a blocker when it was taken. The only thing I can think of that would cause that would be if a newly launched process is recycled (PreallocatedProcessManager::Provide) and then re-taken, all before RecvFirstIdle. (If it's possible for a process to be launched and then recycled without ever having a PBrowser created, that makes this more plausible, because in that case the FirstIdle runnable won't have been scheduled yet.) So I'm leaning back towards just not making preallocated processes blockers at any time (which, given comment #6, doesn't seem likely to change anything).
This fixes/adjusts two things about how content process preallocation is blocked: 1. Processes aren't registered as blockers until after they launch successfully. The main goal is to not leak a blocker if launch fails, but we don't need to block *while* launching synchronously, because this all happens on the main thread. 2. Preallocated processes themselves aren't blockers. The main goal here is so that async preallocation doesn't need extra complexity to avoid being blocked by itself when launch completes. This mostly doesn't affect actual behavior, because we currently support at most one preallocated process. The difference is the window from when the process is sent its first PBrowserConstructor until when it's next idle, where there is now no longer a blocker, but this seems to be relatively short (~100ms) and we don't even try to launch a new process until at least 1s + an idle runnable. This patch does not explicitly RemoveBlocker in ActorDestroy like the first attempt did, because it's unnecessary: this is handled in the ipc:content-shutdown observer.
Attachment #9008597 - Attachment is obsolete: true
Attachment #9017745 - Attachment description: Bug 1488993 - Fix PreallocatedProcessManager blocker management (v2) → Bug 1488993 - Fix PreallocatedProcessManager blocker management (v2).
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/cea43bc88c7a Fix PreallocatedProcessManager blocker management (v2). r=mconley,smaug
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/e0e1a14b592f Fix PreallocatedProcessManager blocker management (v2). r=mconley,smaug
You need to log in before you can comment on or make changes to this bug.