1112551 - Improve start-up time by preloading first contacts in the list

Status: RESOLVED FIXED

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

Description

[Sergi Mansilla [:sergi] (Telenor) [PTO]]

This picks up https://bugzilla.mozilla.org/show_bug.cgi?id=1006331 to implement in master the loading of contacts above the fold right away from HTML stored in localStorage.

The old patch improves startup time (for the user) by ca. 1.2 seconds on Tarako. Some remarks:

1. At the bottom of the page vs. loading as first script in head tag with defer attribute, the defered script takes 56 ms. longer to load.
2. Setting first chunk in localstorage almost has no penalty. Takes 3 ms. of blocking code on Tarako
3. Reading first chunk neither. Takes few ms.

Reasons to use LocalStorage for it:

- Data is bigger than the cookie limit of 4K around 5K raw data, but need to serialize for cookie use, will be 7K in base64 or so)
- Startup penalty idb
- Blocking time is 3ms. on Tarako. I know all the mailing list threads, but this is close to zero.

Reasons for not using IndexedDB for it, along with benchmarks: https://bugzilla.mozilla.org/show_bug.cgi?id=1006331#c27

Comment 1

[Sergi Mansilla [:sergi] (Telenor) [PTO]]

Progress is being done on the following branch: https://github.com/sergi/gaia/compare/contacts-startup-perf

Comment 4

[Shiwei Zhang]

(In reply to Sergi Mansilla [:sergi] (Telenor) [PTO] from comment #1)
> Progress is being done on the following branch:
> https://github.com/sergi/gaia/compare/contacts-startup-perf

Dear Sergi,
   Could you provide a patch for v2.1? There are some distinct differences between v1.4 and v2.1, I made some try, but ‘groups-list’ is empty when contacts_first_chunk.js called.

Thank you.

Comment 5

[Shiwei Zhang]

Attachment: contacts_add_first_chunk.patch

Refered to v1.4 I make this patch for v2.1,The results show the first chunk in localstorage shows immediately, but pictures don't, they takes 2~3s to appear.

Could you make some improvements?

Thank you.

Comment 6

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8544400 [details] [diff] [review]
contacts_add_first_chunk.patch

Thanks Shiwei.

We will be working on a different approach.

Basically, with the current WIP we are caching the first chunk of contacts but we still make the mozContacts requests even if it is not needed and this takes some CPU cycles that we shouldn't be taking. We want to cache not only the first chunk but the entire contacts list (including favorites) and avoid the calls to mozContacts if possible.

The changes required to show the thumbnails faster are being done on Bug 1089538

Comment 7

[Shiwei Zhang]

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #6)
  Thanks Fernando, I suppose we'd better work on optimize mozContacts calls and reduce the time consumption. 

  Can we transfer these calls from DB to cache? In my opinion caching thing in contacts app is not a better solution, we can do this in system app.

Comment 8

[Shiwei Zhang]

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #6)
... ...
> The changes required to show the thumbnails faster are being done on Bug
> 1089538

Yes, we added default photos to improve this.

Comment 9

[jinju.ma]

Dear all,

  Could I ask why the "first-chunk" patch was removed on v2.1? If it's because the following "copy" problem:
    1. Add two different contacts in Contacts app, for example, "aaa" and "bbb".
    2. Kill the Contacts app and entry again.
    3. Sometimes it will appear three contacts, "aaa","bbb" and "bbb".

  I find the reason. Look at the following code，there is a loophole in the cycle. The variable childNodes.length will be changed after calling removeChild. So the real number of cycles are less than childNodes.length.

      for (var i = 0; i < childNodes.length; i++) {
        if (childNodes[i].id !== 'section-group-favorites') {
          childNodes[i].parentNode.removeChild(childNodes[i]);
        }
      }


  I try to modify this cycle, and the "copy" problem no longer appears.
  {
    var maxIndex = childNodes.length - 1;
    while (maxIndex >= 0) {
      if (childNodes[maxIndex].id !== 'section-group-favorites') {
        childNodes[maxIndex].parentNode.removeChild(childNodes[maxIndex]);
      }
      if (maxIndex > 0) {
        maxIndex--;  
      } else {
        break;
      } 
    }
  }

  So, would you reuse this first-chunk mechanism on v2.1?

Comment 10

[Francisco Jordano [:arcturus] [:francisco]]

(In reply to jinju.ma from comment #9)
> Dear all,
> 
>   Could I ask why the "first-chunk" patch was removed on v2.1? If it's
> because the following "copy" problem:
>     1. Add two different contacts in Contacts app, for example, "aaa" and
> "bbb".
>     2. Kill the Contacts app and entry again.
>     3. Sometimes it will appear three contacts, "aaa","bbb" and "bbb".
> 
>   I find the reason. Look at the following code，there is a loophole in the
> cycle. The variable childNodes.length will be changed after calling
> removeChild. So the real number of cycles are less than childNodes.length.
> 
>       for (var i = 0; i < childNodes.length; i++) {
>         if (childNodes[i].id !== 'section-group-favorites') {
>           childNodes[i].parentNode.removeChild(childNodes[i]);
>         }
>       }
> 
> 
>   I try to modify this cycle, and the "copy" problem no longer appears.
>   {
>     var maxIndex = childNodes.length - 1;
>     while (maxIndex >= 0) {
>       if (childNodes[maxIndex].id !== 'section-group-favorites') {
>         childNodes[maxIndex].parentNode.removeChild(childNodes[maxIndex]);
>       }
>       if (maxIndex > 0) {
>         maxIndex--;  
>       } else {
>         break;
>       } 
>     }
>   }
> 
>   So, would you reuse this first-chunk mechanism on v2.1?

It was never added. We created that patch just for 1.4 for Dolphin devices. Now we are planning to reintroduce in a better way for 2.2

Comment 11

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v1

Comment 12

[Fernando Jiménez Moreno [:ferjm]]

First green try https://treeherder.mozilla.org/#/jobs?repo=gaia-try&revision=6fb8eb99528e Now we need more tests

Comment 13

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8567213 [details] [review]
v1

Francisco, I still need to finish the unit tests, but I could use some feedback in the mean time.

I also just realized that we don't evict the cache when we change the locale (RTL vs LTR), so I also need to fix that edge case.

Thanks!

Comment 14

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 8567213 [details] [review]
v1

Really good job here!

Left some comments on github. Basically related to oncontactchange, we don't need to evict manually and just do it oncontactchange. Couple of nits and some questions, but looking really good.

Did a few test and didn't catch any major issue. @Johan, could you test the fb sync, changing a contact in FB and wait a day to see if we get the correct changes?

I think we can change that time on the config file for the app, to make it easy to test.

Comment 15

[Johan Lorenzo [:jlorenzo]]

Comment on attachment 8567213 [details] [review]
v1

This is a great start which really improves the feeling of performance. I don't think it's ready yet to be landed on master for 2 reasons:

> * Search is broken if you search before the contacts' pictures are loaded.
> * ICE contacts are displayed once the pictures are, a user can really 
>   stress out if an emergency contact is not displayed immediately.

Apart from that, I see some other issues which are not blocking the land but are release blockers to me:

> * Delete a contact and then create a new one => cache doesn't get updated,
>   it'll display the deleted contact again after restarting the app
> * Sometimes, favorites contacts appears twice or 3 times. I haven't
>   figured out the exact STR but I've seen it multiple times while adding
>   new contacts.
> * Sometimes, when you add a new Facebook friend who should be in the cache
>   (like a friend called: "Aaron Aname") is not displayed if you close the app
>   just after the sync.
> * Alphascroll works but stays at the A letter until all the contacts are
>   loaded. I think we need a UI feedback (a spinner at the end of the list?) to
>   say: "Hey, don't worry, I'm still loading the rest". Same comment when you
>   scroll down until the end of the cache.
> * Like said in comment 13, we need to handle RTL.

And finally some nice to have:
> * The Facebook icon is loaded at the same time as other icons. I don't know
>   if it'll cost to display it before.
> * The cache is evicted even if a contact was not present initially present.

I'll wait until tomorrow to see if the change of the a FB friend name on FB directly works with this patch.

Here's a mindmup of the test plan I executed: https://www.mindmup.com/#m:g10B7Y2LpZbqKluVVZEbDE5UHpZYW8

Comment 16

[Johan Lorenzo [:jlorenzo]]

I've got the results from the auto-update on FB:
* For a contact who's not in the cache => OK
* For a contact who's in the cache, modified or not => KO

What's happening is, when the update occurs, these contacts are put at the bottom of the cached list (so not in alphabetical order anymore). Then, when you close the app and reopen it, they are correctly ordered.

This bug would be a release blocker.

Comment 17

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8567213 [details] [review]
v1

Thanks for all the feedback. I think this is ready for a new review/qa-approval pass.

Thanks!

Comment 18

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 8567213 [details] [review]
v1

Left a message on github, codewise looking good, when I try on the phone, I'm unable to use fb, gmail or live, with following error:

I/Communications( 4263): Content JS INFO: Idle event!!!. FB Migration about to start
W/GeckoConsole( 2443): [JavaScript Error: "IndexedDB UnknownErr: ActorsParent.cpp:412"]
E/Communications( 4263): Content JS ERROR: Indexed DB cannot be opened:  UnknownError
W/Communications( 4263): [JavaScript Error: "UnknownError" {file: "app://communications.gaiamobile.org/contacts/js/fb/datastore_migrator.js" line: 203}]

Great job btw, need to verify that error is not happening without the patch.

Comment 19

[Fernando Jiménez Moreno [:ferjm]]

Thanks!

That error happens when you flash the device and push the workload reference without launching the contacts app first. That's also happening on master.

Comment 20

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 8567213 [details] [review]
v1

\o/ according to comment 19 that was the only reason for not giving the r+, left some tiny comments, but the work is perfect.

Thanks again for such an important peace of work.

Comment 21

[Johan Lorenzo [:jlorenzo]]

Attachment: Link to 2000 contact as vcard files

Comment 22

[Johan Lorenzo [:jlorenzo]]

Comment on attachment 8567213 [details] [review]
v1

That's a good job and we're getting close! I'm minus'ing the patch because of 2 big issues from which you can't recover. Here are the details:


Blocking the land:
> * The cache doesn't get updated if you add/delete a contact from the an activity dialer or the mozContact API.
It's okay if the contact doesn't belong to the cache.

> * The cache only gets displayed whatever the time you wait
1. Pass the FTU
2. Open contacts
3. Import your 2000 contacts
4. Once done, close the app
5. Reopen contacts again.
6. Wait, wait... the images are never loaded and so the rest of the contact list  


Remaining blockers:
> * Like said in comment 13, we need to handle RTL.
If you switch to an RTL language before the icon are loaded, you'll loose all the contacts but one (the first). No deletion to report if you switch while everything is loaded or while the contact app is not open. That's why I think it doesn't block the land.


Not blocking issues:
> * ICE contacts are displayed once the pictures are, a user can really 
>   stress out if an emergency contact is not displayed immediately.
The icon is visible but can't be taped (whereas regular contacts can). Do you think we can display the ICE panel immediately?

> * Alphascroll works but stays at the A letter until all the contacts are
>   loaded. I think we need a UI feedback (a spinner at the end of the list?) to
>   say: "Hey, don't worry, I'm still loading the rest". Same comment when you
>   scroll down until the end of the cache.
Not changed since. After taking a look at the previous implementation, we're actually isofunctional. I don't see a reason to block on this then.


Fixed:
> * Search is broken if you search before the contacts' pictures are loaded.
Search is now available. You're able to search a cached contact or a just loaded contact. If you look up a contact which has not been loaded yet, you'll have a spinner waiting for the result => This is great!

> * Delete a contact and then create a new one => cache doesn't get updated,
>   it'll display the deleted contact again after restarting the app
Working correctly now.

> * Sometimes, favorites contacts appears twice or 3 times. I haven't
>   figured out the exact STR but I've seen it multiple times while adding
>   new contacts.
I haven't seen the issue again.

> * Sometimes, when you add a new Facebook friend who should be in the cache
>   (like a friend called: "Aaron Aname") is not displayed if you close the app
>   just after the sync.
I haven't seen the issue again.

> * The Facebook icon is loaded at the same time as other icons. I don't know
>   if it'll cost to display it before.
Fixed

> * The cache is evicted even if a contact was not initially present.
Okay in that particular case

> I've got the results from the auto-update on FB:
> * For a contact who's in the cache, modified or not => KO
It's okay now either when the app is displayed or when it's closed. The Facebook icon disappears after an update but is restored after every photo is loaded.

Comment 23

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Johan Lorenzo [:jlorenzo] (QA) from comment #22)
Thanks Johan. 
> > * The cache only gets displayed whatever the time you wait
> 1. Pass the FTU
> 2. Open contacts
> 3. Import your 2000 contacts
> 4. Once done, close the app
> 5. Reopen contacts again.
> 6. Wait, wait... the images are never loaded and so the rest of the contact
> list  
> 

I cannot reproduce this one :(

Comment 24

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Johan Lorenzo [:jlorenzo] (QA) from comment #22)
> Remaining blockers:
> > * Like said in comment 13, we need to handle RTL.
> If you switch to an RTL language before the icon are loaded, you'll loose
> all the contacts but one (the first). No deletion to report if you switch
> while everything is loaded or while the contact app is not open. That's why
> I think it doesn't block the land.

If by "icon" you mean the contact photo, I cannot reproduce this one neither.

Comment 25

[Johan Lorenzo [:jlorenzo]]

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #23)
I purged the cache and the data with the factory menu (restart + vol. up) and I repro'd it 4 times with these STR. I thought they were enough. I'll investigate further then. In the meantime, this can't block the land then.

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #24)
> If by "icon" you mean the contact photo, I cannot reproduce this one neither.
That's right, the contact photo. I tried with 2000 contacts. I'll give it a shot a again today and get a video if I succeed.

Comment 26

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Johan Lorenzo [:jlorenzo] (QA) - On PTO, back on March 16th from comment #22)
> Comment on attachment 8567213 [details] [review]
> v1
> 
> That's a good job and we're getting close! I'm minus'ing the patch because
> of 2 big issues from which you can't recover. Here are the details:
> 
> 
> Blocking the land:
> > * The cache doesn't get updated if you add/delete a contact from the an activity dialer or the mozContact API.
> It's okay if the contact doesn't belong to the cache.
> 

This one should be fixed already in the latest version.

> Not blocking issues:
> > * ICE contacts are displayed once the pictures are, a user can really 
> >   stress out if an emergency contact is not displayed immediately.
> The icon is visible but can't be taped (whereas regular contacts can). Do
> you think we can display the ICE panel immediately?
> 

This one too.

Comment 28

[Fernando Jiménez Moreno [:ferjm]]

This is green on try and passes all my local tests. I agreed with Francisco that it is ok to land this on master given that all the reported issues that I could reproduce has been fixed. We will ask for QA approval to uplift it to 2.2 to Johan again once we return from PTO.

Comment 29

[GitHub Autolander Bot]

http://docs.taskcluster.net/tools/task-graph-inspector/#FBonPx9TSUabHL_YfxRgWA

The pull request failed to pass integration tests. It could not be landed, please try again.

Comment 30

[Fernando Jiménez Moreno [:ferjm]]

The numbers are the following after applying this patch:

➜  gaia git:(master) ✗ test-perf-summary master.json perf.json
communications/contacts (means in ms)  Base   Patch  Δ     Sig?
-------------------------------------  -----  -----  ----  ----
moz-chrome-dom-loaded                   1147    585  -562
moz-chrome-interactive                  1267   1393   126
moz-app-visually-complete               1601    874  -727
moz-content-interactive                 2131   1393  -738
moz-app-loaded                         10942  10409  -533

We safe ~720ms of start-up time \o/

Comment 31

[Kevin Grandon :kgrandon]

Trying again to see what autolander does..

Comment 32

[GitHub Autolander Bot]

Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/f1d0684817e5802961c02a04dcf667cfaf09d6ee

Comment 33

[Johan Lorenzo [:jlorenzo]]

Comment on attachment 8567213 [details] [review]
v1

I don't repro the issues like described in comment 25. I checked the results in comment 26.

The cache gets updated if you edit a contact within the cache. With 2000 contacts it takes a couple of seconds though. It's nearly immediate with 500 contacts. Hence I don't see the point of blocking on that.

Thanks for fixing all these bugs!

Comment 34

[Johan Lorenzo [:jlorenzo]]

[Blocking Requested - why for this release]: Performance improvement is a part of the 2.2 release.

Comment 35

[bhavana bajaj [:bajaj]]

This looks like a huge change and I'd like to know the risk here and understand how much gain we are getting.

Comment 36

[Fernando Jiménez Moreno [:ferjm]]

(In reply to bhavana bajaj [:bajaj] from comment #35)
> This looks like a huge change and I'd like to know the risk here and
> understand how much gain we are getting.

This is indeed a big change, but the improvements are quite outstanding as well. With this patch we save around 50% of the start-up time that we are currently requiring in 2.2. I wrote a blog post about it [1] linking to the Datazilla records and a video where you can see these improvements.

We asked for two QA reviews before landing in master and we also have been testing this patch in master for more than a week already. We caught two small regressions though. I already proposed a fix for one of them (bug 1143583) and I am in the process of fixing the other one (bug 1144433).

I believe the benefits of this patch are bigger than the risk of uplifting it to 2.2. We could always use a final QA review before uplifting it though :)

[1] http://ferjm.ghost.io/improving-fxos-contacts-app-performance/

Comment 37

[Johan Lorenzo [:jlorenzo]]

Triage: Some testing has already been done against the patch but, as it's a huge one, we think it's better to have another round of testing by a second tester. QA wanted to execute exploratory testing around this patch.

Here's a mindmup[1] to have a view of what the performance patch affects. Feel free to ask me any question on it.

Clearing the nomination while we get the results.

[1] https://www.mindmup.com/#m:g10B7Y2LpZbqKluVVZEbDE5UHpZYW8

Comment 40

[Martin Shuman [:Marty]]

I tested start-up behavior and data integrity using a set of 25-30 contacts to perform the following actions, with app closure and restart inbetween.  I then repeated these tests with a set of 200+ contacts.

-Create a contact so it will be inside the cached list
-Create a contact so it will be outside the cached list

-Changed a contact name within the cached list.
-Changed a contact name outside the cached list.
-Changed a contact name outside the cached list so that it moves into the cached list.
-Changed a contact name within the cached list so that it moves outside the cached list.

-Added a photo to a contact in the cached list
-Added a photo to a contact outside the cached list

-Changed a photo for a contact in the cached list
-Changed a photo for a contact outside the cached list

-Removed a photo from a contact in the cached list
-Removed a photo from a contact outside the cached list

-Change the Locale settings from English (LTR) to Arabic (RTL)
-Change the Locale settings from Arabic (RTL) to English (LTR)

The only issue I encountered was when launching the Contacts app in Arabic (RTL), the contact list initially appears in LTR format, then switches to RTL format (bug 1144433).

Comment 41

[Johan Lorenzo [:jlorenzo]]

[Blocking Requested - why for this release]: Performance improvement is a part of the 2.2 release. Another round of testing has been made against master in comment 40.

Comment 42

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8567213 [details] [review]
v1

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Contacts app
[User impact] if declined: Worst performance and longer wait time on app start-up
[Testing completed]: Added unit tests. Plenty of manual tests performed. Went through QA approval.
[Risk to taking this patch] (and alternatives if risky): This is a big patch that touches a lot of pieces in the contacts app, so uplifting it has some risks. However, we've been testing it for three weeks already and the only two regressions found has been quickly fixed. This seems to be pretty stable in master and given the performance improvements I think it is worth taking the risk of uplifting it to 2.2
[String changes made]: None

Comment 43

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/01c26f2c70f71c537b85274b0327710722e18468

Comment 44

[Johan Lorenzo [:jlorenzo]]

Removing the qawanted. The patch has been tested again in comment 40.