894611 - contacts app could further delay DOM placeholder creation

Status: RESOLVED WONTFIX

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

Description

[Ben Kelly [:bkelly, not reviewing]]

With the integration of visibility_monitor (bug 879299) the contacts app would create empty li elements for each contact during load.  These empty placeholders are added to the various group lists and then they are rendered with text when the monitor indicates they are visible.  While this helps defer much of the rendering work, we still have to pay the price of creating and adding the placeholder nodes.

The contacts app could instead defer the creation of the placeholders.  This will help improve load performance at the cost of additional complexity.  Prototyping has shown that this is probably worth it, reducing total load time for 1000 contacts from 7.5s down to 6.5s.

Pull request to come shortly.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

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

This pull request implements deferred placeholder creation.

In order to manage the complexity I broke most of the logic out into a separate module; contacts_list_queue.js.  This "queue" tracks contacts that have not been placed in the DOM yet in ordered lists by group.  It provides functionality for flushing to the DOM at a rate reasonable for the device, fixing scroll positions automatically, etc.

Separate tests includes for the new queue module.

I also added a large comment to contacts_list.js describing the overall load process:

  https://github.com/mozilla-b2g/gaia/pull/11017/files#L1R496

Finally, this code uses dbaron's setZeroTimeout() for faster "next tick" operations.  Since this is also used over in calendar I decided to place this in shared/js as a separate module.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

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

Another change in this pull request:

It also removes uses of node.dataset from contacts_list.js.  Using getAttribute() and setAttribute() gained us about 200ms improvement on load time.  So in this case I think its worth using the functions instead of the convenience dataset mapping.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

By the way, I asked dbaron his thoughts on adding setZeroTimeout() to shared/js.  He was neutral, neither objecting nor strongly endorsing the idea.  Here is his original blog post:

  http://dbaron.org/log/20100309-faster-timeouts

Comment 4

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

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

Hi Ben,

sorry for the delay, just did a couple of comments on the PR but nothing important.

Tried on the phone and looking good, the heavy workload is taking a while to load the whole list ... but thats what you are tackling with this PRs.

Btw, you will need to rebase, I did the tests directly against your branch as with the time passed it's not mergeable with master.

Thanks a lot!
F.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Thanks for the review Francisco!

I expanded one of the comments to make it clear the queue will not remove duplicate contacts.  I think an expanded build customizer for different devices might be a larger project.  The code won't prevent that, but I'd rather tackle that in the future.

Since you said your comments were minor I went ahead and merged:

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

Comment 6

[Jose Manuel Cantera]

First of all my apologies for not having time for reviewing this. I was swamped with new US for v1.2. Unfortunately this bug has to be backed out because it introduces significant regressions. For instance, FB info on the contacts list is never shown. 

My concern here is that with this bug we are entering into the trap of trying to make something good enough perfect. And perfect is the best enemy of good enough. 

I would like to know what is the gain in terms of performance and if adding all these complexity (provided you make it work) does it worth. 

thanks

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Attachment: contacts_defer_ph_fix.patch

First, let me apologize for and address the breakage of facebook rendering.  There were two causes in the code:

1) I introduced a bug when converting from element.dataset to element.setAttribute().  As I wrote in comment 2 this saves us on the order of hundreds of milliseconds during the load of 1000 contacts, so it seems worth the loss of convenience.  When I made the change, however, I did a mechanical s/dataset.foo/setElement('data-foo')/g which is not always correct.  Particularly, dataset.fbUid should be translated to setElement('data-fb-uid') due to the camel case conversion rules.  Correcting this gets some of the FB data to show.

2) The other problem was that when the nodes were flushed to the DOM the image loader was not notified to update its internal state information.  Adding an imgLoader.reload() after the flush solves this.

This patch corrects these two issues.  If we decide to proceed with this work I will incorporate it into a new pull request.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Now I will attempt to address or at least give my view on your other concerns.

(In reply to Jose M. Cantera from comment #6)
> My concern here is that with this bug we are entering into the trap of
> trying to make something good enough perfect. And perfect is the best enemy
> of good enough. 
> 
> I would like to know what is the gain in terms of performance and if adding
> all these complexity (provided you make it work) does it worth. 

So my impression has been that we are still not "good enough" because of the behavior in bug 865747.

Thats a long bug, so let me summarize it here.  We show the first few contacts quickly, but the user cannot jump to a letter group like "T" or "W" until the end of the contacts list is loaded.  Right now the complete load takes many seconds which makes things very confusing for the user.

Our benchmark for this is 1000 contacts from the reference-workload-heavy data set.  The timings can vary by about +/- 500ms, but the rough values for the full load have been:

1) Original time for bug 865747:  ~12.5 seconds (much longer if any scrolling)
2) After bug 879299:              ~8.5  seconds
3) After bug 892155:              ~8    seconds
4) After this work:               ~6.5  seconds

If we look at bug 888666 we can see that it takes ~3.5 seconds to just get the 1000 contacts out of the mozContacts API without any client work.  This means without this patch we are spending ~4.5 seconds (8 - 3.5 = 4.5) just processing and getting the data into the DOM.  This patch reduces this client-side work to ~3 seconds.

If/when we eventually implement client side caching of the contacts we still need to perform all of this client-side work.  (Note, I have attempted to make all of these changes friendly to ultimately receiving the contacts from a different source, such as a cache.  You simply just feed each new contact into loadContact() and it should all work.)

The target numbers I have been given is that the app should be functional within 1.5 seconds with 1000 contacts in the database.  Since the jump scrolling is non-functional until the full load completes I believe a reduction from ~4.5 second to ~3 seconds in full load time is worth the additional complexity.

I hope this provides some assurance that we are not just adding complexity without thought.  Please let me know what you think of these numbers and our goals for fixing bug 865747.

Thank you.

Comment 9

[Jose Manuel Cantera]

thanks, Ben for the detailed explanation. Please provide a new patch and I will review it. However, It will be from next week on.

thanks

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Jose M. Cantera from comment #9)
> thanks, Ben for the detailed explanation. Please provide a new patch and I
> will review it. However, It will be from next week on.

Thanks Jose.  I have to admit after working on anothe pull-request and re-testing this morning I am less certain about this change now.  I need to evaluate it a bit more.

My main concern is that while it improves load time, it does move a lot of work to the scrolling activity.  Scrolling does seem less smooth at times.  Unfortunately I don't have a good. objective way to measure if the scrolling is good enough or not.  Unfortunately it seems like our good scroll measurement tools (like eideticker) only work on committed code.  I have not had any luck with the settings fps option since it seems to dramatically impact performance on my buri.

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

More information regarding my uncertainty over scroll performance:

This code is sensitive to the DEFAULT_FLUSH_SIZE constant in the queue.  If its too large then we can see jank while scrolling.  If its too small, though, then it takes too long to flush the items to the DOM and you may see visual artifacts while scrolling (especially if scrolling backwards since the flush happens front-to-back).

A flush size of 30 to 40 works ok (not well, but ok) for the reference-workload-heavy.  I'm worried, though, that this setting may be too sensitive to actual list length and would not be correct for other sets of contacts.  For example, if we the user had 80% contacts in the 'J' group making it very long it would be more likely to cause visual artifacts.

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

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

Here is an updated pull request that fixes the FB support.

I'm not request r? at this time, though, based on my above concerns regarding the scrolling.  Considering this I am sorry I landed that previous version.  I think it was a case of spending too much time working on the patch and getting used to its scroll performance.  Again, it is "ok" to me, but still slightly janky and probably would get worse with larger lists.

I'm basically leaving this here for us to evaluate further as we try to improve load times.

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

In an attempt to better quantify the scrolling I enabled the fps overlay in the settings.

The values jump around a lot, but the rough impression I got during scrolling after loading completes:

  Before bug 879299:   40 to 45fps
  After bug 879299:    38 to 42fps
  After this patch:    28 to 35fps

So this patch does seem to have a significant scroll penalty.

Comment 14

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

Hi Ben,

Thanks for the measures, do you know if there is a way of doing this measures in the fps not just by looking at the number?

Thanks,
F.

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Francisco Jordano [:arcturus] from comment #14)
> Thanks for the measures, do you know if there is a way of doing this
> measures in the fps not just by looking at the number?

Not that I'm aware of at the moment.  Those numbers I provided are very rough since the fps overlay jumps around a lot during scrolling.

It might be possible to hack up the gecko code that does the overlay to output to logcat instead.  Ideally I'd like something that starts collecting fps samples when scrolling begins and then logs min, max, mean, and stdev when scrolling ends.

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

With the current plans to use datastore and cursors in contacts app in the future, I don't think this bug will ever be implemented.  Marking as WONTFIX.