1337064 - Make tab ID allocation asynchronous

Status: RESOLVED FIXED

(Core :: IPC, defect, P1)

Description

[(no longer active)]

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%)

Comment 1

[(no longer active)]

The 95 percentile time for this is 104ms according to telemetry...

Comment 2

[Samael Wang [:freesamael] (away for now)]

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.

Comment 3

[(no longer active)]

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.

Comment 4

[Samael Wang [:freesamael] (away for now)]

Sorry it's been pended for a while. I'm starting working on this and will get you updated as it progresses.

Comment 5

[Samael Wang [:freesamael] (away for now)]

(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 6

[Samael Wang [:freesamael] (away for now)]

Attachment: Bug 1337064 - Remove sync protocol PContent::AllocateTabId.

Remove sync protocol AllocateTabId. Instead we generate tabId in
each process with nsContentUtils::GenerateTabId, and register
RemoteFrameInfo in parent process. If the tab id was generated from
a content process, it's sent parent through either PBrowserConstructor
or PBrowser::CreateChildProcess.

Review commit: https://reviewboard.mozilla.org/r/128764/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/128764/

Comment 7

[Samael Wang [:freesamael] (away for now)]

Ain't quite confident about this change but would like to have some feedbacks.

Comment 8

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8856857 [details]
Bug 1337064 - Remove sync protocol PContent::AllocateTabId.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128764/diff/1-2/

Comment 9

[Samael Wang [:freesamael] (away for now)]

BTW do you think there should be a dedicated test for this change?

Comment 10

[Kan-Ru Chen [:kanru] (UTC+9)]

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().

Comment 11

[Samael Wang [:freesamael] (away for now)]

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.

Comment 12

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8856857 [details]
Bug 1337064 - Remove sync protocol PContent::AllocateTabId.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128764/diff/2-3/

Comment 13

[(no longer active)]

Thanks Kan-Ru for taking over the review and so sorry for my delay here...

Comment 14

[Samael Wang [:freesamael] (away for now)]

(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

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8856857 [details]
Bug 1337064 - Remove sync protocol PContent::AllocateTabId.

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

Comment 16

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8856857 [details]
Bug 1337064 - Remove sync protocol PContent::AllocateTabId.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128764/diff/3-4/

Comment 17

[Samael Wang [:freesamael] (away for now)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=56f41fb1644c608f7e7c3c9df1fea2750059527f&group_state=expanded

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a69ca6edbe97
Remove sync protocol PContent::AllocateTabId. r=kanru

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a69ca6edbe97