Closed Bug 926452 Opened 7 years ago Closed 2 years ago

The call to getComputedStyle in the onStateChange progress listener of BrowserElementChildPreload.js is expensive (~30ms)

Categories

(Core :: DOM: Core & HTML, defect, P3)

defect

Tracking

()

RESOLVED WONTFIX
mozilla44
Tracking Status
firefox44 --- affected
b2g-v2.5 --- affected
b2g-master --- affected

People

(Reporter: vingtetun, Assigned: kanru)

References

(Blocks 1 open bug)

Details

(Keywords: perf, Whiteboard: [perf-wanted][Profile-wanted])

Attachments

(3 files)

Expensive is relative here since it tooks a few ms, but this hurts page start when this stuff is only really useful for a case where the application is in landscape mode and takes time to load.

It worth trying to move it out of the startup path.
Keywords: perf
Yep, I timed that again on a Tarako. It's pretty terrible, ranging from a few ms to sometimes as long as 25/30ms
Is there a way to eagerly check or pass in orientation to avoid doing this every time?
If I understand apps/system/js/window_manager.js:340 correctly we could delay that a bit with no harm (unless the user is rotating its phone like crazy...).
[Tracking Requested - why for this release]:
Assignee: nobody → kchen
Whiteboard: [perf-wanted][Profile-wanted]
Based on Wilson's test results, he says this is costs around 30ms. Though it is not a huge win, small costs are adding up. 

Thinker/Bobby: could we get an assessment on what it would take to address this bug in 2.5? All small improvements will make a big difference. 

Profile info on bug 1164539
Flags: needinfo?(tlee)
Flags: needinfo?(bchien)
If the style is always calculated before first paint, will this still be helpful for bug 1164539?

Will talk to Kanru after he's back from PTO.
Attached image before.png
Attached image after.png
AS mentioned earlier in this bug, getComputedStyle() is called on every app's launch path to determine the background-color. This background-color is then set on an element in the system app that contains the mozbrowser <iframe>.

This is done so that when devices are rotated and apps take some time to reflow we can show a better color than white in the empty space in the period before the app has reflowed to fill.

If this is the only use-case, it seems we could delay this work till after the launch path. I don't know enough about mozbrowser to suggest when would be a better time. I think Dale knows more about this. Dale any guidance here?
Flags: needinfo?(dale)
(In reply to Wilson Page [:wilsonpage] from comment #7)
> Profile with c++ stack traces:
> https://people.mozilla.org/~bgirard/cleopatra/
> #report=5c719c423891c6a2e7c493947f631741fed25f64

It's GetOrCreateDOMReflector() of getComputedStyle() taking time. BTW, I don't see this bug in normal application launch.
IIRC Vivien said this code isn't actual required any more and could be stripped. NI to clarify and maybe patch ;)
Flags: needinfo?(21)
Blocks: AppStartup
Flags: needinfo?(bchien)
Priority: -- → P2
Summary: The call to getComputedStyle in the onStateChange progress listener of BrowserElementChildPreload.js is expensive → The call to getComputedStyle in the onStateChange progress listener of BrowserElementChildPreload.js is expensive (~30ms)
(In reply to Wilson Page [:wilsonpage] from comment #13)
> IIRC Vivien said this code isn't actual required any more and could be
> stripped. NI to clarify and maybe patch ;)

Remove the code will see white screen.

In bug 869903, there was a discussion which to set the default backgrdound color black, also :mwu tried to set the background color at mozbrowserfirstpaint but it failed some tests.
My plan is to remove the code from bug 869903 and fix the white screen properly.

The background color of browser iframe was black, but then change to white [1], not sure about the reason. Revert it [2] back to #fff and remove the getComputedStyle() will not see bug 869903.

But we need some background for the code of bug 1047829 [3], Vivien?

[1] https://github.com/mozilla-b2g/gaia/commit/9e8e6d5318c31d75fc3fbeaaa0dab44297213fd8
[2] https://github.com/mozilla-b2g/gaia/blob/master/apps/system/style/window.css#L66
[3] https://github.com/mozilla-b2g/gaia/commit/2e20dfb616a8ec8f01a7000dd6bdb32eb2b10dc1#diff-46b349b14289333755febebd351e16ccL35
(In reply to Ting-Yu Chou [:ting] from comment #15)
> The background color of browser iframe was black, but then change to white
> [1], not sure about the reason.
> [1]
> https://github.com/mozilla-b2g/gaia/commit/
> 9e8e6d5318c31d75fc3fbeaaa0dab44297213fd8

Ben, do you remember the reason of this change?
Flags: needinfo?(bfrancis)
(In reply to Ting-Yu Chou [:ting] from comment #15)
> The background color of browser iframe was black, but then change to white
> [1], not sure about the reason. Revert it [2] back to #fff and remove the
> getComputedStyle() will not see bug 869903.

Correction: Revert it [2] back to #000.
iirc, we need a white background (and black text color) by default for pages that have no styling to display properly.
(In reply to [:fabrice] Fabrice Desré from comment #18)
> iirc, we need a white background (and black text color) by default for pages
> that have no styling to display properly.

Then maybe some code has been changed, otherwise this should be broken once orientationchange is fired as it changes browser iframe's background color. I'll double check, thanks for the info :)
(In reply to Ting-Yu Chou [:ting] from comment #19)
> (In reply to [:fabrice] Fabrice Desré from comment #18)
> > iirc, we need a white background (and black text color) by default for pages
> > that have no styling to display properly.
> 
> Then maybe some code has been changed, otherwise this should be broken once
> orientationchange is fired as it changes browser iframe's background color.
> I'll double check, thanks for the info :)

I meant mozbrowserloadend.
Double checked, Fabrice (comment 18) is right, set browser iframe background color to black will break the page without styling.
Flags: needinfo?(bfrancis)
Clearing as it looks like this has ownership and details now, cheers
Flags: needinfo?(dale)
From LayerScope, I found the RefLayerComposite of content has rotated orientation, but its reference layer (ContainerLayerComposite) is still unrotated.
(In reply to Ting-Yu Chou [:ting] from comment #23)
> From LayerScope, I found the RefLayerComposite of content has rotated
> orientation, but its reference layer (ContainerLayerComposite) is still
> unrotated.

I've talked to :mtseng whether in this case to fill remaining area black make sense or not, it sounds ok, but there're some other cases could have similar situation, e.g., 3d transform.
(In reply to Ting-Yu Chou [:ting] from comment #24)
> (In reply to Ting-Yu Chou [:ting] from comment #23)
> > From LayerScope, I found the RefLayerComposite of content has rotated
> > orientation, but its reference layer (ContainerLayerComposite) is still
> > unrotated.
> 
> I've talked to :mtseng whether in this case to fill remaining area black
> make sense or not, it sounds ok, but there're some other cases could have
> similar situation, e.g., 3d transform.

I think the code can be in RefLayerComposite::RenderLayer(), but I guess it's better to check a flag that they're in different orientation instead of comparing the rect/region everytime.

Bas, I am not familiar with the code, could you please suggest how should I proceed? Thank you.
Flags: needinfo?(bas)
Once support for background_color will land (in bug 1215077), the code that reads the background color of the page can just be removed. There will be a background-color derived from the theme_color, or from background_color if present and support is enabled.
Flags: needinfo?(21)
Attachment #8674233 - Flags: review?(fabrice)
Depends on: 1215077
(In reply to vnicolas from comment #26)
> Created attachment 8674233 [details] [diff] [review]
> stop.sending.bgcolor.on.loadend.patch
> 
> Once support for background_color will land (in bug 1215077), the code that
> reads the background color of the page can just be removed. There will be a
> background-color derived from the theme_color, or from background_color if
> present and support is enabled.

What will be the consequences for pages that have no manifest to get a background color from?
Flags: needinfo?(21)
(In reply to [:fabrice] Fabrice Desré from comment #27)
> (In reply to vnicolas from comment #26)
> > Created attachment 8674233 [details] [diff] [review]
> > stop.sending.bgcolor.on.loadend.patch
> > 
> > Once support for background_color will land (in bug 1215077), the code that
> > reads the background color of the page can just be removed. There will be a
> > background-color derived from the theme_color, or from background_color if
> > present and support is enabled.
> 
> What will be the consequences for pages that have no manifest to get a
> background color from?

They can always use a <meta name="theme-color" content="..."> instruction to control the background-color of this part of the UI. It is not perfect but more flexible than the one shot default background-color.
Attachment #8674233 - Flags: review?(fabrice) → review+
https://hg.mozilla.org/mozilla-central/rev/b0359c684793
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Clear NI to bas as this is resolved through another route.
Flags: needinfo?(bas)
Depends on: 1213051
This bug caused bug 1213051. That bug is a 2.5+ blocker.  Vivien agrees that we should back this patch out until 1215077 lands. See https://bugzilla.mozilla.org/show_bug.cgi?id=1213051#c17
Flags: needinfo?(kchen)
Setting NI flag for Farbrice to help with backout. Thanks
Flags: needinfo?(kchen) → needinfo?(fabrice)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
c
Flags: needinfo?(tlee)
Flags: needinfo?(21)
Moving to p3 because no activity for at least 1 year(s).
See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage for more information
Priority: P2 → P3

Closing bugs with b2g-master=affected as it is likely to be out dated.

Status: REOPENED → RESOLVED
Closed: 5 years ago2 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.