848188 - [Contacts] Use cursor getAll to fetch contacts instead of the find method

Status: RESOLVED FIXED

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

Description

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

Use the new cursor api to fetch the contacts and start the contacts list painting sooner.

Comment 1

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

Attachment: WIP - Contacts using cursor to paint in chunks ASAP

Hi,

We get a huge improvement using the cursor, but we have to find the balance between painting fast and having a usable and scrollable app while painting.

Doing some measures:

{
  "stats": {
    "suites": 2,
    "tests": 2,
    "passes": 2,
    "pending": 0,
    "failures": 0,
    "start": "2013-03-06T00:25:23.055Z",
    "end": "2013-03-06T00:27:43.407Z",
    "duration": 140352,
    "application": "communications/contacts"
  },
  "failures": [],
  "passes": [
    {
      "title": "startup time ",
      "fullTitle": "communications/contacts > startup time ",
      "duration": 65203,
      "mozPerfDurations": [
        798,
        786,
        787,
        775,
        769
      ],
      "mozPerfDurationsAverage": 783
    },
    {
      "title": "rendering time > last chunk",
      "fullTitle": "communications/contacts > rendering time > last chunk",
      "duration": 74612,
      "mozPerfDurations": [
        1921.752931000001,
        1953.796386,
        1773.9868150000002,
WIP Bug-848188
        1897.369385,
        1989.2883310000016
      ],
      "mozPerfDurationsAverage": 1907.2387696000005
    }
  ]
}
]

We needed 6 seconds to paint the whole list before, now 1.9. Unfortunately don't know why the measure for the first chunk is not taken, will need to take a look tomorrow.

Alberto any idea or suggestion? I've been trying to play with chunk size, initial buffer and so on.

Comment 2

[alberto.pastor]

I think there is a bug with the "first-chunk" test. Julien and Rik probably are the people you want to talk with.

Comment 3

[Dietrich Ayala (:dietrich)]

Blocking a blocker -> blocking+.

Comment 4

[Andrew Overholt [:overholt]]

Seems you're the owner here, Francisco :)

Comment 5

[Andrew Overholt [:overholt]]

How are things going here?

Comment 6

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

We tried several things, right now we will try to propose a patch mix from the WIP before and some work from Alberto.

Comment 7

[alberto.pastor]

We have been trying to integrate cursors with contacts app, but we are having some issues:

- We receive the first contact after 400msec for 1000 contacts, so the idea is start painting each chunk (we also tried contact by contact) on every new contact on the cursor. The problem is that the CPU is so busy requesting the remaining contacts (even if we stop calling continue()) that the scrolling is very irresponsive. 

Here the patch:

https://github.com/albertopq/gaia/tree/contacts-init-time

Here the profile:

http://people.mozilla.com/~bgirard/cleopatra/#report=8b34f2bc5aabaaf761316ef95dd496621645f78a


Even if we only paint 1 chunk, scrolling on that one while keep receiving contacts through the cursor kills the scrolling.

We will need Platform's help.

Comment 8

[Reuben Morais [:reuben]]

Alberto, can you apply the patch in bug 844558 and see if it helps?

Comment 9

[alberto.pastor]

(In reply to Reuben Morais [:reuben] from comment #8)
> Alberto, can you apply the patch in bug 844558 and see if it helps?

Hi Reuben, I can't apply the patch in b2g-18. I manually applied it, but it seems it doesn't work (I don't get any contacts).

Comment 10

[Gregor Wagner [:gwagner]]

We debugged this problem in the last few days.

We fixed bug 851741 that caused the cursor.continue() call to be sync and we didn't return to the event loop. So we still got all the contacts before we actually started rendering. The original profile looked like:

http://people.mozilla.com/~bgirard/cleopatra/#report=351fb5175bfd7e2e3d149328fd6e433779fcbdc2

With this bug fixed we get now:
http://people.mozilla.com/~bgirard/cleopatra/#report=e14c29651aebe54239ac37182033c3e3d3910633

We still get many reflows that take a long time even if we don't scroll.

We also had to fix the gaia code a little bit because the async continue didn't work. I will attach my current diff.

Comment 11

[Gregor Wagner [:gwagner]]

Attachment: patch

Comment 12

[alberto.pastor]

Quick update on this, yesterday I tested :gwagner patches, and it stills blocks the UI even if we are not rendering but calling cursor.continue(). The situation is that if start using getAll instead find, we are going to be able to render the first chunk faster, but it will take longer on making the app usable.

Comment 13

[Dietrich Ayala (:dietrich)]

Thanks Alberto. What's the next step?

Comment 14

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Hi Francisco,

May I confirm with you that this issue is about trying to reduce the initial start up time of Contact app?
It is not related to the contact list scrolling performance, right?

Thank you.

Comment 15

[alberto.pastor]

Hi! Francisco is on holidays, so I'll take this. The goal is trying to show faster the first chunk, regardless the number of contacts you have. The issue is that, even if we only render 1 chunk and just keep requesting the remaining contacts (without rendering anymore) the UI is blocked due the CPU being busy requesting more contacts. I sent reuben and gwagner a patch for doing that test, but I didn't have any reply back. I think next steps are Gregor/Reuben talking with me all together and trying to find a balanced solution between gaia and platform.

Comment 16

[Gregor Wagner [:gwagner]]

(In reply to Alberto Pastor [:albertopq] from comment #15)
> Hi! Francisco is on holidays, so I'll take this. The goal is trying to show
> faster the first chunk, regardless the number of contacts you have. The
> issue is that, even if we only render 1 chunk and just keep requesting the
> remaining contacts (without rendering anymore) the UI is blocked due the CPU
> being busy requesting more contacts. I sent reuben and gwagner a patch for
> doing that test, but I didn't have any reply back. I think next steps are
> Gregor/Reuben talking with me all together and trying to find a balanced
> solution between gaia and platform.

We have this in perf now as well and it just shows that the reflows are super expensive. I don't think the CPU is busy with transferring contacts. Can you send me your code again that I should test?

Comment 17

[alberto.pastor]

Which reflows do you mean? In the code I sent you we just render 1 chunk, so just 1 reflow is done. You cannot scroll that chunk until you receive the last contact.

https://github.com/albertopq/gaia/tree/test-only-rendering-50

Comment 18

[Joe Cheng [:jcheng] (please needinfo)]

seem like [:arcturus] is on PTO. and [:albertopq] is taking over. do you want to take this bug [:albertopq]? thanks

Comment 19

[Gregor Wagner [:gwagner]]

(In reply to Alberto Pastor [:albertopq] from comment #17)
> Which reflows do you mean? In the code I sent you we just render 1 chunk, so
> just 1 reflow is done. You cannot scroll that chunk until you receive the
> last contact.
> 
> https://github.com/albertopq/gaia/tree/test-only-rendering-50

The patch in bug 855015 fixes the delay problem in the beginning. So we constantly get contacts now but the profiler still shows a lot of time spent in the refresh driver.:
http://people.mozilla.com/~bgirard/cleopatra/#report=f40ce05ae2c168811a788582cb5d37f70d579f50

So here I increased the number of contacts to render from 50 to 500.
Towards the end of the profile I try to scroll.

Comment 20

[alberto.pastor]

What's the point of increasing to 500? What we want to achieve is rendering a small chunk and being able to scroll it while we are requesting more contacts. Of course rendering 500 contacts is expensive. I'm going to be in PTO so I would suggest someone in Moz taking this bug as it's being delayed for working in different timezones. If we achieve request chunks and render them asynchronously while requesting more (as SMS is being able to do) that will work. If we block the UI until we have all the contacts, I would prefer keep using "find" method without cursors (and may be find a solution for locally storing the 1st chunk for not delaying the startup when a lot of contacts)

Comment 21

[Josh Carpenter [:jcarpenter]]

We may also want to consider prioritizing loading of user favorites, and then populating the remainder of the list. Favorites are more likely to be accessed than non-Favorites, and they appear at the top of the list. It seems like an elegant approach, so long as the user actually uses the Favorites feature.

Comment 22

[Andrew Overholt [:overholt]]

Any thoughts here, Gregor?  See Alberto's question in comment 20.  (FWIW, I'm willing to bet you two have caught up via other means of communication :)

Comment 23

[Andrew Overholt [:overholt]]

(Sorry, it seems like we're just blocked here on bug 855015)

Comment 24

[Alex Keybl [:akeybl]]

Moving back to tef? until we hear back about https://bugzilla.mozilla.org/show_bug.cgi?id=846895#c29

Comment 25

[alberto.pastor]

Attachment: Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/9121

Pointer to Github pull-request

Comment 26

[alberto.pastor]

Results after the patch (500 contacts):

https://www.dropbox.com/s/rsjwbojv2uk0fqs/IMG_1657%20-%20Broadband.m4v

{
  "stats": {
    "suites": 2,
    "tests": 2,
    "passes": 2,
    "pending": 0,
    "failures": 0,
    "start": "2013-04-11T17:31:26.813Z",
    "end": "2013-04-11T17:33:40.378Z",
    "duration": 133565,
    "application": "communications/contacts"
  },
  "failures": [],
  "passes": [
    {
      "title": " second-chunk",
      "fullTitle": "communications/contacts >  second-chunk",
      "duration": 75706,
      "mozPerfDurations": [
        1042.205809999985,
        1113.7695300000014,
        1251.8615730000001,
        1249.2370630000005,
        1411.0412570000008
      ],
      "mozPerfDurationsAverage": 1213.6230465999975
    },
    {
      "title": " startup-path-done",
      "fullTitle": "communications/contacts >  startup-path-done",
      "duration": 75706,
      "mozPerfDurations": [
        3923.9196780000057,
        3639.465333,
        3607.4523919999992,
        3687.1337920000005,
        4256.469726000001
      ],
      "mozPerfDurationsAverage": 3822.888184200002
    },
    {
      "title": "startup time ",
      "fullTitle": "communications/contacts > startup time ",
      "duration": 57330,
      "mozPerfDurations": [
        783,
        798,
        859,
        791,
        784
      ],
      "mozPerfDurationsAverage": 803
    }
  ]
}
]

Note we are measuring the "second chunk" rendering as the performance tests doesn't have enough time to start listening to the first one. The time of the first chunk should be near the startup time.

Comment 27

[alberto.pastor]

As we are caching the first chunk, we need to fix it if it has changed. I've uploaded some videos on how it behaves when creating, editing or removing contacts that are previously cached.

https://www.dropbox.com/sh/ens54m35yox01sw/xJWx5rum-L#/

I think is worth showing incorrect information at the very beginning given the time we save, and correct it later (taking in account the screenshot is already incorrect as well), but I would like UX taking a look and giving feedback.

Josh, could you please share your thoughts on this? All the videos are recorded with 500 contacts.

Comment 28

[Josh Carpenter [:jcarpenter]]

Thanks Alberto. I'll review then let's discuss tomorrow at the office.

Comment 29

[Josh Carpenter [:jcarpenter]]

Looks promising, Alberto. I also really like the scrolling performance improvements! Let's talk more on Monday.

Comment 30

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

New version of the patch working well, despite it doesn't include the improvement of the first cached chunk, but, IMHO, we should do this in a separate bug.

r plusing this patch for the use of cursors.

Comment 31

[alberto.pastor]

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

Comment 32

[Michael Vines [:m1] [:evilmachines]]

(setting right bug flags for uplift)

Comment 33

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

I was not able to uplift this bug to v1-train and v1.0.1.  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-train and v1.0.1, 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-train
  git cherry-pick -x -m1 dc5b1185cd210e45c75d420593467b22b3d73f69
  <RESOLVE MERGE CONFLICTS>
  git commit
  git checkout v1.0.1
  git cherry-pick -x $(git log -n1 v1-train)

Comment 34

[alberto.pastor]

Landed in v1-train

v1-train: 73346747fdf8b4b57e905517b7e7c4ee3a57bae6

but  for landing on v1.0.1, we need to merge first

Bug 848771

Comment 35

[alberto.pastor]

v1.0.1: cf87a67e752a945bd502c1a30db17bdd74f2ca2b

Comment 36

[sarsenyev]

Can you please provide steps to verify this fix - as we will blackbox test from the UI?

Comment 37

[Reuben Morais [:reuben]]

The only way this is visible in the UI is by measuring the time before the first contacts are displayed, this patch shouldn't change any other behaviors.

Comment 38

[sarsenyev]

As per comment # 37, After downloading 431 contacts from Facebook, measuring time before the first contact displayed is 794 milliseconds on Inari device and  742 milliseconds on Leo device

Environmental  Variables:
Inari Build ID: 20130502070201
Kernel Date: Feb 21
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/5c1d67e0c242
Gaia: 11477c127ae9be5051e4cfbcbf3da1d4150f9967

Environmental  Variables:
Unagi Build ID: 20130502070205
Kernel Date: Apr 25
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/499c3627e89a
Gaia: 495c46489eb256be598a19ea54d7837ce4fc385b

Comment 39

[sarsenyev]

As per comment 38, Inari and Leo devices used for the test

Comment 40

[Cody Roesch [:croesch]]

Information provided from our test devices. Not sure what targets were but this is something to go on.

Comment 41

[Deepa Subramanian]

The user experiences faster and smoother scroll on Inari device consistently averaging 55 frames/sec while on Unagi, it was lower in numbers. When the user Kills/restarts the Unagi device and launch the contact Apps again, the user experiences the slow scrolling and a small pause during scrolling.

Please see the video http://www.youtube.com/watch?v=r94XCH3X6zE for more info.

Inari Build ID: 20130603070208
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/42555e1e72fa
Gaia: fcae23654296c9cc645c2b7e77a2c36bf494803a

Unagi Build ID: 20130604070205
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/997cdbf5d012
Gaia: 5534304aee934055f08f21ce93261ba2a596516a