1051852 - [Messages] use 'DOMContentLoaded' instead of 'load' to init the various resources

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::SMS, defect)

Description

[Julien Wajsberg [:julienw] (PTO -> Jul 24)]

I noticed that using 'DOMContentLoaded' brings a nice boost at startup without any regression.


[Blocking Requested - why for this release]: blocks  blocker.

Comment 1

[Julien Wajsberg [:julienw] (PTO -> Jul 24)]

Attachment: github PR

Comment 2

[Julien Wajsberg [:julienw] (PTO -> Jul 24)]

Attachment: 2014-08-11-173948.webm

Comment 3

[Julien Wajsberg [:julienw] (PTO -> Jul 24)]

On the video we clearly see that the "load" event happens later with the patch (which is expected because DB and DOM work competes with other things Gecko does at startup) but the threads appears much earlier.

Comment 4

[bhavana bajaj [:bajaj]]

Julien, can CAF pick this up for their testrun today ? Wanted to make sure that's ok and also ensure this patch would apply to the 2.0 code?

Comment 5

[Julien Wajsberg [:julienw] (PTO -> Jul 24)]

Yeah this piece of code has not changed since we banched, should be fine.

Comment 6

[Julien Wajsberg [:julienw] (PTO -> Jul 24)]

*branched

Comment 7

[Steve Chung [:steveck]]

Comment on attachment 8470868 [details] [review]
github PR

The description on MDN seems DOMContentLoaded should be faster than load event, but using DOMContentLoaded will become longer load time on my flame/buri with latest master build :-/  It will have 50ms delay on flame and even 250ms delay on buri. Would it related to version of gaia/gecko?

Comment 8

[Steve Chung [:steveck]]

(In reply to Steve Chung [:steveck] from comment #7)
> Comment on attachment 8470868 [details] [review]
> github PR
> 
> The description on MDN seems DOMContentLoaded should be faster than load
> event, but using DOMContentLoaded will become longer load time on my
> flame/buri with latest master build :-/  It will have 50ms delay on flame
> and even 250ms delay on buri. Would it related to version of gaia/gecko?

After side by side comparison, using DOMContentLoaded actually display faster than using load event, so it looks we did start up before load event and it will postpone load event. BTW, I can barely point out the difference on flame comparison.

Comment 9

[Steve Chung [:steveck]]

Comment on attachment 8470868 [details] [review]
github PR

I've tried for a while and it seems faster than original version, and no additional reflow(I'm afraid that it might lead reflow because we delay the image/iframe loading, but it seems safe anyway). Maybe we need to refine the app load time measuring someday.

Comment 10

[Julien Wajsberg [:julienw] (PTO -> Jul 24)]

(In reply to Steve Chung [:steveck] from comment #7)
> Comment on attachment 8470868 [details] [review]
> github PR
> 
> The description on MDN seems DOMContentLoaded should be faster than load
> event, but using DOMContentLoaded will become longer load time on my
> flame/buri with latest master build :-/  It will have 50ms delay on flame
> and even 250ms delay on buri. Would it related to version of gaia/gecko?

We see it on my video: the "load" event happens later than without the patch (it's when the splashscreen is removed), however on the video we clearly see that the threads are appearing earlier.

I think the "load" event happens later because we're doing some things (like DB, DOM operations) while Gecko is also doing things before the load event (loading CSS especially). But this is IMO good that we try to display threads and things before we have CSS, so that we have less and less expensive reflows at the start.

Something interesting: we see that some threads are _already_ displayed when the chrome is showing. This is really nice when we'll have your patch as well because we'll see that the app is already loaded when it shows up!


As for Flame vs Buri... this gives some weight to my argument that Flames are too fast to study performance :/ (and our profiler and other tools do not help us at all here).



Thanks !

Comment 11

[Julien Wajsberg [:julienw] (PTO -> Jul 24)]

(In reply to Steve Chung [:steveck] from comment #9)
> Comment on attachment 8470868 [details] [review]
> github PR
> 
> I've tried for a while and it seems faster than original version, and no
> additional reflow(I'm afraid that it might lead reflow because we delay the
> image/iframe loading, but it seems safe anyway). Maybe we need to refine the
> app load time measuring someday.

This is what the perf team is doing, with the additional measurements they do. Clearly, measuring the "load" event is wrong, and that's exactly what led to the fact we use the 'load' event instead of 'DOMContentLoaded' even if it was faster.

Comment 12

[Julien Wajsberg [:julienw] (PTO -> Jul 24)]

master: 02f778766521b84da0ca3e575bfe35c37f46803d

Comment 13

[Dietrich Ayala (:dietrich)]

Julien, double-checking the level of risk here for triage - this change affects the order of execution of the app. Is there anything timing-dependent that might be impacted?

Comment 14

[Julien Wajsberg [:julienw] (PTO -> Jul 24)]

Yes there is a potential race with the l10n lib loading its data. Now, I've been testing with slow Buri and I didn't see the issue, but it does not mean it's not here. I also haven't tested with v2.0.

The effect of the race would be that no thread is displayed, so it's something that would be highly visible in QA tests.

Comment 15

[Preeti Raghunath(:Preeti)]

Requesting QA input prior to uplift.

Comment 16

[Jason Smith [:jsmith]]

(In reply to Preeti Raghunath(:Preeti) from comment #15)
> Requesting QA input prior to uplift.

Mentioned in vidyo - figured out how to set this against the qa-approval flag, so switching this to the qa-approval flag.

Comment 17

[Julien Wajsberg [:julienw] (PTO -> Jul 24)]

I got the issue when I tried the current SMS app on v1.3 (which is faster) so it's a good idea to fix the race. Even if I didn't get it on the current master, we could have it if Gecko is made quicker (and I hope we'll be able to do it).

Oleg, can you have a look at this tomorrow? Please file a separate bug for this.

2 possible solutions IMO:

* in renderThreads, replace "appendThread" call by "navigator.mozL10n.once(this.appendThread.bind(this, thread))". The problem is that "once" calls "setTimeout" if the context is ready, which may give some delay to render the first panel, and we want to avoid this. But you can try this first and check if it's visually slower (ideally on Buri).

* the other solution is, in appendThread, add at the start:

  if (navigator.mozL10n.readyState !== 'complete') {
    navigator.mozL10n.once(this.appendThread.bind(this, thread));
    return;
  }

This eliminates the setTimeout if we're ready.

Last solution is to avoid needing the l10n lib at this time (we need it because of Utils' date/time utility functions), because I _think_ we handle these items in the mozL10n's ready callback in startup.js anyway.

Thanks !

Comment 18

[Oleg Zasypkin [:azasypkin]]

(In reply to Julien Wajsberg [:julienw] from comment #17)
> I got the issue when I tried the current SMS app on v1.3 (which is faster)
> so it's a good idea to fix the race. Even if I didn't get it on the current
> master, we could have it if Gecko is made quicker (and I hope we'll be able
> to do it).
> 
> Oleg, can you have a look at this tomorrow? Please file a separate bug for
> this.
> 
Sure, filed a bug 1054004 and assigned it to myself. Will look into it tomorrow.

Comment 19

[bhavana bajaj [:bajaj]]

julien/steve, do we know how much the numbers improved here given we have a fallout in 1054004, do you think its worth the risk and might help the original bug(1038176) as much ?

Comment 20

[Julien Wajsberg [:julienw] (PTO -> Jul 24)]

Yes it really helps a lot

Comment 21

[bhavana bajaj [:bajaj]]

(In reply to Julien Wajsberg [:julienw] (PTO 08/20 -> 09/15; contact schung instead) from comment #20)
> Yes it really helps a lot

uplift this after 1054004 is resolved, correct ?

Comment 22

[Eric Chang [:ericcc] [:echang]]

(In reply to Jason Smith [:jsmith] from comment #16)
> (In reply to Preeti Raghunath(:Preeti) from comment #15)
> > Requesting QA input prior to uplift.
> 
> Mentioned in vidyo - figured out how to set this against the qa-approval
> flag, so switching this to the qa-approval flag.
I run a small set against v2.0+patch, looks fine, not sure how to cancel this qa-approval flag.
https://moztrap.mozilla.org/runtests/run/5031/env/27835/

Comment 23

[Jason Smith [:jsmith]]

(In reply to Eric Chang [:ericcc] [:echang] from comment #22)
> (In reply to Jason Smith [:jsmith] from comment #16)
> > (In reply to Preeti Raghunath(:Preeti) from comment #15)
> > > Requesting QA input prior to uplift.
> > 
> > Mentioned in vidyo - figured out how to set this against the qa-approval
> > flag, so switching this to the qa-approval flag.
> I run a small set against v2.0+patch, looks fine, not sure how to cancel
> this qa-approval flag.
> https://moztrap.mozilla.org/runtests/run/5031/env/27835/

If you go into the details of the patch here - https://bugzilla.mozilla.org/attachment.cgi?id=8470868&action=edit, you can select + under qa-approval to state that you have signed off on an uplift of this patch.

Comment 24

[Julien Wajsberg [:julienw] (PTO -> Jul 24)]

(In reply to bhavana bajaj [:bajaj] from comment #21)
> (In reply to Julien Wajsberg [:julienw] (PTO 08/20 -> 09/15; contact schung
> instead) from comment #20)
> > Yes it really helps a lot
> 
> uplift this after 1054004 is resolved, correct ?

It would be better, yes :)

Comment 25

[bhavana bajaj [:bajaj]]

lets uplift this only after the race in 1054004 is fixed.

Comment 26

[Oleg Zasypkin [:azasypkin]]

Removing NO_UPLIFT, since bug 1054004 has landed on master and v2.0

Comment 27

[Ryan VanderMeulen [:RyanVM]]

v2.0: https://github.com/mozilla-b2g/gaia/commit/4c8b5ced1966079086d86dec3098ecf340881306

Comment 28

[Eric Chang [:ericcc] [:echang]]

Verified okay
https://moztrap.mozilla.org/runtests/environment/5547/

Gaia-Rev        092d2b7678774c8b0b06dca0e0a8119e9eafdec3
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/69ca61f7edf3
Build-ID        20141005160203
Version         32.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141005.194303
FW-Date         Sun Oct  5 19:43:14 EDT 2014
Bootloader      L1TC00011840

Comment 29

[SandKing]

From Comment28 change the Tracking Flags