Closed Bug 936908 Opened 11 years ago Closed 11 years ago

contacts doRenderTimer() can still take too long (~20ms)

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

(blocking-b2g:koi+, b2g-v1.2 fixed)

RESOLVED FIXED
blocking-b2g koi+
Tracking Status
b2g-v1.2 --- fixed

People

(Reporter: bkelly, Assigned: bkelly)

Details

(Keywords: perf, Whiteboard: [c=handeye p=3 s= u=1.2])

Attachments

(1 file)

From profiling contacts app for bug 936535 it appears that doRenderTimer() can still take ~20ms sometimes which is too long.  Profile is here:

  http://people.mozilla.org/~bgirard/cleopatra/#report=ee1e36d8ca0f3e8432bdc46ed6ce33a28480a5f3

This work was broken up a bit in bug 925348, but apparently it was not enough.

I need to investigate further, but initial profiling suggests we can get some decent improvements just by removing the code here which supports rendering contacts in batches.  The change for bug 925348 essentially made doRenderTimer() operate on a single contact at a time, but we still use a timer, allocate an array, etc.

My quick test to remove the timer portion seemed to show significant improvement:

  http://people.mozilla.org/~bgirard/cleopatra/#report=31f586bddfb61fd6241af7b2b6c2f4ae4118285b

I don't fully understand why this helps as much as it does yet.  One idea is that modifying the DOM from the mutation handler directly allows gecko to consolidate some painting/style/layout work since its already in the midst of scrolling.

Note, I still see one doRenderTimer() call taking 50ms here, but this appears to be caused by the b2g parent process is performing an animation at the same time.  I'm guessing this is the FixedHeader translation.
We are observing FPS numbers of 44 fps for the following build:
Gecko:
 964f7c844f2793092bab72612d5da2f5bcfd6c2e
Gaia:
 be4ea00a50236b10eb0a03232a28ffd0048e0cb8
Vikram, I assume you guys are using a high speed camera for your measurements, correct?  I'm making do with out compositor reporting at the moment.
In reply to https://bugzilla.mozilla.org/show_bug.cgi?id=936908#c2:
> Vikram, I assume you guys are using a high speed camera for your measurements, correct?  I'm making do
> with out compositor reporting at the moment.
Yes, we are.
Francisco, this pull request reduces the amount of time spent rendering contacts at a single time in order to avoid jank.  It does this by removing the onscreen and offscreen timer batching mechanism.

While the monitor would notify us about each row individually, the timer would get delayed enough during heavy load allowing up to 4 or 5 contacts to get batched.  By removing the timer we help ensure that only a single contact gets rendered at a time.

This also decreases the monitor scroll delta so that it will detect we need a new contact render sooner.
Attachment #830603 - Flags: review?(francisco.jordano)
Comment on attachment 830603 [details] [review]
Pull request at https://github.com/mozilla-b2g/gaia/pull/13602

Tried on the phone, working for me perfectly.

Also unit tests passing in local, rerunning travis, but I think travis is again a bit fuzzy so ok to merge for me.

Thanks a lot Ben!
Attachment #830603 - Flags: review?(francisco.jordano) → review+
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Ben,

What is the acceptable number if ~20ms is long? Please provide risk analysis of the fix.
Flags: needinfo?(bkelly)
(In reply to Preeti Raghunath(:Preeti) from comment #7)
> What is the acceptable number if ~20ms is long? Please provide risk analysis
> of the fix.

Given that our goal is 60fps we need to draw a frame once every 16ms.  In order to allow some time budget for drawing overhead, I'd like this to be 5ms or less.  I believe this patch puts us in the 5ms to 10ms range.  Not ideal, but an improvement.

This is a low risk change.  Previously we waited a little bit every time we got notified a contact came "onscreen" so we could batch all the rendering together.  This patch simply performs the same rendering code immediately instead of batching them up in a separate timer call.
Flags: needinfo?(bkelly)
I wouldn't block on it for 1.2. 1.3 makes sense but given the junction of the release 1.2 I am not keen on taking for it 1.2
(In reply to Preeti Raghunath(:Preeti) from comment #9)
> I wouldn't block on it for 1.2. 1.3 makes sense but given the junction of
> the release 1.2 I am not keen on taking for it 1.2

This is directly work I did for the koi+ blocked bug 936535 and bug 937425.  I'm a little confused why this is not koi+ as well.
Ben you can ask for gaia approval
blocking-b2g: koi? → koi+
Severity: normal → blocker
Whiteboard: [c=handeye p=3 s= u=] → [c=handeye p=3 s= u=1.2]
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 38d26104bbb97b46207aacd46aeb2e8d3b3e77eb
  <RESOLVE MERGE CONFLICTS>
  git commit
Flags: needinfo?(bkelly)
Uplifted to v1.2:   4967b8daaf4c6362c6c3987815268d0ba37ec385
Flags: needinfo?(bkelly)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: