Make tab ID allocation asynchronous

RESOLVED FIXED in Firefox 55

Status

()

P1
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: Ehsan, Assigned: freesamael)

Tracking

(Blocks: 1 bug)

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [qf:p1])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
PContent::Msg_AllocateTabId can take a long time:

<https://telemetry.mozilla.org/new-pipeline/dist.html#!cumulative=0&end_date=2017-02-05&keys=PContent%253A%253AMsg_AllocateTabId!PContent%253A%253AMsg_RpcMessage!PContent%253A%253AMsg_GetGfxVars!PAPZCTreeManager%253A%253AMsg_ReceiveMouseInputEvent&max_channel_version=nightly%252F54&measure=IPC_SYNC_LATENCY_MS&min_channel_version=null&product=Firefox&sanitize=1&sort_keys=submissions&start_date=2017-02-02&table=0&trim=1&use_submission_date=0>

Start	End	IPC_SYNC_LATENCY_MS Count
0	1	0 (0%)
1	3	43.5k (37.83%)
3	8	17.02k (14.8%)
8	20	18.27k (15.88%)
20	50	21.66k (18.84%)
50	126	11.19k (9.73%)
126	317	2.84k (2.47%)
317	796	418 (0.36%)
796	2k	67 (0.06%)
2k	Infinity	31 (0.03%)
(Reporter)

Comment 1

2 years ago
The 95 percentile time for this is 104ms according to telemetry...
Ehsan,

I'm interested at helping but may need some mentoring in this bug. Olli mentioned maybe we could allocate tab id from child side. Also had a short discussion with Kanru. I'll see what I can do here and may reach you later if I need help. Suggestions are welcome.
Assignee: nobody → sawang
(Reporter)

Comment 3

2 years ago
Thanks for working on this!

I have a rough suggestion for what to do here.  ContentProcessManager::AllocateTabId() is the underlying logic that we need to modify.  As you can see, that function does two distinct tasks: Updating ContentProcessInfo::mRemoteFrames, and actually allocating a tab ID (which is incrementing gTabId.)

The second part can easily be done in the child process.  Currently gTabId lives in the parent.  What we want to do is to partition the 64-bit integer space into ranges of N identifiers (which would be the maximum number of TabChilds we can create in a content process, we should be able to easily set this to something very high, like a million.)  The parent process can be in charge of telling each child process about its tab ID range by passing something on the command line.  My patch in bug 1332036 shows a very simple way to do that.

The first part of the work in that function can only happen in the parent process.  If you look at the callers of ContentParent::AllocateTabId(), I can see 3 different cases:

 * <https://dxr.mozilla.org/mozilla-central/rev/0eef1d5a39366059677c6d7944cfe8a97265a011/dom/ipc/ContentParent.cpp#802> which can probably be safely removed, it seems to be an optimization around saving on the sync IPC message.
 * <https://dxr.mozilla.org/mozilla-central/rev/0eef1d5a39366059677c6d7944cfe8a97265a011/dom/ipc/ContentParent.cpp#4097> which is the sync IPC message in question.  On the child process side <https://dxr.mozilla.org/mozilla-central/rev/0eef1d5a39366059677c6d7944cfe8a97265a011/dom/ipc/ContentChild.cpp#769> soon after we allocate the tab ID, we send a PBrowser constructor IPC message.  We can send the tabId we just allocated alongside that message.  Then in the parent process side, you need to do the updating of mRemoteFrames.  I think you probably want to add a max tab ID member to RemoteFrameInfo and update it here.  It'll be used below.
 * <https://dxr.mozilla.org/mozilla-central/rev/0eef1d5a39366059677c6d7944cfe8a97265a011/dom/ipc/ContentParent.cpp#1030> here we are in the parent process already.  This is sort of the reverse case of the first bullet point, in that we can use the max tab ID we store in the previous step to compute a new one, and send what we just computed to the child process again as part of PBrowser constructor.  That way the child process can update its gTabId or similar accordingly.

You should be a bit careful around race conditions, such as the child process allocating a tabId and before having a chance to send it through the PBrowser ctor, the parent process allocating another tabId.  But another approach may be allocating a full separate range for the parent process and use it in the 3rd bullet point case and not bother with storing the max tab ID, this way the parent and child can create as many tabId's as they want in whatever order.  We do need to ensure the correctness of the information stored in mRemoteFrames, but I think by sending tabIds alongside PBrowser ctor that should not be an issue.

I hope there isn't any details that I'm missing.  Please double check it all since I wrote this really quickly so it may be that I overlooked some detail.  I'd be happy to help more if needed.
Whiteboard: [qf:p1]
Priority: -- → P1
Sorry it's been pended for a while. I'm starting working on this and will get you updated as it progresses.
(In reply to :Ehsan Akhgari (busy) from comment #3)
> <https://dxr.mozilla.org/mozilla-central/rev/
> 0eef1d5a39366059677c6d7944cfe8a97265a011/dom/ipc/ContentParent.cpp#802>
> which can probably be safely removed, it seems to be an optimization around
> saving on the sync IPC message.

Just a note. This one seems to be used in nested OOP (something I never quite understand...), so if I understand it correctly it was:

| Parent Process          | Child Process            | Grand Child Process  |
|-------------------------|--------------------------|----------------------|
|                         |  CreateBrowser           |                      |
| RecvCreateChildProcess <-- SendCreateChildProcess  |                      |
| AllocateTabId           |                          |                      |
|                         |  SendPBrowserConstructor --> AllocPBrowserChild |
|---------------------------------------------------------------------------| 

If I generate tabId right in the child process (I'm taking the window id approache to generate unique id across processes), I'll need to send another message to parent process to update RemoteFrameInfo. I'm not sure if that has to happen before SendPBrowserConstructor.
Comment hidden (mozreview-request)
Ain't quite confident about this change but would like to have some feedbacks.
Comment hidden (mozreview-request)
BTW do you think there should be a dedicated test for this change?

Comment 10

a year ago
mozreview-review
Comment on attachment 8856857 [details]
Bug 1337064 - Remove sync protocol PContent::AllocateTabId.

https://reviewboard.mozilla.org/r/128764/#review134796

I think I can review this. Overall this looks good to me but requires some change for nested case.

::: dom/ipc/ContentChild.cpp:772
(Diff revision 2)
> -  TabId tabId;
> +  TabId tabId(nsContentUtils::GenerateTabId());
> -  SendAllocateTabId(openerTabId,
> -                    *ipcContext,
> -                    GetID(),
> -                    &tabId);
> -

We still need to register the id to the main process here. See comments below.

::: dom/ipc/ContentProcessManager.cpp
(Diff revision 2)
> -
> -    const PopupIPCTabContext &ipcContext = aContext.get_PopupIPCTabContext();
> -    MOZ_ASSERT(ipcContext.opener().type() == PBrowserOrId::TTabId);
> -
> -    remoteFrameIter = iter->second.mRemoteFrames.find(ipcContext.opener().get_TabId());
> -    if (remoteFrameIter == iter->second.mRemoteFrames.end()) {
> -      ASSERT_UNLESS_FUZZING("Failed to find tab id.");
> -      return TabId(0);
> -    }
> -

Why do you remove this check?

::: dom/ipc/PContent.ipdl:902
(Diff revision 2)
>      async DeallocateTabId(TabId tabId,
>                            ContentParentId cpId,
>                            bool aMarkedDestroying);

I think this message could be changed to UnregisterRemoteFrame as well.

::: dom/ipc/nsIContentParent.cpp:156
(Diff revision 2)
> +
> +    // The creation of PBrowser was triggered from content process.
> +    // We need to register remote frame with the child generated tab id.
> +    TabId openerTabId = opener->GetTabId();
> +    ContentProcessManager* cpm = ContentProcessManager::GetSingleton();
> +    if (!cpm->RegisterRemoteFrame(aTabId, openerTabId, aContext, aCpId)) {
> +      return nullptr;
> +    }

This will assert in nested process because ContentProcessManager is only available in the main process.

Let's rename the sync AllocateTabId message to async RegisterRemoteFrame and still register the frame in ContentChild::ProvideWindowCommon().
Attachment #8856857 - Flags: review-
(Assignee)

Comment 11

a year ago
mozreview-review-reply
Comment on attachment 8856857 [details]
Bug 1337064 - Remove sync protocol PContent::AllocateTabId.

https://reviewboard.mozilla.org/r/128764/#review134796

> We still need to register the id to the main process here. See comments below.

As per offline discussion, since ProvideWindowCommon doesn't work for grand child process at this moment anyway, in this bug we'd focus on normal child process handling only, and it's done by sending TabId along with SendPBrowserConstructor.

> Why do you remove this check?

As per offline discussion, at this point the IPCTabContext will hold a opener TabParent instead of a TabId. Since it wasn't a critical check we'd just skip this part.

> This will assert in nested process because ContentProcessManager is only available in the main process.
> 
> Let's rename the sync AllocateTabId message to async RegisterRemoteFrame and still register the frame in ContentChild::ProvideWindowCommon().

Added a check & MOZ_ASSERT for ContentBridgeParent case.
Attachment #8856857 - Flags: review?(ehsan)
Comment hidden (mozreview-request)
(Reporter)

Comment 13

a year ago
Thanks Kan-Ru for taking over the review and so sorry for my delay here...
(In reply to :Ehsan Akhgari (super long backlog, slow to respond, not reviewing for a while) from comment #13)
> Thanks Kan-Ru for taking over the review and so sorry for my delay here...

We all know you've been super busy :)

Comment 15

a year ago
mozreview-review
Comment on attachment 8856857 [details]
Bug 1337064 - Remove sync protocol PContent::AllocateTabId.

https://reviewboard.mozilla.org/r/128764/#review135602
Attachment #8856857 - Flags: review?(kchen) → review+
Comment hidden (mozreview-request)
Keywords: checkin-needed

Comment 18

a year ago
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a69ca6edbe97
Remove sync protocol PContent::AllocateTabId. r=kanru
Keywords: checkin-needed

Comment 19

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a69ca6edbe97
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.