1041291 - [Vertical Homescreen] Load improvements for configurator.js

Status: RESOLVED FIXED

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

Description

[Kevin Grandon :kgrandon]

We currently load init.json every time if VersionHelper.isUpgrade returns false. If we already have a configuration saved, we should not need to load and parse this file.

My guess is that we could save between ~20-40ms off initial startup.

Comment 1

[Kevin Grandon :kgrandon]

This that we might be able to improve upon:

 - Do not always load init.json
 - Streamline or remove datastore preference access.
 - Consider caching navigator.getFeature

Comment 2

[Kevin Grandon :kgrandon]

Turns out most of the calls in this file are rather tiny - the bulk of the delay here comes from VersionHelper(). Seems like we should ditch it.

Comment 3

[Johan Lorenzo [:jlorenzo]]

Nominating as per bug 1037706 comment 28.

Comment 4

[Kevin Grandon :kgrandon]

I think we need to profile to determine performance savings amount before we block on this.

Comment 5

[Cristian Rodriguez (:crdlc)]

I gonna try do something here during morning

Comment 6

[Cristian Rodriguez (:crdlc)]

Attachment: Configuration, version helper and SV only for first time

Hi Kevin, the purpose of this patch is to reduce the startup time. The idea is not to load "icc_helper.js", "shared/js/version_helper.js" and "js/configurator.js" when the home has been initialized. Can you take some measure here? I think that we have obtained almost 1 second. My hamachi is much faster :)

Comment 7

[Kevin Grandon :kgrandon]

Comment on attachment 8460110 [details]
Configuration, version helper and SV only for first time

I'm currently measuring to access impact, but the code seems solid to me and should provide a nice boost. I would also like to confirm with Carmen that SV customization should continue to work. Thanks!

Comment 8

[Lucas Adamski [:ladamski]]

This is a dependency of a CAF bug but isn't blocking 2.0.  Should it be?

Comment 9

[Jason Smith [:jsmith]]

(In reply to Lucas Adamski [:ladamski] (plz needinfo) from comment #8)
> This is a dependency of a CAF bug but isn't blocking 2.0.  Should it be?

Kevin - What do you think?

Comment 10

[Kevin Grandon :kgrandon]

[Blocking Requested - why for this release]:

(In reply to Jason Smith [:jsmith] from comment #9)
> (In reply to Lucas Adamski [:ladamski] (plz needinfo) from comment #8)
> > This is a dependency of a CAF bug but isn't blocking 2.0.  Should it be?
> 
> Kevin - What do you think?

I don't think we can make blocking decisions until we get a proper measurement here. I think our options are to either unblock until we get a measurement, or if it would make people feel better, let's just block and assume this will provide a slight boost.

It looks like this should be good though and I'll do some measurements tomorrow. Feel free to block for now, thanks.

Comment 11

[Kevin Grandon :kgrandon]

I measured and this appears to save us around 200ms. It's worth taking that for 2.0.

Comment 12

[Cristian Rodriguez (:crdlc)]

Attachment: Dragdrop feature loaded lazily

1) Delay the drag-drop module after rendering icons and load it lazily
2) Move "sort" method in item.js to "save" method that is only used the first time so we can avoid to parse it next times

These are some ideas that don't hurt us and we can gain some milliseconds thought

Comment 13

[Kevin Grandon :kgrandon]

Comment on attachment 8460787 [details]
Dragdrop feature loaded lazily

I'm confused about moving the save method - I guess the idea is that this moves performance from parse-time to runtime? I'm not really sure if it's worth the readability hit, we should measure the impact of it I think?

Happy about the dragdrop changes, we could also lazy load it inside the collection app I guess.

Comment 14

[Cristian Rodriguez (:crdlc)]

(In reply to Kevin Grandon :kgrandon from comment #13)
> Comment on attachment 8460787 [details]
> Just another idea
> 
> I'm confused about moving the save method - I guess the idea is that this
> moves performance from parse-time to runtime? I'm not really sure if it's
> worth the readability hit, we should measure the impact of it I think?

I answered in github

> 
> Happy about the dragdrop changes, we could also lazy load it inside the
> collection app I guess.

Yes, it is done in the pr

Comment 15

[Kevin Grandon :kgrandon]

Comment on attachment 8460787 [details]
Dragdrop feature loaded lazily

Looks good to me, can you be sure to fix the unit tests before landing? Thanks!

Comment 16

[Carmen Jimenez Cabezas]

Comment on attachment 8460110 [details]
Configuration, version helper and SV only for first time

I've tested this with and without single variant and with and without a valid SIM present on first run. 
It works perfectly, and the patch looks good to me.

Comment 17

[Cristian Rodriguez (:crdlc)]

First patch (Configuration, version helper and SV only for first time) merged in master:

https://github.com/crdlc/gaia/commit/bb5e48c0536703b89ef8020bd5bb18ca38283301

Comment 18

[Cristian Rodriguez (:crdlc)]

Second patch (Load drag-drop lazily) merged in master:

https://github.com/crdlc/gaia/commit/d96402f14f4e17659e9a8126c0f17f944f6e9112

Comment 19

[Cristian Rodriguez (:crdlc)]

v2.0:

https://github.com/mozilla-b2g/gaia/commit/d1437e4c4246a7e8c7e16001f4d6bd7d61e9da60
https://github.com/mozilla-b2g/gaia/commit/abdb110c6421c53221ab2d59ddb536bfc062e7a8

Comment 20

[bhavana bajaj [:bajaj]]

Kevin, do we have the updated measurements here which you were planning to do to see how much improvement the patch gives in 2.0 ?

Comment 21

[Kevin Grandon :kgrandon]

(In reply to bhavana bajaj [:bajaj] from comment #20)
> Kevin, do we have the updated measurements here which you were planning to
> do to see how much improvement the patch gives in 2.0 ?

This patch is already in 2.0. In comment 11 we measured the savings as ~200ms.