835679 - Make PRenderFrame ctor async and parent->child (~55ms)

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Working from this profile

http://people.mozilla.com/~bgirard/cleopatra/#report=427594015c91a5fedfa918262af7c4767efa68c5

the SendGetDPI() call is real overhead that's delaying the child process publishing its first frame.

The PRenderFrame constructor call looks like a lot of overhead in that profile, but it's actually not unfortunately.  However, I don't think that call needs to be synchronous and removing it will show a more informative profile.  (The async version will also be slightly cheaper.)

We can piggyback the removal of GetDPI() on the new async PRenderFrame ctor.

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 1: Piggy-back DPI getter on RenderFrame ctor

Removes GetDPI call from profiles.

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 2: Refactor RenderFrame configuration into its own struct

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 3: Make RenderFrame construction async

Gets this off the profiles.

I feel pretty good about these patches, but part 1 in particular needs serious tryservering.

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 707495 [details] [diff] [review]
part 1: Piggy-back DPI getter on RenderFrame ctor

Going to be optimistic and tryserver in parallel.

Comment 5

[Nick Cameron [:nrc]]

Sorry cjones, I don't think I'm qualified to review this code - I've never touched it and I have no idea what is going on. Wish I could be more help.

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

roc volunteered you last night :).  Want to learn?  Happy to walk you through the code here.

Comment 7

[Nick Cameron [:nrc]]

Sure, more than happy to learn this stuff

Comment 8

[Nick Cameron [:nrc]]

Comment on attachment 707495 [details] [diff] [review]
part 1: Piggy-back DPI getter on RenderFrame ctor

Review of attachment 707495 [details] [diff] [review]:
-----------------------------------------------------------------

The only possible hole I see is where (previously) we create a RenderFrame when the TabParent does not have a frame element or a widget. Then the frame element is set. Then we call GetDPI. Previously this would work out fine and now we would not create the RenderFrame. I can't convince myself that this won't happen. Do you think it will not happen or do you think things will be OK if it does.

::: dom/ipc/TabParent.cpp
@@ +1122,4 @@
>  
>    nsRefPtr<nsFrameLoader> frameLoader = GetFrameLoader();
>    if (!frameLoader) {
> +    printf_stderr("Can't allocate graphics resources, aborting subprocess\n");

Why has this changed to a printf rather than an abort?

@@ +1137,5 @@
> +    nsCOMPtr<nsIDocument> doc = do_QueryInterface(ownerDoc);
> +    widget = nsContentUtils::WidgetForDocument(doc);
> +  }
> +  if (!widget) {
> +    printf_stderr("Can't find a widget, aborting subprocess\n");

This would be better as an abort too, unless there is a reason to use printfs

Comment 9

[Nick Cameron [:nrc]]

Comment on attachment 707509 [details] [diff] [review]
part 2: Refactor RenderFrame configuration into its own struct

Review of attachment 707509 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM. Much nicer to use the struct than 5 args.

Comment 10

[Nick Cameron [:nrc]]

Comment on attachment 707530 [details] [diff] [review]
part 3: Make RenderFrame construction async

Review of attachment 707530 [details] [diff] [review]:
-----------------------------------------------------------------

A few questions below, but r=me with these answered. I'm least confident in reviewing this patch - I think it is OK, but feel like I could be missing something by not knowing the context so well.

::: dom/ipc/TabChild.cpp
@@ +1920,4 @@
>  {
>      static_cast<PuppetWidget*>(mWidget.get())->InitIMEState();
>  
> +    RenderFrameConfig& config = mRenderConfig;

Why not use mRenderConfig in the below code instead of config and then drop config?

@@ +1932,4 @@
>                                                             &config.maxTextureSize());
>      } else {
>          // Pushing transactions to the parent content.
> +        shadowManager = mRemoteFrame->SendPLayersConstructor();

Is it possible mRemoteFrame is null here?

::: dom/ipc/TabParent.cpp
@@ +261,5 @@
> +      if (!GetRenderFrame()) {
> +        RenderFrameConfig config;
> +        PRenderFrameParent* frame = CreateRenderFrame(&config);
> +        if (!frame) {
> +          // XXX we probably need to start killin' now ...

Should we at least assert or abort here? Seems risky just returning.

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Thanks for the reviews! :)

(In reply to Nick Cameron [:nrc] from comment #8)
> Comment on attachment 707495 [details] [diff] [review]
> part 1: Piggy-back DPI getter on RenderFrame ctor
> 
> Review of attachment 707495 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The only possible hole I see is where (previously) we create a RenderFrame
> when the TabParent does not have a frame element or a widget. Then the frame
> element is set. Then we call GetDPI. Previously this would work out fine and
> now we would not create the RenderFrame. I can't convince myself that this
> won't happen. Do you think it will not happen or do you think things will be
> OK if it does.

I don't know yet.  Results on tryserver were not good so it's possible this change was bad.  Investigating.

> 
> ::: dom/ipc/TabParent.cpp
> @@ +1122,4 @@
> >  
> >    nsRefPtr<nsFrameLoader> frameLoader = GetFrameLoader();
> >    if (!frameLoader) {
> > +    printf_stderr("Can't allocate graphics resources, aborting subprocess\n");
> 
> Why has this changed to a printf rather than an abort?

NS_ERROR() doesn't abort, it's just the equivalent of NS_ASSERTION(false, "Msg").  If this check fails, we want to see it in logcat on gonk.  NS_ERROR() only prints in DEBUG builds.

The "aborting subprocess" part of that comes from returning nullptr from this constructor handler.  IPDL-generated code takes that as a sign that something is seriously wrong and kills the subprocess.

> @@ +1137,5 @@
> > +    nsCOMPtr<nsIDocument> doc = do_QueryInterface(ownerDoc);
> > +    widget = nsContentUtils::WidgetForDocument(doc);
> > +  }
> > +  if (!widget) {
> > +    printf_stderr("Can't find a widget, aborting subprocess\n");
> 
> This would be better as an abort too, unless there is a reason to use printfs

Same here.

Comment 12

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Local testing shows failures starting with part 2 :(.  Confirming with tryserver.

Comment 13

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Part 2 fixed; small thinko, not posting for re-review.

Comment 14

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Part 3 is failing because window.open() requires synchronous init of graphics resources, because of bug 763602.  Hmmmmm ......

Comment 15

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

FTR, I fixed part 3 a while back, but that brought back the bug where window.open() frames show up as blank white.  Interestingly no tests on tryserver failed, we should fix that ...

I think these patches have a nonnegligible risk of causing stability regressions from introducing new edge cases and that outweighs the startup gain from here atm.  So I'm putting this aside for the moment.

Comment 16

[Julien Wajsberg [:julienw]]

bug 835698 comment 54 implies we should eventually fix this here for a nice startup perf boost. So asking for at least tracking here.

Comment 17

[Lukas Blakk [:lsblakk] use ?needinfo]

Ben - can you look into this and see if there's a possibility for a low-risk perf win on v1.1 here?

Comment 18

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Hm, this is way outside my normal playpen and otherwise I'm utterly swamped at the moment. Unassigning here in the hopes that we can find someone else.

Nick, can you suggest anyone who can pick this up?

Comment 19

[Nick Cameron [:nrc]]

(In reply to ben turner [:bent] from comment #18)
> Hm, this is way outside my normal playpen and otherwise I'm utterly swamped
> at the moment. Unassigning here in the hopes that we can find someone else.
> 
> Nick, can you suggest anyone who can pick this up?

I do not sorry. Outside my scope of experience too, by a long way.

(In reply to [e-mail] from comment #17)
> Ben - can you look into this and see if there's a possibility for a low-risk
> perf win on v1.1 here?

Even if someone gets this working, I do not think it is really low risk. See comment 15 - "I think these patches have a nonnegligible risk of causing stability regressions".


Long run, I'd be happy to take a look at this, but I am swamped right now.

Comment 20

[Dietrich Ayala (:dietrich)]

Comment #16 indicates this still might be something we should do. Anyone know if that's the case, or is this b2g18-specific at this point?

Comment 22

[Thinker Li [:sinker]]

Is there any plan to revise this bug?

Comment 23

[Nick Cameron [:nrc]]

I don't know of any plans around this bug. It is probably still a good idea to do it, but I'm afraid I remember very little about it.

Comment 25

[Thinker Li [:sinker]]

bug 1107259 solves GetDPI() issue here.

Comment 26

[Thinker Li [:sinker]]

see bug 1094010 comment 14.

Comment 27

[Thinker Li [:sinker]]

Chris, would you go back to work on this bug?

Comment 28

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

No, sorry.

Comment 29

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

But I'm happy to give advice to anyone who does :).

Comment 30

[Kai-Chih Hu [:khu]]

Bobby, please help to see if we're planning to have this bug fixed in 2.2. Thanks.

Comment 31

[Ting-Yu Chou [:ting] (away)]

Fabrice has removed the sync IPC SendGetDPI() and SendGetDefaultScale() in bug 1107259. So the issue here remains is making PRenderFrame ctor async, I'll focus on part 2 and 3.

The profile cjones captured shows 52ms on SendPRenderFrameConstructor, and a recent profile (bug 1094010 comment 6) shows 27ms.

Comment 32

[Ting-Yu Chou [:ting] (away)]

PRenderFrame ctor seems is async already [1], see bug 1085655. Are there anything else we expect from this bug?

[1] http://dxr.mozilla.org/mozilla-central/source/dom/ipc/PBrowser.ipdl#102

Comment 33

[Thinker Li [:sinker]]

Ting, thanks!  I think we could close this bug.

Comment 34

[Kai-Chih Hu [:khu]]

Bobby, please make sure the decision is correct: +'ed and closed it.