Closed Bug 848188 Opened 7 years ago Closed 7 years ago

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

Categories

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

x86
macOS
defect
Not set

Tracking

(blocking-b2g:tef+, b2g18 fixed, b2g18-v1.0.1 fixed)

RESOLVED FIXED
blocking-b2g tef+
Tracking Status
b2g18 --- fixed
b2g18-v1.0.1 --- fixed

People

(Reporter: arcturus, Assigned: arcturus)

References

Details

(Whiteboard: [FFOS_perf])

Attachments

(2 files, 1 obsolete file)

Use the new cursor api to fetch the contacts and start the contacts list painting sooner.
blocking-b2g: --- → tef?
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.
Attachment #721534 - Flags: feedback?(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.
Blocking a blocker -> blocking+.
blocking-b2g: tef? → tef+
Seems you're the owner here, Francisco :)
Assignee: nobody → francisco.jordano
How are things going here?
We tried several things, right now we will try to propose a patch mix from the WIP before and some work from Alberto.
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.
Flags: needinfo?(anygregor)
Alberto, can you apply the patch in bug 844558 and see if it helps?
(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).
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.
Flags: needinfo?(anygregor)
Attached patch patchSplinter Review
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.
Whiteboard: [FFOS_perf]
Thanks Alberto. What's the next step?
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.
Flags: needinfo?(francisco.jordano)
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.
Flags: needinfo?(francisco.jordano)
Depends on: 855015
(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?
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
seem like [:arcturus] is on PTO. and [:albertopq] is taking over. do you want to take this bug [:albertopq]? thanks
(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.
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)
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.
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 :)
(Sorry, it seems like we're just blocked here on bug 855015)
Moving back to tef? until we hear back about https://bugzilla.mozilla.org/show_bug.cgi?id=846895#c29
blocking-b2g: tef+ → tef?
Attachment #736366 - Flags: review?(francisco.jordano)
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.
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.
Flags: needinfo?(jcarpenter)
blocking-b2g: tef? → tef+
Thanks Alberto. I'll review then let's discuss tomorrow at the office.
Flags: needinfo?(jcarpenter)
Looks promising, Alberto. I also really like the scrolling performance improvements! Let's talk more on Monday.
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.
Attachment #721534 - Attachment is obsolete: true
Attachment #721534 - Flags: feedback?(alberto.pastor)
Attachment #736366 - Flags: review?(francisco.jordano) → review+
https://github.com/mozilla-b2g/gaia/commit/dc5b1185cd210e45c75d420593467b22b3d73f69
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
(setting right bug flags for uplift)
Flags: needinfo?(jhford)
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)
Flags: needinfo?(jhford)
Landed in v1-train

v1-train: 73346747fdf8b4b57e905517b7e7c4ee3a57bae6

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

Bug 848771
Depends on: 848771
No longer depends on: 855015
v1.0.1: cf87a67e752a945bd502c1a30db17bdd74f2ca2b
Can you please provide steps to verify this fix - as we will blackbox test from the UI?
Flags: needinfo?
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.
Flags: needinfo?
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
As per comment 38, Inari and Leo devices used for the test
Information provided from our test devices. Not sure what targets were but this is something to go on.
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
You need to log in before you can comment on or make changes to this bug.