902873 - [Contacts] Performance improvements when selecting in contact list

Status: RESOLVED FIXED

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

Description

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

+++ This bug was initially created as a clone of Bug #901079 +++

As part of the review of bug 901079, we decided to move to a different but (this one), some task that we have pending to improve the contact list in select mode.

We did this to work in parallel in this improvements and the export functionality that depends on this select mode.

Some of the topics (sure we can find more) we have pending for this bug are already commented in PR: 
https://github.com/mozilla-b2g/gaia/pull/11398

There we found things like:
- Better css selectors
- Take care of the navigator.mozContacts.find({}) we do in the contacts selecte promise
- Click handler in the search while in select mode

Sure we can find as well more points of improvement :)

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

I can work this next week unless someone else gets to it first.  Thanks for writing it up!

Comment 2

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

Hi,

was using the current code to continue working on the export functionality, and right now in some cases (when we 'select all'), we have to fetch all the contacts again (that's what we will try to avoid here I guess), but taking into account that we are returning a promise of contacts, fetching contacts and then filtering just the ids, does it makes sense that this selection process, returns already a promise of contacts, not contact ids?

Am I asking for too much :(?

Cheers!

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Well, this means the list would have to do that fetching instead.  Let me think about it.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Francisco, what do you think of the following:

1) I will modify selectFromList() to return a cursor style object.  So essentially like the promise, but you get onsuccess() called multiple times and you need to call continue() to get the next item.  This cursor will provide the complete contact object.

2) The contact_list.js code will implement the cursor by wrapping the underlying contacts API.  In the case of select all, our continue() will simply call the underlying getAll() cursor's continue().  In the case of a small set of contacts, we would call contact API's find() for the next contact.

3) The code can further optimize by looking in our loadedContacts hash for the contact object.  If its there, then we can just return it immediately.

So the export code would then use the select API by doing something like this:

  var cursor = contacts.List.selectFromList('Selection Title');
  cursor.onsuccess = function onsuccess(contact) {
    if (!contact) {
      // selection complete!
      return;
    }

    // export contact

    // if we decide we want to stop the selection process
    if ( /* some condition */) {
      cursor.cancel();
      return;
    }

    // otherwise get the next contact
    cursor.continue();
  };

  cursor.onerror = function onerror(err) {
    // handle error... no more contacts coming from this cursor
  }

Does this sound reasonable?

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Sorry, one additional detail.  The contact list would implicitly leave the selection mode after either of the following conditions:

1) We run out of selected contacts and pass null to onsuccess callback.  When the callback returns then we exit the selection mode.

2) When cursor.cancel() is called, then we exit the selection mode immediately.  If there are remaining selected contacts to be reported then they are discarded.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Again, sorry for the bugmail spam, but one last thought.

Perhaps we should make loading the full contact optional.  For example, I don't think deleting contacts would require that.

To do that we would add a boolean parameter to selectFromList() indicating if contact objects are needed.  The onsuccess() callback would get both an id and an contact object passed to it.

So:

  var cursor = contacts.List.selectFromList('Selection Title', true);
  cursor.onsuccess = function onsuccess(id, contact) {
    // id is always provided
    // contact is provided because we passed true to selectFromList() above
    cursor.continue();
  };

Comment 7

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

(In reply to Ben Kelly [:bkelly] from comment #4)
> Francisco, what do you think of the following:
> 
> 1) I will modify selectFromList() to return a cursor style object.  So
> essentially like the promise, but you get onsuccess() called multiple times
> and you need to call continue() to get the next item.  This cursor will
> provide the complete contact object.
> 
> 2) The contact_list.js code will implement the cursor by wrapping the
> underlying contacts API.  In the case of select all, our continue() will
> simply call the underlying getAll() cursor's continue().  In the case of a
> small set of contacts, we would call contact API's find() for the next
> contact.
> 
> 3) The code can further optimize by looking in our loadedContacts hash for
> the contact object.  If its there, then we can just return it immediately.
> 
> So the export code would then use the select API by doing something like
> this:
> 
>   var cursor = contacts.List.selectFromList('Selection Title');
>   cursor.onsuccess = function onsuccess(contact) {
>     if (!contact) {
>       // selection complete!
>       return;
>     }
> 
>     // export contact
> 
>     // if we decide we want to stop the selection process
>     if ( /* some condition */) {
>       cursor.cancel();
>       return;
>     }
> 
>     // otherwise get the next contact
>     cursor.continue();
>   };
> 
>   cursor.onerror = function onerror(err) {
>     // handle error... no more contacts coming from this cursor
>   }
> 
> Does this sound reasonable?

Sorry for the delay way to much work :S.

Sounds good to me, my only concern is in some strategies we can save contact as we get it, but in other cases we have to wait till we have all the contacts (like sending a vcard with 2000 contacts via bluethooth). Shouldn't be a problem at all.

I've just pushed the code for the generic export, using current solution, but at least you could see what we are working.

https://github.com/mozilla-b2g/gaia/pull/11550

Right now the list is leaving the selection mode once the user press on the action.

Will try to find you in IRC.

Thanks!

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Work-in-progress branch is here:

  https://github.com/wanderview/gaia/tree/contacts-select-perf

This moves the selection state information out of the DOM and into a backing model.  This lets us only render checkboxes that are actually on the screen.  Switching modes and performing selection is also fast.

I still need to integrate this with the promise/cursor.  I am planning to pursue an interface as I outlined in comment 6.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Pull request at https://github.com/mozilla-b2g/gaia/pull/12068

This pull request makes a few changes:

1) Perform list selection on a backing javascript object instead of using the DOM directly.  This allows us to avoid maintaining the full DOM for large lists.
2) Refactor selection styles to use more targeted selectors.  Styles are applied and dropped for DOM nodes as they come on and off screen.
3) Provide selection results via a streaming cursor interface.
4) If the caller wants contacts loaded, then try to get them from the list cache if present or pull them from mozContacts if the cache has been flushed.

For now I have done a very trivial integration with the export code that does not take advantage of the streaming or loading code.  I did this to try to avoid conflicting with other feature code currently in progress.

From a performance perspective this code maintains our current load speed, but allows us to enter/leave selection mode much more quickly.  The ~2 second jank for 1000 contacts is gone with these changes.

Comment 10

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

Comment on attachment 802233 [details]
Pull request at https://github.com/mozilla-b2g/gaia/pull/12068

Jose could you talk a look to this, I'll be on PTO next week and won't be able to review it.

Thanks!

Comment 11

[Jose Manuel Cantera]

Provided some substantial comments on GH, about the size of the PR and other architectural details about the proposed optimizations.

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Jose M. Cantera from comment #11)
> Provided some substantial comments on GH, about the size of the PR and other
> architectural details about the proposed optimizations.

Thanks, I pushed changes where I could and responded with questions for further clarification on a few items.

Note, if you feel a dramatically different approach is needed we may need the comms team to take the lead there.  Right now my main priority needs to be the 1.2 app launch and FPS regressions.

Since this effects export code directly, should we get Francisco's input after his return?  (tomorrow?)

Thanks again for the review.  Great comments!

Comment 13

[Jose Manuel Cantera]

Definetely we need to talk to Francisco about this. Bt I think it is a good idea to take over this work by the Comms team. 

thanks!

Comment 14

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

Hi folks, sorry for the delay, still didn't have time to dive completely in the code (specially after the changes by the first great review).

I promise tomorrow you'll have a detailed review, but as Jose comments, perhaps we will free you from this work, and continue the changes. What is not clear, yet, to me is if should be 1.2 or 1.3 work.

Cheers,
F.

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Unassigning myself per comment 13 and comment 14.

Comment 16

[Dietrich Ayala (:dietrich)]

(In reply to Francisco Jordano [:arcturus] from comment #14)
> I promise tomorrow you'll have a detailed review, but as Jose comments,
> perhaps we will free you from this work, and continue the changes. What is
> not clear, yet, to me is if should be 1.2 or 1.3 work.

This is a koi+ bug. Contacts performance is already so slow that we cannot ship 1.2 without fixing it.

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

Taking this again until we can find out who will be working it from the comms team side.  David, what do you think?  Thanks.

Comment 18

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

Hi all,

taking this to free Ben from this work.

@Ben, if you don't mind I'll take this work, and will ask you for review when ready :)

Thanks a lot!

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

Thanks Francisco!  Please let me know if you have any questions or if I can help in any way.

Also, in terms of the 1.2 vs 1.3 discussion, I think this plays into the OOM crash we saw at Oslo (bug 914532).  While this patch by itself is not sufficient to solve that issue, I think we do need some streaming API like the cursor I propose in order to avoid loading the entire list.  It would seem solving that OOM crash would be something to target for 1.2 (although its currently only koi?).

Thanks again.

Comment 20

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

Hi folks,

I've been going deep in the PR and, as Jose comments, perhaps is a bit big (risky).

Also found a couple of problems:
- When selection all, we just select the list of contacts already render, when we are on a list of 2000 contacts, the new ones not added to the list previously are not selected.
- When clicking on select all, and we deselect one element, if we are in the case of a huge list (not rendered completely) we have contacts (the newly created) that are not selected, despite that the user click on select all.

A part from that, I think that the idea of working with the visibility monitor just as Ben coded it is a must.

So I''ll propose tackle this step by step.

I'm attaching a first PR, that adds Ben's code regarding the use of the visibility monitor, and the selection based on ids. Also here I took into account the previous corner cases that I've commented previously. This keeps the use of the promise as it is, but improves a lot the use of the list.

We can merge this first PR, and later give another step trying to get ride of the promise.

What do you think guys?

Comment 21

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

Attachment: PR with the selection based on visibility monitor

Ok, now attaching the patch.

As you can see, 60% of the code is reused from Ben's proposal, just adapting to work with the corner cases (at least the ones that I can figure out), and still working with the promise.

Comment 22

[Jose Manuel Cantera]

Comment on attachment 802233 [details]
Pull request at https://github.com/mozilla-b2g/gaia/pull/12068

cancelling review as we are going to make this patch obsolete

Comment 23

[Jose Manuel Cantera]

Comment on attachment 812768 [details]
PR with the selection based on visibility monitor

nice implementation, although some commments left on Github, particularly some comments concerning additional optimizations that could be implemented. 

thanks Francisco!

Comment 24

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 812768 [details]
PR with the selection based on visibility monitor

Overall I think it looks good.  As I wrote in the PR, though, I believe there are still some holes in keeping selectedContacts object up-to-date and places where the DOM is still being used instead of selectedContacts.

Thanks Francisco!

Comment 25

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

Comment on attachment 812768 [details]
PR with the selection based on visibility monitor

Ready for a proper review :)

Comment 26

[Jose Manuel Cantera]

Comment on attachment 812768 [details]
PR with the selection based on visibility monitor

good work,

thanks!

Comment 27

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 812768 [details]
PR with the selection based on visibility monitor

Thanks for the updates!

Unfortunately, though, I think I see some inconsistencies in the selectAllPending variable.  I think we should document exactly how we want that to work a bit more and make sure all the cases are handled before landing.  See my comments on github.

Also, there is an undefined variable used in the promise resolve() function.

For these reasons I'm going to r- for now and request another round of reviews.  Sorry I didn't catch these issues in the last round!

Comment 28

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

Comment on attachment 812768 [details]
PR with the selection based on visibility monitor

Thanks for the review, you catched a couple of bugs.

I've just uploaded a new version, with the problems solved and with a (hopefully) better explanation of the selectPendingAll.

Note that I renamed this variable, to pretend to be more clear with it's current behaviour, but even though, more comments have been added to explain better what it does.

Thanks again Ben!

Comment 29

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 812768 [details]
PR with the selection based on visibility monitor

Add myself back to the r? to remind myself to do this.  Sorry I missed doing the review on friday, but didn't see it in my review queue.  I'll take care of it first thing Monday!

Comment 30

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

Thanks Ben,

Don't know why I missed setup the review back to you, and set it for ... no one :)

Comment 31

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 812768 [details]
PR with the selection based on visibility monitor

Added a few more minor comments on Github.  Once those are addressed and travis is green this should be good to land, so r+.  (Hopefully current travis breakage will be resolved by tomorrow.)

Thanks Francisco!

Comment 32

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

Landed:

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

Comment 33

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

@Ben, FYI, I added the latest suggestions you commented :)

Comment 34

[John Ford [:jhford] CET/CEST Berlin Time]

I was not able to uplift this bug to v1.2.  If this bug has dependencies which are not marked in this bug, please comment on this bug.  If this bug depends on patches that aren't approved for v1.2, we need to re-evaluate the approval.  Otherwise, if this is just a merge conflict, you might be able to resolve it with:

  git checkout v1.2
  git cherry-pick -x -m1 c4412deb4fbbae5baefb10abeac1c9bd729606e3
  <RESOLVE MERGE CONFLICTS>
  git commit

Comment 35

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

Landed in v1.2:

https://github.com/mozilla-b2g/gaia/commit/9b4abd6644981d399a838991165b6a525ef6bb0d

Thanks!

Comment 36

[John Ford [:jhford] CET/CEST Berlin Time]

(In reply to Francisco Jordano [:arcturus] from comment #35)
> Landed in v1.2:
> 
> https://github.com/mozilla-b2g/gaia/commit/
> 9b4abd6644981d399a838991165b6a525ef6bb0d
> 
> Thanks!

Marking as landed on branch.