1028452 - [Collections] Opening a smart collection is slow

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::Everything.me, defect, P1)

Description

[Peter deHaan [:pdehaan]]

STR:
1. Flash to 2.0.0 (I'm using B2G 2.0.0-prerelease, 32.0a2, b20140620000202)
2. Open the Music or Social Smart Collection.


Actual results:
I'm on Mozilla Guest network, but I also have a 3G sim card in. Loading is painfully slow and I never see content.


Expected results:
Faster. I'm not sure what the huge delay is (big background image, remote server fetch, etc) but my screen goes to sleep long before i see *any* content.

Comment 1

[Peter deHaan [:pdehaan]]

Note that "Games" and "Entertainment" loaded after a lengthy delay, but ultimately loaded.
Now the icons for those two groups disappeared from my homescreen and reverted to some rocket icon. Will file as separate bug.

Comment 2

[Jason Smith [:jsmith]]

Can we get a video of the bug here to see the performance issue in question?

Comment 3

[Kevin Grandon :kgrandon]

There is definitely some performance tweaks that need to be addressed here. We may be able to add caching for second launches, but I think we need to optimize the very first launch case.

Devs - we should do some profiling of the code to check the time for each server response and compare that to app load time.

(This is slightly out of control on the gaia side being that smart collections are mainly a server based thing).

Comment 4

[Ran Ben Aharon [:ranbena] EverythingMe]

Regarding caching - should I open a separate bug? Is it desired?

Another issue that contributes to this perception is that installed pinned apps are displayed only after web result promise resolve. I think it's a quick fix. Lmk if you want me on it.

Comment 5

[Brogan Zumwalt [Inactive]]

Issue appears significantly improved from Fridays build. Smart Collection loading still may be slower than expected.

Youtube link: http://youtu.be/4pcgRetnx94

Environmental Variables:
Device: Flame 2.0
Build ID: 20140623000201
Gaia: 729f214b887ce8efe7d870145d31acb2c6427817
Gecko: 117ba3eda4d2
Version: 32.0a2 (2.0) 
Firmware Version: v122

Comment 6

[Kevin Grandon :kgrandon]

(In reply to Ran Ben Aharon (Everything.me) from comment #4)
> Regarding caching - should I open a separate bug? Is it desired?

I think we should look into it, but only after the very first load is better personally.


> Another issue that contributes to this perception is that installed pinned
> apps are displayed only after web result promise resolve. I think it's a
> quick fix. Lmk if you want me on it.

This would be good to have. Also I'm seeing pinned apps pop-up immediately, and then appear to refresh after the web results come in. We should probably save some cycles and make sure we don't re-render the pinned results.

Comment 7

[Jason Smith [:jsmith]]

Can we get a video of 1.4 as a point of comparison?

Comment 8

[James Lal [:lightsofapollo]]

Going to take a quick look into this and see if it is something we can improve without caching

Comment 9

[James Lal [:lightsofapollo]]

A quick investigation on my phone it took 4.8s to show the collections once the app had launched (_way_ long time) the interesting thing is the majority of the time spent was in the failed geo location search https://github.com/mozilla-b2g/gaia/blob/master/shared/js/everythingme/device.js#L46 (4s).

If you see this in your logs "evme position error" then smart collection startup is going to be way slow.

Comment 10

[James Lal [:lightsofapollo]]

Note the above applies to any and all files using the eme shared scripts (collections view, add, etc..) they block showing the UI on the geo location call which times out (On my flame eng build anyway in San Jose)

Comment 11

[James Lal [:lightsofapollo]]

Resetting to default for now... Can we verify my assumptions above? This may not be as bad as it looks because of the geolocation timeouts.

Comment 12

[Brogan Zumwalt [Inactive]]

Video of smart collection loading on 1.4

Youtube Link: http://youtu.be/uQLxCErwJsU

Environmental Variables:
Device: Flame 1.4
Build ID: 20140624000202
Gaia: 1d52323cd5b6c7d646f444712c81777d34f74e36
Gecko: 4e9d3d4412f9
Version: 30.0 (1.4) 
Firmware Version: v122

Comment 13

[Jason Smith [:jsmith]]

So I think the performance is worse in the new homescreen based on the videos here, but I can't tell by how much.

Can we do some stopwatch measurements (5 measurements for each branch) with the same collection on 1.4 vs. 2.0 to see how fast it takes that collection to load?

Comment 14

[James Lal [:lightsofapollo]]

The performance is likely worse for various reasons... We can make this better at various levels but I think the worst area right now (when you exclude the geolocation timeouts) is the creation of collections.

Steps to add a collection:

  - Start an IAC with the collections app asynchronously 

  - When adding a collection return only the collection names without saving to datastore or any other operations that delay the selection of the collection names.

  - Return the collection details in the activity response (with or without the icon not sure....)

  - Show the collection on the homescreen

  - Asynchronously save the collections to the datastore by talking back over IAC to the collections app.

This approach is more complicated but gives us a lot more room for potential perceived performance improvements.

Comment 15

[James Lal [:lightsofapollo]]

Breaking off the add collection optimizations into a new bug blocking this one

Comment 16

[Jason Smith [:jsmith]]

Based on the videos alone, triage decided to block on this. The measurements are still useful to get here though.

Comment 17

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

James, we're assigning this to you in perf triage since you seem most able to handle this. If someone else is a better fit for this please reassign to them.

Comment 18

[James Lal [:lightsofapollo]]

I won't be able to fix every issue here so I am going to un-assigning myself (and pick up those items I am fixing).

To recap there are three issues we know about here:

  1. Delete of collections are super slow (fix pending review)
  2. Geolocation timeouts make everything look 4s slower on startup (see above for what to look for)
  3. Adding collections could be faster by showing the text first then backfilling other things (like actually saving the collection to datastore, etc..)


Remember to be careful about 2 when debugging these issues if it takes only 5s to view the collection and geolocation timeouts are occurring then maybe we don't need to do anything.

Another approach would be to show some better spinner, etc.. while e.me "device" is initializing so we don't just sit there.

Comment 19

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

James, any idea who we might ping to figure out an assignee for this?

Comment 20

[Kevin Grandon :kgrandon]

I will probably be taking this in the next few days, but not assigning it to myself until I start working on it. I might get one of the E.me guys to take it, so let's leave unassigned for now until I check with them.

Comment 21

[Kevin Grandon :kgrandon]

I am renaming this bug to reflect that it will only track viewing of a smart collection. We have recently done some work to make deletion better.

Comment 22

[Kevin Grandon :kgrandon]

I spoke to Ran about this today, and I think we need to make sure we do not delay API requests for geolocation calls. I think we are going to investigate caching of the geolocation information and just using geoIP for the initial load.

Comment 23

[Kevin Grandon :kgrandon]

Attachment: Github pull request

WIP - this is a potential solution that caches the geolocation result and reuses it for future requests. If we don't have a geolocation result, we should just use the geoIP fallback.

We need to see if this is acceptable from the E.me side. Note that the geolocation cache can be updated whenever the user interacts with the collection app including dropping an app on a collection, installing an app, or uninstalling an app. There may be additional hooks that we could leverage to ensure the cache is "fresh" if we run into problems.

Comment 24

[Kevin Grandon :kgrandon]

Hey Doug - we are seeing constant geolocation timeouts on flame devices that are contributing to poor smart collection performance. I was just wondering if this was a known issue or if you could point us at some bugs to look at? Thanks!

Comment 25

[Kevin Grandon :kgrandon]

Comment on attachment 8446823 [details] [review]
Github pull request

Ran - while we investigate the geolocation slowness, can you tell me if you think that this would be an acceptable approach? 

I personally feel that the accuracy tradeoff is acceptable for the increased responsiveness of the collection app. More details above, let me know what you think.

Comment 26

[Doug Turner (:dougt)]

if somehow you're blocking content rendering or UX on geolocation, you're doing it wrong.  There are *no* performance guarantees of this API.  I'll look into what's going on wrt geolocation.

Comment 27

[Doug Turner (:dougt)]

Is this using QC's geolocation implementation or ours?

James, are these the logs you sent me?

Comment 28

[Ran Ben Aharon [:ranbena] EverythingMe]

Kevin, I'm doing a few content related location simulation tests to determine if the UX would be hindered.

Comment 29

[Ran Ben Aharon [:ranbena] EverythingMe]

Comment on attachment 8446823 [details] [review]
Github pull request

Good to go

Comment 30

[Kevin Grandon :kgrandon]

Landed: https://github.com/mozilla-b2g/gaia/commit/9943f1984b9a50d721a05a60a9476215841ca536

I believe this *greatly* improves the current startup performance on the flame devices. We should also continue looking at geolocation API performance on this device, but don't need to do it in this bug.

I feel that the performance of opening a smart collection is now good enough to close this one.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

v2.0: https://github.com/mozilla-b2g/gaia/commit/880049c2cf169ccc7f7ea991912119baf9930996

Comment 32

[Johan Lorenzo [:jlorenzo]]

Verified the difference between build 20140629160202 and build 20140708000322 (which contains the fix from bug 1032015). The difference is really noticeable.

Comment 33

[Amir Nissim (:amirn)]

(In reply to Kevin Grandon :kgrandon from comment #23)
> Created attachment 8446823 [details] [review]
> Github pull request
> 
> WIP - this is a potential solution that caches the geolocation result and
> reuses it for future requests. If we don't have a geolocation result, we
> should just use the geoIP fallback.

It looks like the caching calls are failing because localStorage can not store the Position object here:
https://github.com/EverythingMe/gaia/blob/910341-collection-cache/shared/js/everythingme/device.js#L47

The log shows:
E/GeckoConsole(26581): [JavaScript Error: "DataCloneError: The object could not be cloned." {file: "app://collection.gaiamobile.org/shared/js/async_storage.js" line: 100}]

Is this new?

Comment 34

[Kevin Grandon :kgrandon]

Attachment: Github pull request - follow-up

Yeah, might be a platform regression. I think this should fix it, but I want to flash a new gecko and verify.

Comment 35

[James Lal [:lightsofapollo]]

>DataCloneError: The object could not be cloned

Randomly sometimes WebIDL changes have effected cloning/jsonifying of things.

Comment 36

[Kevin Grandon :kgrandon]

Comment on attachment 8478014 [details] [review]
Github pull request - follow-up

Amir - This makes the error go away for me. Want to review this follow-up?

Comment 37

[Amir Nissim (:amirn)]

Comment on attachment 8478014 [details] [review]
Github pull request - follow-up

Added one comment on Github. 10x.

Comment 38

[Amir Nissim (:amirn)]

(In reply to James Lal [:lightsofapollo] from comment #35)
> >DataCloneError: The object could not be cloned
> 
> Randomly sometimes WebIDL changes have effected cloning/jsonifying of things.

Does localStorage jsonfy non String values?
The doc says it only receives DOMStrings as values.

Comment 39

[Kevin Grandon :kgrandon]

Follow-up landed in master: https://github.com/mozilla-b2g/gaia/commit/36f28696046ca9d3bfd4edf38f231dc8fcafbe2f