Closed Bug 1170538 Opened 10 years ago Closed 10 years ago

(nga-performance) Audit and squeeze every ounce of performance from the virtual-list toolkit

Categories

(Firefox OS Graveyard :: Gaia, defect)

Product:
Firefox OS Graveyard
Component:
Gaia
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: etienne, Unassigned)

References

Details

The toolkit component is still under development and lives in the dom-scheduler repo [1] for now. But we already have a demo app running [2] to start profiling. The current implementation is not bad, but I'm sure we would benefit a lot from a performance audit by someone with deeper platform knowledge. Not sure what the best way to proceed is but I'm happy to help test any tweak/suggestions that might come out of this! [1] https://github.com/etiennesegonzac/dom-scheduler [2] https://github.com/etiennesegonzac/dom-scheduler/tree/master/demo-app
I'm not sure what use cases you want to profile, could you please list?
(In reply to Ting-Yu Chou [:ting] from comment #1) > I'm not sure what use cases you want to profile, could you please list? Ranked by priority: * scrolling the list * toggling the edit mode * re-ordering the list
* Just scroll. http://people.mozilla.org/~bgirard/cleopatra/#report=7facf52aabfaa0c48a6d40acbe5e5b0d9c8de353 * Toggled edit mode on and off once. http://people.mozilla.org/~bgirard/cleopatra/#report=5c7cb9d4a39000f9809d3babf0e59993de137be1 * Dragged a list item down to a position, the items shifted up, released finger. http://people.mozilla.org/~bgirard/cleopatra/#report=1f1ea2a213ac6b39ebaeaff15ae0d569e90ba8fd Gaia Revision 4d1f2a45c252b9a45028d280813dd5113de35fcb Gaia Date 2015-06-08 03:37:18 Gecko Revision https://hg.mozilla.org/mozilla-central/rev/7d4ab4a9febd Gecko Version 41.0a1 Device Name hammerhead Firmware(Release) 5.1 Will check the profiles later.
(In reply to Ting-Yu Chou [:ting] from comment #3) > Will check the profiles later. Cool! Standing by, ready to implement any suggestion :)
(In reply to Ting-Yu Chou [:ting] from comment #3) > * Just scroll. > http://people.mozilla.org/~bgirard/cleopatra/#report=7facf52aabfaa0c48a6d40acbe5e5b0d9c8de353 43.5% of time the content process main thread is idle and FPS is ~60 from HUD, it scrolls quite smooth. There's no new item was inserted in this profile. I noticed two things: - 11.9% PLayerTransaction::SendUpdate() Synchronous IPC PLayerTransaction::SendUpdate() is blocked since compositor thread is busy doing CompositorParent::CompositeToTarget(). For instance Refresh21 [2080, 2140], the SendUpdate() blocks main thread for 33ms. The JS updates style.transform of <li> to change its postion [1]. Jerry, in this case, is the sync SendUpdate() expected? - 4.5% list.prototype.render/renderItem() @ scheduled-list.js Sometimes the items have been placed in previous render call already, should be able to skip them by comparing startIndex and endIndex if no item is reordered or inserted. [1] https://github.com/etiennesegonzac/dom-scheduler/blob/master/lib/scheduled-list.js#L559
Flags: needinfo?(hshih)
Here are the ops using SendUpdate(). https://dxr.mozilla.org/mozilla-central/search?q=AddEdit&case=false For this case https://github.com/etiennesegonzac/dom-scheduler/blob/master/lib/scheduled-list.js#L559, It will use sync ipc call. Since we change the layer position, I think it's make sense to use sync op. https://hg.mozilla.org/mozilla-central/annotate/95afddf894e3/gfx/layers/ipc/ShadowLayers.cpp#l573 Could we use css animation instead of setting the transform?
Flags: needinfo?(hshih) → needinfo?(janus926)
(In reply to Ting-Yu Chou [:ting] from comment #3) > * Toggled edit mode on and off once. > http://people.mozilla.org/~bgirard/cleopatra/#report=5c7cb9d4a39000f9809d3babf0e59993de137be1 In Rasterize#3 I see nsDisplayText::Paint(), seems the list item and its overlay are merged to the same layer. I tried to add |will-change: opacity;| to |li .overlay| per Jeff's suggestion to make them separated to two layers, but I didn't notice any peformance difference. The point here is the touchend event listener [1] makes changes by promises and both toggleTransitioning() and toggleEditMode() resolves when transitionend. Note the button has transition duration 0.1s, and the overlay has 0.2s, i.e., in best case the overlay will be done painting 0.3s after toggling the button. So I removed the transitionend event waiting for toggleTransitioning(), then the overlay comes up faster and feels better. When CSS transition have duration >0, waiting for transistionend for each change will make things happen in sequence and longer. We should make sure whether that's what we want. [1] https://github.com/etiennesegonzac/dom-scheduler/blob/master/demo-app/js/app.js#L75
(In reply to Jerry Shih[:jerry] (UTC+8) from comment #6) > Could we use css animation instead of setting the transform? I am not sure. Etienne? Jeff, if this change avoids synchronous SendUpdate(), I guess the LayerTransaction will happen more often and we won't see white screen easily when scroll fast, is my understanding correct?
Flags: needinfo?(janus926) → needinfo?(hshih)
(In reply to Ting-Yu Chou [:ting] from comment #8) > Jeff, if this change avoids synchronous SendUpdate(), I guess the > LayerTransaction will happen more often and we won't see white screen easily > when scroll fast, is my understanding correct? Sorry I meant Jerry, not Jeff. Anyway, I've talked to Jerry and he said using animation can let compositor to handle the movement, and usualy get better performance. But need Etienne to clarify whether can it be used. I am thinking whether we can make PLayerTransaction::SendUpdate() happens more often and not blocked if it is necessary, so probably we won't see white screen easily.
Flags: needinfo?(hshih)
(In reply to Ting-Yu Chou [:ting] from comment #7) > When CSS transition have duration >0, waiting for transistionend for each > change will make things happen in sequence and longer. We should make sure > whether that's what we want. I modified the code a bit [1], and the toggling runs smoother. https://github.com/etiennesegonzac/dom-scheduler/compare/master...janus926:bug-1170538-toggle
(In reply to Ting-Yu Chou [:ting] from comment #5) > - 4.5% list.prototype.render/renderItem() @ scheduled-list.js > Sometimes the items have been placed in previous render call already, > should be able to skip them by comparing startIndex and endIndex if no item > is reordered or inserted. Adding it to my todo!
(In reply to Ting-Yu Chou [:ting] from comment #8) > (In reply to Jerry Shih[:jerry] (UTC+8) from comment #6) > > Could we use css animation instead of setting the transform? > > I am not sure. Etienne? We're transitioning to computed position not known in advance. So we'd have to generate keyframes in JS to switch to animations, would the gain be worth the added complexity?
(In reply to Ting-Yu Chou [:ting] from comment #10) > (In reply to Ting-Yu Chou [:ting] from comment #7) > > When CSS transition have duration >0, waiting for transistionend for each > > change will make things happen in sequence and longer. We should make sure > > whether that's what we want. > > I modified the code a bit [1], and the toggling runs smoother. > > https://github.com/etiennesegonzac/dom-scheduler/compare/master...janus926: > bug-1170538-toggle Didn't want to potentially change the scheduling sequence but just landed something around this idea [1]. [1] https://github.com/etiennesegonzac/dom-scheduler/commit/c0b9b4ff246f08773b8a07b598df00bad58d8b20
(In reply to Etienne Segonzac (:etienne) from comment #12) > We're transitioning to computed position not known in advance. > So we'd have to generate keyframes in JS to switch to animations, would the > gain be worth the added complexity? Doesn't sound worthy. I expect only less white screen from eliminating synchronous PLayerTransaction::SendUpdate(), not performance improvement.
(In reply to Ting-Yu Chou [:ting] from comment #3) > * Dragged a list item down to a position, the items shifted up, released finger. > http://people.mozilla.org/~bgirard/cleopatra/#report=1f1ea2a213ac6b39ebaeaff15ae0d569e90ba8fd I didn't see noticable slow things from this profile.
(In reply to Etienne Segonzac (:etienne) from comment #13) > Didn't want to potentially change the scheduling sequence but just landed > something around this idea [1]. > > [1] > https://github.com/etiennesegonzac/dom-scheduler/commit/ > c0b9b4ff246f08773b8a07b598df00bad58d8b20 Just want to make sure you've noticed that the 2nd toggleTransitioning() is still done after 0.2s transition of changing hover's opacity. I am not sure is this a UX design, but I feel the button is more responsive after separating toggleEditMode() from button's toggling animation, it scales back right after my finger left the screen.
(In reply to Ting-Yu Chou [:ting] from comment #16) > (In reply to Etienne Segonzac (:etienne) from comment #13) > > Didn't want to potentially change the scheduling sequence but just landed > > something around this idea [1]. > > > > [1] > > https://github.com/etiennesegonzac/dom-scheduler/commit/ > > c0b9b4ff246f08773b8a07b598df00bad58d8b20 > > Just want to make sure you've noticed that the 2nd toggleTransitioning() is > still done after 0.2s transition of changing hover's opacity. I am not sure > is this a UX design, but I feel the button is more responsive after > separating toggleEditMode() from button's toggling animation, it scales back > right after my finger left the screen. The actual edit mode toggling could be delayed if the list is busy scrolling. So we keep the button "shrunk" and with pointer-events:none until the action is performed.
(In reply to Ting-Yu Chou [:ting] from comment #14) > (In reply to Etienne Segonzac (:etienne) from comment #12) > > We're transitioning to computed position not known in advance. > > So we'd have to generate keyframes in JS to switch to animations, would the > > gain be worth the added complexity? > > Doesn't sound worthy. I expect only less white screen from eliminating > synchronous PLayerTransaction::SendUpdate(), not performance improvement. That would be awesome to have less white screen. This is a perceived performance improvement!
I've now implemented all the content side fixes mentioned here. I'm ok to close it unless we want to leave this open for the potential platform improvements.
(In reply to Ting-Yu Chou [:ting] from comment #5) > - 11.9% PLayerTransaction::SendUpdate() > Synchronous IPC PLayerTransaction::SendUpdate() is blocked since > compositor thread is busy doing CompositorParent::CompositeToTarget(). For > instance Refresh21 [2080, 2140], the SendUpdate() blocks main thread for > 33ms. Filed bug 1181033 for this.
(In reply to Ting-Yu Chou [:ting] from comment #20) > (In reply to Ting-Yu Chou [:ting] from comment #5) > > - 11.9% PLayerTransaction::SendUpdate() > > Synchronous IPC PLayerTransaction::SendUpdate() is blocked since > > compositor thread is busy doing CompositorParent::CompositeToTarget(). For > > instance Refresh21 [2080, 2140], the SendUpdate() blocks main thread for > > 33ms. > > Filed bug 1181033 for this. Cool, closing here.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Blocks: 1199327
You need to log in before you can comment on or make changes to this bug.