1373773 - PCompositorBridge::Msg_NotifyChildCreated sync IPC when opening tabs

Status: NEW

(Core :: Graphics: Layers, enhancement, P3)

Description

[Markus Stange [:mstange]]

Profile: http://bit.ly/2rzzSmg

If you enable Activity Stream using the pref browser.newtabpage.activity-stream.enabled and open new tabs, the parent process sends a synchronous IPC messsage "NotifyChildCreated" to the compositor thread. This slows down tab opening.

The Activity Stream pref makes a difference here because the current "new tab" page runs in the parent process, whereas the new one will run in the content process.

We preload "new tab" tabs, so you may need to open more than one new tab before the issue becomes apparent.

Comment 1

[Andrew Overholt [:overholt]]

Given we want to ship Activity Stream soon (that's my understanding, at least), I'm inclined to mark this [qf:p1]. Bas/Milan, WDYT?

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

This should be measured on a Windows machine where the GPU process is enabled.

Comment 3

[Milan Sreckovic [:milan] (needinfo for best results)]

Let's answer David's question; can we tie this to a release criteria, or is it a "regular" qf:p1 because of new tab?

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

The extra content process being created is unintentional, and we're working with e10s folks to make it go away.  I suspect that the strategies currently being bandied about for doing that (bug 1372664 comment 7) would make this notification go away, though I'm not sure about that.  Needinfo to :gabor; :gabor can you confirm or deny my suspicion?

Comment 5

[Gabor Krizsanits (INACTIVE)]

(In reply to Dan Mosedale (:dmose) from comment #4)
> The extra content process being created is unintentional, and we're working
> with e10s folks to make it go away.  I suspect that the strategies currently
> being bandied about for doing that (bug 1372664 comment 7) would make this
> notification go away, though I'm not sure about that.  Needinfo to :gabor;
> :gabor can you confirm or deny my suspicion?

Sorry for the lag I should have answered this earlier, yes that is correct.

Comment 6

[Milan Sreckovic [:milan] (needinfo for best results)]

Let's check then when bug 1372664 gets resolved.

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

After lots of back-and-forth about how we're hoping to ship Activity Stream in 56, the code is now in what I _think_ should be its final state for 56: as in the initial comment, much of AS will now be running in a content process rather than the main process. Somewhat related, :gabor has turned off the preallocated content process for now, and as I understand it, that will stay off for 56. 

I expect that the behavior that mstange reported initially is still an issue with the following caveats:

> If you enable Activity Stream using the pref
> browser.newtabpage.activity-stream.enabled and open new tabs, the parent
> process sends a synchronous IPC messsage "NotifyChildCreated" to the
> compositor thread. This slows down tab opening.

This is likely to be most noticable on the first new tab opened in the first browser window, as that one will not be preloaded.  
This is particularly true if a new tab is opened before the maximum number of content processes have been instantiated (i.e. clicking on four links in the home page, and then hitting new tab should just cause the new tab to run in an existing content process).

> We preload "new tab" tabs, so you may need to open more than one new tab
> before the issue becomes apparent.

Note that the first "new tab" in every window will not be preloaded.

:milan, :bas, what's the appropriate next step?

Comment 8

[Florian Quèze [:florian]]

(In reply to Dan Mosedale (:dmose) from comment #7)

> Note that the first "new tab" in every window will not be preloaded.

I think this is meant to change with bug 1353013.

Comment 9

[Bas Schouten (:bas.schouten)]

I'm not sure what the question here is? Is there evidence this is still a problem on Windows? If so do we have a profile of that? I don't see it showing up on a quick glance.

This message is needed, and I believe it has to be synchronous, although I'm no expert on this code. (Is there a way it could be asynchronous David?)

We don't seem to be doing a shocking amount of work inside of it, so without a profile it's hard to tell why it would be slow.

Comment 10

[David Anderson [:dvander] - inactive, e-mail if emergency]

Indeed it must be synchronous so the compositor can associate the tab with the correct window. The message itself won't cause any performance problems, but it's possible that a previous slow message could cause the sync message to block longer than we want. It's (probably) possible to make it asynchronous but it'd require some kind of significant refactoring of the way TabChild::InitRendering works.

So, we'd want to see evidence of this message impacting Windows responsiveness.

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

Sounds like we can come back to this at a later date, if and when it starts showing up in a meaningful way in profiles.

Comment 12

[Jean Gong :jgong]

Ok. Based on Dan's comment 11. I will move this bug to QF:p2 for post 57.

Comment 13

[Florian Quèze [:florian]]

Here is a profile where it contributes at least 500ms of jank on a fast macbook when opening a couple new tabs: https://perfht.ml/2wIAYKF

Comment 14

[Bas Schouten (:bas.schouten)]

Jeff, this appears to be a real issue on OS X, this seems to be a -lot- of uploading textures. Have you seen this issue before? Is there anything we can do here to improve the situation? I don't seem to see this on Windows based on a quick glance.

Comment 15

[Kartikaya Gupta (email:[e-mail])]

Bug 1265824 should help with texture upload on macOS, we should see if this is still a problem once that's landed.

Comment 16

[Daniel Holbert [:dholbert]]

Florian, if you (or anyone) still see significant jank here (in light of changes per comment 15), please let us know so we can update priority accordingly.

For now, we're assuming there's still some avoidable dependencies between main + compositor threads here, but they're not as bad as they were previously. Hence, qf:p3:responsiveness.

Comment 17

[Markus Stange [:mstange]]

Here's a more recent profile that shows that the sync message still exists: https://perfht.ml/2RnZfUY
It is a lot shorter on macOS these days, though: 17ms to 20ms in this profile. It still contributes to a bit of jank in the parent process, but not much.

Comment 18

[Florian Quèze [:florian]]

(In reply to Daniel Holbert [:dholbert] from comment #16)

Florian, if you (or anyone) still see significant jank here (in light of changes per comment 15), please let us know so we can update priority accordingly.

I can't reproduce the problem anymore because we no longer create new content processes when opening activity stream or about:blank in new tabs, they all go in the same "privileged content" process.

Opening new tabs on my super fast macbook is still janky, but when looking at a profile (https://perfht.ml/2H8UJ89) I would blame that on us restyling several times in a row, which seems to be an entirely different bug.