Mike Conley (:mconley) (:⚙️) Assignee
	Description • 8 years ago

When opening new windows from content (via window.open, for example), the CreateWindow message sent by ContentChild[1] is immediately followed by a second sync message to get the RenderFrameInfo from the parent.

This second sync messages regresses our tpaint tests on multiple platforms. On Windows, for example, I have a profile here[3] that shows us spending ~50ms waiting on the result of this sync message. The parent, as far as I can tell, doesn't need to do anything too intensive to return the RenderFrameInfo - it's just that the parent is off doing other times when the child asks for it, and it's blocked.

So if we could combine the two sync messages, I think that'd help here.

[1]: https://dxr.mozilla.org/mozilla-central/rev/d848a5628d801a460a7244cbcdea22d328d8b310/dom/ipc/ContentChild.cpp#884
[2]: https://dxr.mozilla.org/mozilla-central/rev/d848a5628d801a460a7244cbcdea22d328d8b310/dom/ipc/ContentChild.cpp#911
[3]: http://people.mozilla.org/~bgirard/cleopatra/?zippedProfile=http://mozilla-releng-blobs.s3.amazonaws.com/blobs/Try/sha512/6c9ae00721db9ecad047d736bb9484422b3ac8ab6a42ab0ddf8aba98c90f53d506a27d624a8cec55e3387f61ff2c77424eab58eb58f8a585131ab30831fdabf3#report=de6ea8e7ef2b36b67bb0821b7e3cd355f69289c5&filter=%5B%7B%22type%22%3A%22RangeSampleFilter%22,%22start%22%3A4391,%22end%22%3A4992%7D%5D&selection=0,6937,6,7,6938,6939,4,5,6,7,6938,8,9,10,1831,1832,1833,1834,1835,1836,1837,897,26,27,28,6945,29,30,27,2044,6946,6947,6948,6949,6950,51,52,6951,6952,6957,6958,1589,1590,1591,1592,1593,1594,1595,1596,46

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 1 • 8 years ago

One thing I'm thinking here is that we might be optimizing the wrong path. We only take this path when window.open is called. That's not terribly common compared to the user opening a new tab. I feel like that's what we should be measuring (and optimizing if need be).

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 2 • 8 years ago

(In reply to Bill McCloskey (:billm) from comment #1)
> One thing I'm thinking here is that we might be optimizing the wrong path.
> We only take this path when window.open is called. That's not terribly
> common compared to the user opening a new tab. I feel like that's what we
> should be measuring (and optimizing if need be).

Hm. Are you saying that we should care less about tpaint, and we should design a new test that measures how long it takes to open new tabs from something like Ctrl-T or hitting the New Tab button?

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 3 • 8 years ago

Yes.

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 4 • 8 years ago

Here's a way to do this:

1. Send up a new RenderFrame before the CreateWindow call. We won't have a frameloader yet, so the AllocPRenderFrame code won't really do anything.
2. Pass the new PRenderFrame to CreateWindow. CreateWindow will fill in the RenderFrameParent with everything it normally gets from frameloader.
3. CreateWindow returns any other information needed, like the texture factory identifier and layer tree ID.

This is kinda hacky, but at least it doesn't rely on weird IPC gymnastics.

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 5 • 8 years ago

So I did a hack to make this work as a PoC, and I saw very little (if any) improvement in our tpaint scores. Upon profiling, it became apparent why: yes, we were no longer blocking the content while it waits for the RenderFrameInfo. Instead, we were blocking content while it waits for the sync Browser:Init message that browser-child.js sends up.

We should definitely eliminate as many sync messages as we can, but fixing this bug alone is not going to impact our tpaint score.

I think bigger wins are likely to be gotten by freeing up the parent to respond to these sync messages as soon as possible.

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 6 • 8 years ago

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

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 7 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=36f3b9d5b2bb

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 8 • 8 years ago

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

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 9 • 8 years ago

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

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 10 • 8 years ago

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

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 11 • 8 years ago

Hey kanru - smaug said you might have time to review this. Let me know if you need me to redirect. Thanks!

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 12 • 8 years ago

Comment on attachment 8735980 [details]
MozReview Request: Bug 1251032 - Make it possible to assign a frameloader to RenderFrameParent after construction. r?kanru

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

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 13 • 8 years ago

Comment on attachment 8735981 [details]
MozReview Request: Bug 1251032 - Send RenderFrame info down to child in CreateWindow message. r?kanru

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

	Kan-Ru Chen [:kanru] (UTC+9)
	Comment 14 • 8 years ago

Comment on attachment 8735979 [details]
MozReview Request: Bug 1251032 - Don't return layersId or textureFactoryIdentifier as outparams in RenderFrameParent constructor. r?kanru

https://reviewboard.mozilla.org/r/43027/#review39719

::: dom/ipc/TabParent.cpp:897
(Diff revision 1)
> +          layersId = renderFrame->GetLayersId();
> +          renderFrame->GetTextureFactoryIdentifier(&textureFactoryIdentifier);

Hmm, could you create a followup to change GetTextureFactoryIdentifier to also return value directly?

	Kan-Ru Chen [:kanru] (UTC+9)
	Comment 15 • 8 years ago

Comment on attachment 8735980 [details]
MozReview Request: Bug 1251032 - Make it possible to assign a frameloader to RenderFrameParent after construction. r?kanru

https://reviewboard.mozilla.org/r/43029/#review39721

	Kan-Ru Chen [:kanru] (UTC+9)
	Comment 16 • 8 years ago

Comment on attachment 8735981 [details]
MozReview Request: Bug 1251032 - Send RenderFrame info down to child in CreateWindow message. r?kanru

https://reviewboard.mozilla.org/r/43031/#review39723

::: dom/ipc/ContentChild.cpp:907
(Diff revision 2)
>                            aWindowIsNew,
>                            &frameScripts,
> -                          &urlToLoad)) {
> +                          &urlToLoad,
> +                          &textureFactoryIdentifier,
> +                          &layersId)) {
> +      PRenderFrameChild::Send__delete__(renderFrame);

I feel we call this Send__delete__() method manually way too many times. Maybe a good idea to use a RAII class.

::: dom/ipc/TabParent.cpp:2599
(Diff revision 2)
> +  uint64_t layersId = 0;
> +  layersId = renderFrame->GetLayersId();

nit: combine this to one line.

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 17 • 8 years ago

https://reviewboard.mozilla.org/r/43027/#review39719

> Hmm, could you create a followup to change GetTextureFactoryIdentifier to also return value directly?

Thanks - filed bug 1260771 for this.

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 18 • 8 years ago

https://reviewboard.mozilla.org/r/43031/#review39723

> I feel we call this Send__delete__() method manually way too many times. Maybe a good idea to use a RAII class.

Good idea. I've filed bug 1260774 for getting an RAII class put together to automate deleting IPC Actors that don't meet certain conditions.

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 19 • 8 years ago

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

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 20 • 8 years ago

Comment on attachment 8735981 [details]
MozReview Request: Bug 1251032 - Send RenderFrame info down to child in CreateWindow message. r?kanru

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

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 21 • 8 years ago

Comment on attachment 8736574 [details]
MozReview Request: Bug 1251032 - Send RenderFrame info down to child in BrowserFrameOpenWindow. r?kanru

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

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 22 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6427ac294316

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 23 • 8 years ago

Gentle review ping?

	Kan-Ru Chen [:kanru] (UTC+9)
	Comment 24 • 8 years ago

Comment on attachment 8736574 [details]
MozReview Request: Bug 1251032 - Send RenderFrame info down to child in BrowserFrameOpenWindow. r?kanru

https://reviewboard.mozilla.org/r/43401/#review41245

	Pulsebot
	Comment 25 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/797ae7f4a0b1
https://hg.mozilla.org/integration/mozilla-inbound/rev/736edb6b8924
https://hg.mozilla.org/integration/mozilla-inbound/rev/31f191d97089
https://hg.mozilla.org/integration/mozilla-inbound/rev/5acaba393ffe

	Joel Maher ( :jmaher ) (UTC -8)
	Comment 26 • 8 years ago

a 10% tabpaint linux64 e10s improvement with this!!
https://treeherder.mozilla.org/perf.html#/alerts?id=756

	Carsten Book [:Tomcat]
	Comment 27 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/797ae7f4a0b1
https://hg.mozilla.org/mozilla-central/rev/736edb6b8924
https://hg.mozilla.org/mozilla-central/rev/31f191d97089
https://hg.mozilla.org/mozilla-central/rev/5acaba393ffe

	Joel Maher ( :jmaher ) (UTC -8)
	Comment 28 • 8 years ago

sorry, not tabpaint, but tpaint (also osx)

	Chris Peterson [:cpeterson]
	Comment 29 • 8 years ago

Mike, should this fix be uplifted to Aurora 47 in preparation for our e10s experiments on Beta 47? Or does this only affect tpaint and not real world usage?

	Chris Peterson [:cpeterson]
	Comment 30 • 8 years ago

Verified by Joel in comment 26.

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 31 • 8 years ago

(In reply to Chris Peterson [:cpeterson] from comment #29)
> Mike, should this fix be uplifted to Aurora 47 in preparation for our e10s
> experiments on Beta 47? Or does this only affect tpaint and not real world
> usage?

I don't think so, no. I think this is a pretty invasive patch, and imo, it should ride the trains.

	Chris Peterson [:cpeterson]
	Comment 32 • 8 years ago

Thanks. status-firefox47=wontfix

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 33 • 8 years ago
bugnotes

http://people.mozilla.org/~mconley2/bugnotes/bug-1251032.html