920561 - Ensure LazyLoader.load does not slow down app rendering

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Comment 1

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: lazyloader.not.in.load.path.patch

Comment 2

[Michał Budzyński (:michalbe)]

R+ me, thanks.

Comment 3

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

https://github.com/mozilla-b2g/gaia/commit/6dcb079db2ad16a2b2995c039f358ff9d6a2f6d4

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

I'm sorry, but this introduced unit test failures such as:

  1) [communications-contacts]  "before each" hook:
     Error: TypeError: Contacts.extServices.showDuplicateContacts is not a function (http://communications.gaiamobile.org:8080/contacts/js/views/form.js?time=1380145158658:535)
      at onerror (http://test-agent.gaiamobile.org:8080/common/vendor/mocha/mocha.js:4959)

So I backed out:

  https://github.com/mozilla-b2g/gaia/commit/56b68fc51500cc0415f288ab939cd86e92f1fdc6

Comment 5

[Kevin Grandon :kgrandon]

Is there a specific app that is having problems? In calendar for example I believe we do a setTimeout(fn, 0) to lazy load what's required. I'm wondering if we should handle this on a per-app level. I was also worried about having the 4ms minimum wait when we don't want it if we always call this.

Comment 6

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Kevin Grandon :kgrandon from comment #5)
> Is there a specific app that is having problems? In calendar for example I
> believe we do a setTimeout(fn, 0) to lazy load what's required. I'm
> wondering if we should handle this on a per-app level. I was also worried
> about having the 4ms minimum wait when we don't want it if we always call
> this.

Settings and likely clock. But I suspect others as well.

Comment 7

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Kevin Grandon :kgrandon from comment #5)
> Is there a specific app that is having problems? In calendar for example I
> believe we do a setTimeout(fn, 0) to lazy load what's required. I'm
> wondering if we should handle this on a per-app level. I was also worried
> about having the 4ms minimum wait when we don't want it if we always call
> this.

Also it should not be an additional 4ms delay if those are not nested, isn't it?

Comment 8

[Kevin Grandon :kgrandon]

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #6)
> Settings and likely clock. But I suspect others as well.

Not fully against landing this, but I would rather handle this per app, or make sure there's no "4ms minimum" for nested calls where it's used in other apps.

Comment 9

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: test.contact.patch

Seems to fix the contacts red.

Comment 10

[Kevin Grandon :kgrandon]

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #7)
> (In reply to Kevin Grandon :kgrandon from comment #5)
> > Is there a specific app that is having problems? In calendar for example I
> > believe we do a setTimeout(fn, 0) to lazy load what's required. I'm
> > wondering if we should handle this on a per-app level. I was also worried
> > about having the 4ms minimum wait when we don't want it if we always call
> > this.
> 
> Also it should not be an additional 4ms delay if those are not nested, isn't
> it?

Correct, but I feel like apps may already be calling it in a setTimeout(). Calendar definitely used to for example.

Comment 11

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Kevin Grandon :kgrandon from comment #10)
> (In reply to Vivien Nicolas (:vingtetun) (:21) from comment #7)
> > (In reply to Kevin Grandon :kgrandon from comment #5)
> > > Is there a specific app that is having problems? In calendar for example I
> > > believe we do a setTimeout(fn, 0) to lazy load what's required. I'm
> > > wondering if we should handle this on a per-app level. I was also worried
> > > about having the 4ms minimum wait when we don't want it if we always call
> > > this.
> > 
> > Also it should not be an additional 4ms delay if those are not nested, isn't
> > it?
> 
> Correct, but I feel like apps may already be calling it in a setTimeout().
> Calendar definitely used to for example.

I would be fine to remove the setTimeout(fn, 0) from other apps (in a followup) to avoid the extra 4ms if you want. But it sounds saner to enforce that at the library level since LazyLoad as a name tends to make people think that will be loaded later!

(we're replying very closely, just mid-air collision you!)

Comment 12

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

The dialer seems to layzload in the onload handler directly too.

Comment 13

[Kevin Grandon :kgrandon]

Comment on attachment 809915 [details] [diff] [review]
lazyloader.not.in.load.path.patch

Review of attachment 809915 [details] [diff] [review]:
-----------------------------------------------------------------

I am fine moving this into the library.

::: shared/js/lazy_loader.js
@@ +23,5 @@
> +      // we wait in the window manager to remove the cover and show
> +      // the app itself. setTimeout here ensure that the load event
> +      // is fired before we load those additional scripts that are
> +      // not needed for displaying quickly the app.
> +      setTimeout(function(self) {

If we're going to land this again, perhaps we could make a few updates?

1 - Platform standard is no anon functions, so call this nextTick or something.

2 - Can we consolidate setTimeout calls around the parent method here: https://github.com/mozilla-b2g/gaia/blob/master/shared/js/lazy_loader.js#L94

Comment 14

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Kevin Grandon :kgrandon from comment #13)
> Comment on attachment 809915 [details] [diff] [review]
> lazyloader.not.in.load.path.patch
> 
> Review of attachment 809915 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I am fine moving this into the library.
> 
> ::: shared/js/lazy_loader.js
> @@ +23,5 @@
> > +      // we wait in the window manager to remove the cover and show
> > +      // the app itself. setTimeout here ensure that the load event
> > +      // is fired before we load those additional scripts that are
> > +      // not needed for displaying quickly the app.
> > +      setTimeout(function(self) {
> 
> If we're going to land this again, perhaps we could make a few updates?
> 
> 1 - Platform standard is no anon functions, so call this nextTick or
> something.

Oh man I fight so much for that and I'm caught doing it! thanks :)

> 
> 2 - Can we consolidate setTimeout calls around the parent method here:
> https://github.com/mozilla-b2g/gaia/blob/master/shared/js/lazy_loader.js#L94

Sure. That sounds saner in fact.

Comment 15

[Kevin Grandon :kgrandon]

Comment on attachment 810260 [details] [diff] [review]
test.contact.patch

Review of attachment 810260 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fine to be assuming all contacts tests pass :) Please ensure this doesn't cause any defects in travis before landing. Thanks!!

Comment 16

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

https://github.com/mozilla-b2g/gaia/commit/8ed692c6c6df1fcabe3c65ca8fee9cd54e625244

Pushed again. Time for me to sleep.

Comment 17

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

https://github.com/mozilla-b2g/gaia/commit/ca776d1bb0fb1da21b624624892f3930e3ae800a

I didn't broke the tests, they were green but many apps were broken... backouting myself.

Comment 18

[Kevin Grandon :kgrandon]

Ack! What kind of issues were you seeing?

I wonder if me suggesting moving the setTimeout caused the failures? Perhaps some applications were thinking that it was ok to consider that the dom loader would be synchronous. Currently it has an asynchronous interface, but only uncomments the text content - so it's *possible* to use synchronously. If that's the case, I'm ok going back to your original setTimeout around just JS and CSS. Sorry!

Comment 19

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Kevin Grandon :kgrandon from comment #18)
> Ack! What kind of issues were you seeing?
> 
> I wonder if me suggesting moving the setTimeout caused the failures? Perhaps
> some applications were thinking that it was ok to consider that the dom
> loader would be synchronous. Currently it has an asynchronous interface, but
> only uncomments the text content - so it's *possible* to use synchronously.
> If that's the case, I'm ok going back to your original setTimeout around
> just JS and CSS. Sorry!

I wonder if this is related to the html import stuff that became asynchronous now and was setTimeouted too. I will redo the patch with the old try and check on travis (i'm running it locally since this night and it seems fine)

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/312f40a7ce25

Comment 21

[Carsten Book [:Tomcat]]

oh sorry (and thanks to bkelly) this was backed out later in https://hg.mozilla.org/mozilla-central/rev/f029fc8f001b

Comment 22

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: lazyload.patch

Let's do that per app to not break all the tests then...

Comment 23

[Etienne Segonzac (:etienne)]

Comment on attachment 816669 [details] [diff] [review]
lazyload.patch

Review of attachment 816669 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with nit for the dialer part

::: apps/communications/dialer/js/dialer.js
@@ +16,4 @@
>    window.addEventListener('load', function getSettingsListener() {
>      window.removeEventListener('load', getSettingsListener);
>  
> +    setTimeout(function() {

`setTimeout(function nextTick() {`

Comment 24

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Etienne Segonzac (:etienne) from comment #23)
> Comment on attachment 816669 [details] [diff] [review]
> lazyload.patch
> 
> Review of attachment 816669 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with nit for the dialer part
> 
> ::: apps/communications/dialer/js/dialer.js
> @@ +16,4 @@
> >    window.addEventListener('load', function getSettingsListener() {
> >      window.removeEventListener('load', getSettingsListener);
> >  
> > +    setTimeout(function() {
> 
> `setTimeout(function nextTick() {`

Shame on me.

Comment 25

[Fabien Cazenave [:kaze]]

Comment on attachment 816669 [details] [diff] [review]
lazyload.patch

Looks OK to me, but this patch lets me think that we should probably make LazyLoader.load() asynchronous instead of putting every `LazyLoader.load()' statement in a setTimeout.

I understand we don’t want to modify this right now, but maybe we should open a follow-up bug for this?

Comment 26

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Fabien Cazenave [:kaze] from comment #25)
> Comment on attachment 816669 [details] [diff] [review]
> lazyload.patch
> 
> Looks OK to me, but this patch lets me think that we should probably make
> LazyLoader.load() asynchronous instead of putting every `LazyLoader.load()'
> statement in a setTimeout.
>

Yep. That was my first suggestions but it needs more investigation for tests... I openeed bug 926851.

https://github.com/mozilla-b2g/gaia/commit/95862b40ecd724d75057be4c4d5dc8578ad58c0b

Comment 27

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Mike you may want this one for settings in koi.

Comment 28

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #27)
> Mike you may want this one for settings in koi.

If you look at datazilla this week, this one is the stuff that has move settings next to calendar.

Comment 29

[Mike Lee [:mlee]]

Thanks Vivien. That's a great improvement! Our perf numbers for 1.2 look good so let's pick this improvement up in 1.3.