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

RESOLVED FIXED in 2.2 S2 (19dec)

Status

()

RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: cjones, Unassigned)

Tracking

({perf})

unspecified
2.2 S2 (19dec)
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(feature-b2g:2.2+, b2g18+ affected)

Details

Attachments

(3 attachments)

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.
Assignee: nobody → jones.chris.g
Created attachment 707495 [details] [diff] [review]
part 1: Piggy-back DPI getter on RenderFrame ctor

Removes GetDPI call from profiles.
Created attachment 707509 [details] [diff] [review]
part 2: Refactor RenderFrame configuration into its own struct
Created attachment 707530 [details] [diff] [review]
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 on attachment 707495 [details] [diff] [review]
part 1: Piggy-back DPI getter on RenderFrame ctor

Going to be optimistic and tryserver in parallel.
Attachment #707495 - Flags: review?(ncameron)
Attachment #707509 - Flags: review?(ncameron)
Attachment #707530 - Flags: review?(ncameron)

Comment 5

6 years ago
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.
roc volunteered you last night :).  Want to learn?  Happy to walk you through the code here.

Comment 7

6 years ago
Sure, more than happy to learn this stuff

Comment 8

6 years ago
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

6 years ago
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.
Attachment #707509 - Flags: review?(ncameron) → review+

Comment 10

6 years ago
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.
Attachment #707530 - Flags: review?(ncameron) → review+
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.
Local testing shows failures starting with part 2 :(.  Confirming with tryserver.
Part 2 fixed; small thinko, not posting for re-review.
Part 3 is failing because window.open() requires synchronous init of graphics resources, because of bug 763602.  Hmmmmm ......
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.

Updated

6 years ago
Attachment #707495 - Flags: review?(ncameron)
bug 835698 comment 54 implies we should eventually fix this here for a nice startup perf boost. So asking for at least tracking here.
tracking-b2g18: --- → ?
Ben - can you look into this and see if there's a possibility for a low-risk perf win on v1.1 here?
Assignee: cjones.bugs → bent.mozilla
status-b2g18: --- → affected
tracking-b2g18: ? → +
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?
Assignee: bent.mozilla → nobody

Comment 19

6 years ago
(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 lsblakk@mozilla.com 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 #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?
Keywords: perf

Updated

4 years ago
Blocks: 1093564

Updated

4 years ago
Duplicate of this bug: 1102059
Is there any plan to revise this bug?
Flags: needinfo?(ncameron)

Updated

4 years ago
Blocks: 1074783

Updated

4 years ago
feature-b2g: --- → 2.2?

Comment 23

4 years ago
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.
Flags: needinfo?(ncameron)
Blocks: 1110590
No longer blocks: 1093564

Updated

4 years ago
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1107259

Updated

4 years ago
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
bug 1107259 solves GetDPI() issue here.

Updated

4 years ago
Summary: Make PRenderFrame ctor async and parent->child and remove synchronous GetDPI() → Make PRenderFrame ctor async and parent->child

Updated

4 years ago
Summary: Make PRenderFrame ctor async and parent->child → Make PRenderFrame ctor async and parent->child (~55ms)
Chris, would you go back to work on this bug?
Flags: needinfo?(cjones.bugs)
No, sorry.
Flags: needinfo?(cjones.bugs)
But I'm happy to give advice to anyone who does :).

Comment 30

4 years ago
Bobby, please help to see if we're planning to have this bug fixed in 2.2. Thanks.
Flags: needinfo?(bchien)
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.
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
Ting, thanks!  I think we could close this bug.

Comment 34

4 years ago
Bobby, please make sure the decision is correct: +'ed and closed it.
Status: REOPENED → RESOLVED
feature-b2g: 2.2? → 2.2+
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED

Updated

4 years ago
Flags: needinfo?(bchien)

Updated

4 years ago
Target Milestone: --- → 2.2 S2 (19dec)
You need to log in before you can comment on or make changes to this bug.