Closed
Bug 835679
Opened 12 years ago
Closed 10 years ago
Make PRenderFrame ctor async and parent->child (~55ms)
Categories
(Core :: General, defect)
Tracking
()
People
(Reporter: cjones, Unassigned)
References
Details
(Keywords: perf)
Attachments
(3 files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
nrc
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
nrc
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•12 years ago
|
Assignee: nobody → jones.chris.g
Reporter | ||
Comment 1•12 years ago
|
||
Removes GetDPI call from profiles.
Reporter | ||
Comment 2•12 years ago
|
||
Reporter | ||
Comment 3•12 years ago
|
||
Gets this off the profiles.
I feel pretty good about these patches, but part 1 in particular needs serious tryservering.
Reporter | ||
Comment 4•12 years ago
|
||
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)
Reporter | ||
Updated•12 years ago
|
Attachment #707509 -
Flags: review?(ncameron)
Reporter | ||
Updated•12 years ago
|
Attachment #707530 -
Flags: review?(ncameron)
Comment 5•12 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.
Reporter | ||
Comment 6•12 years ago
|
||
roc volunteered you last night :). Want to learn? Happy to walk you through the code here.
Comment 7•12 years ago
|
||
Sure, more than happy to learn this stuff
Comment 8•12 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•12 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•12 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+
Reporter | ||
Comment 11•12 years ago
|
||
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.
Reporter | ||
Comment 12•12 years ago
|
||
Local testing shows failures starting with part 2 :(. Confirming with tryserver.
Reporter | ||
Comment 13•12 years ago
|
||
Part 2 fixed; small thinko, not posting for re-review.
Reporter | ||
Comment 14•12 years ago
|
||
Part 3 is failing because window.open() requires synchronous init of graphics resources, because of bug 763602. Hmmmmm ......
Reporter | ||
Comment 15•12 years ago
|
||
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•12 years ago
|
Attachment #707495 -
Flags: review?(ncameron)
Comment 16•12 years ago
|
||
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:
--- → ?
Comment 17•12 years ago
|
||
Ben - can you look into this and see if there's a possibility for a low-risk perf win on v1.1 here?
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•12 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 20•11 years ago
|
||
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•10 years ago
|
Blocks: B2G-Multicore
Updated•10 years ago
|
feature-b2g: --- → 2.2?
Comment 23•10 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)
Updated•10 years ago
|
Blocks: AppStartup
Updated•10 years ago
|
No longer blocks: B2G-Multicore
Updated•10 years ago
|
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → DUPLICATE
Updated•10 years ago
|
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Comment 25•10 years ago
|
||
bug 1107259 solves GetDPI() issue here.
Updated•10 years ago
|
Updated•10 years ago
|
Summary: Make PRenderFrame ctor async and parent->child and remove synchronous GetDPI() → Make PRenderFrame ctor async and parent->child
Comment 26•10 years ago
|
||
Updated•10 years ago
|
Summary: Make PRenderFrame ctor async and parent->child → Make PRenderFrame ctor async and parent->child (~55ms)
Comment 27•10 years ago
|
||
Chris, would you go back to work on this bug?
Flags: needinfo?(cjones.bugs)
Reporter | ||
Comment 29•10 years ago
|
||
But I'm happy to give advice to anyone who does :).
Comment 30•10 years ago
|
||
Bobby, please help to see if we're planning to have this bug fixed in 2.2. Thanks.
Flags: needinfo?(bchien)
Comment 31•10 years ago
|
||
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•10 years ago
|
||
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•10 years ago
|
||
Ting, thanks! I think we could close this bug.
Comment 34•10 years ago
|
||
Bobby, please make sure the decision is correct: +'ed and closed it.
Status: REOPENED → RESOLVED
feature-b2g: 2.2? → 2.2+
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
Updated•10 years ago
|
Flags: needinfo?(bchien)
Updated•10 years ago
|
Target Milestone: --- → 2.2 S2 (19dec)
You need to log in
before you can comment on or make changes to this bug.
Description
•