1397181 - Label child actors allocation using SystemGroup

Status: RESOLVED WONTFIX

(Core :: DOM: Core & HTML, enhancement, P2)

Description

[Bevis Tseng[:bevis][:btseng](Exited)]

I'd like to restrict that all the allocation of child actors wouldn't notify any web content. The benefit is that we could label all the constructor ipc message using SystemGroup.

If not, we should move this notification to a new runnable instead.

Note: all the actors and its message can be labeled with other SechedulerGroup by overriding its IToplevelProtocol::GetConstructedEventTarget.

Comment 1

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (WIP) Label child actors allocation using SystemGroup

Comment 2

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v1) Patch: Part 1: Label child actors allocation using SystemGroup.

After further study of each AllXxxChild implementation, I didn't see any exception which touches the web content during actor child allocation.
Hence, I'd like to have all the child actor allocation on main thread labeled with SystemGroup by default if not specified in IToplevelProtocol::GetConstructedEventTarget().

For future implementation which would like to touch the web content right after allocation, I'd suggest to separate this logic from allocation by defining a dedicated message/runnable to handle it instead.

Treeherder result looks fine as well, btw:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=45f251b85f8ec1df013426916bacf0034201550b

Comment 3

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v1) Patch: Part 2: Remove dead code of PContent::Msg_PIPCBlobInputStreamConstructor__ID from ContentChild.

This is to fix the mistake I've made in Bug 1394350 which is a dead code to a constructor message according to the logic in IToplevelProtocol::GetMessageEventTarget().
Sorry for not thinking over it carefully. :\

Comment 4

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Bevis Tseng[:bevis][:btseng] from comment #2)
> Created attachment 8905401 [details] [diff] [review]
> (v1) Patch: Part 1: Label child actors allocation using SystemGroup.
> 
> After further study of each AllXxxChild implementation, I didn't see any
> exception which touches the web content during actor child allocation.
> Hence, I'd like to have all the child actor allocation on main thread
> labeled with SystemGroup by default if not specified in
> IToplevelProtocol::GetConstructedEventTarget().
> 
> For future implementation which would like to touch the web content right
> after allocation, I'd suggest to separate this logic from allocation by
> defining a dedicated message/runnable to handle it instead.
> 
> Treeherder result looks fine as well, btw:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=45f251b85f8ec1df013426916bacf0034201550b

After reading bug 1398420, I think I didn't think over carefully whether a system group runnable has a dependency to another labeled runnable even though this system group runnable didn't touch any web content.
Besides, most unlabeled child actors are allocated proactively by content process instead of receiving a request from parent. The improvement of the labeling by this patch is mostly for PContent::Msg_PIPCBlobInputStreamConstructor__ID according to the telemetry analysis.

Maybe we should put this patch on hold and label each constructor message individually after clarifying its dependency on other runnables.

Comment 5

[Bevis Tseng[:bevis][:btseng](Exited)]

Set to WONTFIX per comment 4.