Closed Bug 1051852 Opened 6 years ago Closed 6 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:2.0+, b2g-v2.0 verified, b2g-v2.1 fixed)

VERIFIED FIXED
2.1 S2 (15aug)
blocking-b2g 2.0+
Tracking Status
b2g-v2.0 --- verified
b2g-v2.1 --- fixed

People

(Reporter: julienw, Assigned: julienw)

References

Details

(Whiteboard: [sms-sprint-2.1S2])

Attachments

(2 files)

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


[Blocking Requested - why for this release]: blocks  blocker.
Assignee: nobody → felash
Attached file github PR
Attachment #8470868 - Flags: review?(schung)
Whiteboard: [sms-sprint-2.1S2]
Attached video 2014-08-11-173948.webm
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.
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?
Flags: needinfo?(felash)
Yeah this piece of code has not changed since we banched, should be fine.
Flags: needinfo?(felash)
*branched
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?
Attachment #8470868 - Flags: review?(schung)
(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 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.
Attachment #8470868 - Flags: review+
(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 !
(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.
master: 02f778766521b84da0ca3e575bfe35c37f46803d
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S2 (15aug)
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?
Flags: needinfo?(felash)
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.
Flags: needinfo?(felash)
Requesting QA input prior to uplift.
Flags: needinfo?(echang)
Keywords: qawanted
Whiteboard: [sms-sprint-2.1S2] → [sms-sprint-2.1S2],
(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.
Flags: needinfo?(echang)
Attachment #8470868 - Flags: qa-approval?(echang)
Keywords: qawanted
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 !
Flags: needinfo?(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.
Flags: needinfo?(azasypkin)
Status: RESOLVED → VERIFIED
Status: VERIFIED → RESOLVED
Closed: 6 years ago6 years ago
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 ?
Flags: needinfo?(schung)
Flags: needinfo?(felash)
Yes it really helps a lot
Flags: needinfo?(schung)
Flags: needinfo?(felash)
(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 ?
(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/
Flags: needinfo?(jsmith)
QA Whiteboard: [COM=Gaia::Messages]
(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.
Flags: needinfo?(jsmith)
Attachment #8470868 - Flags: qa-approval?(echang) → qa-approval+
QA Whiteboard: [COM=Gaia::Messages] → [COM=Gaia::SMS]
(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 :)
lets uplift this only after the race in 1054004 is fixed.
blocking-b2g: 2.0? → 2.0+
Whiteboard: [sms-sprint-2.1S2], → [sms-sprint-2.1S2], [NO_UPLIFT]
Removing NO_UPLIFT, since bug 1054004 has landed on master and v2.0
Whiteboard: [sms-sprint-2.1S2], [NO_UPLIFT] → [sms-sprint-2.1S2]
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
Status: RESOLVED → VERIFIED
From Comment28 change the Tracking Flags
You need to log in before you can comment on or make changes to this bug.