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)
Tracking
(blocking-b2g:koi+, 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.
Comment 1•11 years ago
|
||
We are observing FPS numbers of 44 fps for the following build: Gecko: 964f7c844f2793092bab72612d5da2f5bcfd6c2e Gaia: be4ea00a50236b10eb0a03232a28ffd0048e0cb8
Assignee | ||
Comment 2•11 years ago
|
||
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.
Comment 3•11 years ago
|
||
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.
Assignee | ||
Comment 4•11 years ago
|
||
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 5•11 years ago
|
||
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+
Assignee | ||
Comment 6•11 years ago
|
||
Thanks Francisco! Travis is also green now. Landed: https://github.com/mozilla-b2g/gaia/commit/38d26104bbb97b46207aacd46aeb2e8d3b3e77eb
Assignee | ||
Updated•11 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Comment 7•11 years ago
|
||
Ben, What is the acceptable number if ~20ms is long? Please provide risk analysis of the fix.
Flags: needinfo?(bkelly)
Assignee | ||
Comment 8•11 years ago
|
||
(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)
Comment 9•11 years ago
|
||
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
Assignee | ||
Comment 10•11 years ago
|
||
(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.
Comment 11•11 years ago
|
||
Ben you can ask for gaia approval
Updated•11 years ago
|
blocking-b2g: koi? → koi+
Updated•11 years ago
|
Severity: normal → blocker
Whiteboard: [c=handeye p=3 s= u=] → [c=handeye p=3 s= u=1.2]
Comment 12•11 years ago
|
||
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)
Assignee | ||
Comment 13•11 years ago
|
||
Uplifted to v1.2: 4967b8daaf4c6362c6c3987815268d0ba37ec385
Flags: needinfo?(bkelly)
You need to log in
before you can comment on or make changes to this bug.
Description
•