Closed Bug 1199327 Opened 9 years ago Closed 7 years ago

(nga-performance) Audit the initialization phase of the gaia-fast-list virtual list component

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 WONTFIX

People

(Reporter: etienne, Unassigned)

References

Details

You can find an example app using the component here [1] We want to optimize the time it takes to initialize the list and get to the first rendering. Wilson did a fair amount of profiling already but we could use some "platform eyes" on this :) [1] https://github.com/gaia-components/fast-list/tree/master/examples/gaia-fast-list
Hey Ting, flagging you since you already know the virtual list code pretty well :) We're open to any kind of suggestions to make things better, but in particular we'd like to know if setting the willChange (and creating all the list items layers) should be delayed like in [1]. [1] https://github.com/gaia-components/fast-list/pull/45
Flags: needinfo?(janus926)
Sure, I'll come back later. Keep NI.
Spent some time with :Cwiiis looking into the very expensive >150ms paint on initialization. He thinks there is an issue with 'progressive paint' on `overflow: scroll` containers. This means that everything is being painted, including the stuff outside the scroll viewport, in the first pass. By changing our scroll container from `overflow: scroll` to `overflow: hidden` reduces initial paint to ~40ms. I suggest we try initializing with `overflow: hidden` and find a sensible time in the future to switch to `overflow: scroll`. This will cause an expensive paint, but hopefully during idle time. Perhaps `onload` could be appropriate?
(In reply to Wilson Page [:wilsonpage] from comment #3) > Spent some time with :Cwiiis looking into the very expensive >150ms paint on > initialization. He thinks there is an issue with 'progressive paint' on > `overflow: scroll` containers. This means that everything is being painted, > including the stuff outside the scroll viewport, in the first pass. > > By changing our scroll container from `overflow: scroll` to `overflow: > hidden` reduces initial paint to ~40ms. I suggest we try initializing with > `overflow: hidden` and find a sensible time in the future to switch to > `overflow: scroll`. This will cause an expensive paint, but hopefully during > idle time. Perhaps `onload` could be appropriate? Ahah this is totally a Cwiiis-y hack :) But we should be able to re-use the PR 45 to test it, I'll make the change.
Just to clarify, I actually recommend you hide the off-screen content with display:none or visibility:hidden (I think either will do) - if you set overflow: hidden, the user won't be able to scroll when the app opens if it blocks for too long before setting overflow:scroll - also, using display/visibility, you may avoid the relayerisation when you make the extra content visible, which will make your second paint/relayout less expensive.
(my hacks have evolved a little in the past year :))
When I launch the app, I saw those from logcat and the app showed just white screen: E/fast-list/gaia-fast-list(21443): [JavaScript Error: "TypeError: document.registerElement is not a function" {file: "app://gaia-fast-list.gaiamobile.org/bower_components/gaia-component/gaia-component.js" line: 73}] E/fast-list/gaia-fast-list(21443): [JavaScript Error: "TypeError: list.configure is not a function" {file: "app://gaia-fast-list.gaiamobile.org/app.js" line: 7}] How should I fix this?
(In reply to Ting-Yu Chou [:ting] from comment #7) > When I launch the app, I saw those from logcat and the app showed just white > screen: > > E/fast-list/gaia-fast-list(21443): [JavaScript Error: "TypeError: > document.registerElement is not a function" {file: > "app://gaia-fast-list.gaiamobile.org/bower_components/gaia-component/gaia- > component.js" line: 73}] > E/fast-list/gaia-fast-list(21443): [JavaScript Error: "TypeError: > list.configure is not a function" {file: > "app://gaia-fast-list.gaiamobile.org/app.js" line: 7}] > > How should I fix this? Arrrrggg. The app must be missing the "moz-extremely-unstable-and-will-change-webcpomonents" permission.
(In reply to Etienne Segonzac (:etienne) from comment #8) > The app must be missing the > "moz-extremely-unstable-and-will-change-webcpomonents" permission. I thought you were joking, and then realized it's really the name.
Captured two profiles [1] and [2] on Flame 1024MB without and with the PR in comment 1. Note I added performance markers |loadNext| and |modelConcat| in app.js when entering loadNext() and before calling list.model.concat(). I will check them and follow up later. Gecko: f2518b8a7b97 Gaia: 31e595f86f6bf159b3a9a46816a6ac00a55ca9f9 [1] https://people.mozilla.org/~bgirard/cleopatra/#report=93d97b08c108b644b5bae91661de9421cc1221e5 [2] https://people.mozilla.org/~bgirard/cleopatra/#report=078e0a6b6642fc0d161a37f0715d96f91c83009d
I haven't figured out how much does delay setting willChange impact the performance, but here're things I found so far: 1. The first render() call have startIndex 0 and endIndex -1 since the model is empty, and this makes the overflow-y hidden in PR#45 doesn't really help, since _prepareForScroll() will be called before the first batch of data arrive. 2. The big chunks on profile are: a) the render() call after very fist model.concat(), it creates and inserts items (>100ms) b) 1st refresh driver tick after a), seems for restyle/reflow/paint the 20 inserted items (>250ms) My theory is to do things earlier, after requesting data and before data arrive, and do less things to paint first screen when receive data: a. Somehow pre-create and even insert first page items to DOM before data arrive. b. Make sure _prepareForScroll() removes overflow-y hidden after the items of first page are painted. c. Create items out of the first screen later, or use display:none / visibility:hidden to skip restyle/reflow them. I just tried to set endIndex 8 when !_rendered to render only first screen items, and it can save ~180ms from 2a and 2b on Flame.
Flags: needinfo?(janus926)
Thanks Ting! Here's my takeaways: - We could create the `<li>`s before .render(), so that when `model` is set we have less work to do. - First render can be sped up if we create just enough items to fill the viewport (criticalStart -> criticalEnd) - We could reduce the initial paint/layout time if we `display: none` / `visibility: hidden` out of viewport items.
(In reply to Wilson Page [:wilsonpage] from comment #12) > Thanks Ting! Here's my takeaways: No problem, hope it helps and looking forward for good news ;) > > - We could create the `<li>`s before .render(), so that when `model` is set > we have less work to do. If doing this before |model| is set, will need to make sure the change does not delay app's JS to get data back. Maybe do this only if the model is set but is empty?
Just want to warn again that setting overflow-y: hidden will disable scrolling completely until the relayout and redraw after removing it. This can take a significant enough amount of time that a user would be trying to scroll and nothing happens, despite the interface looking like it should be able to scroll. A better solution would be to postpone adding any list items that go beyond the bounds of the screen until after first-paint. Note that also changing overflow causes the entire element to redraw, including the area that doesn't change - this is also costly.
From the profile in comment 10, I can see many samples of nsDisplayBackgroundColor::Paint and nsStyleContext::StyleBackground in a refresh driver tick, which might be the reason why the painting takes long.
(In reply to Ting-Yu Chou [:ting] from comment #15) > From the profile in comment 10, I can see many samples of > nsDisplayBackgroundColor::Paint and nsStyleContext::StyleBackground in a > refresh driver tick, which might be the reason why the painting takes long. Both of these calls are followed by nsRuleNode::ResolveVariableReferences() which suggest perhaps CSS Variables are adding some time here. I will try a profile without CSS Variables and report back.
I'll look into the restyling, but I am not sure what files I should be looking at. The URL in comment 0 doesn't work, and there is no file called gaia-fast-list.js (which is what the profiles above mention) in that repo. Where should I be looking?
Flags: needinfo?(etienne)
(In reply to Cameron McCormack (:heycam) from comment #18) > I'll look into the restyling, Thanks! > but I am not sure what files I should be > looking at. The URL in comment 0 doesn't work, and there is no file called > gaia-fast-list.js (which is what the profiles above mention) in that repo. > Where should I be looking? The component itself [1] (and most of the example apps [2]) moved to their own repo. [1] https://github.com/gaia-components/gaia-fast-list [2] https://github.com/gaia-components/gaia-fast-list/tree/master/examples/caching
Flags: needinfo?(etienne)
(In reply to Cameron McCormack (:heycam) from comment #18) > I'll look into the restyling, but I am not sure what files I should be > looking at. The URL in comment 0 doesn't work, and there is no file called > gaia-fast-list.js (which is what the profiles above mention) in that repo. > Where should I be looking? The gh-pages branch has all the dependencies included, so you can simply `git clone` and run examples/caching/index.html on desktop, or use WebIDE to flash the examples/caching app to a device.
(In reply to Wilson Page [:wilsonpage] from comment #17) > Doesn't seem to make a massive difference to the 'Styles #1' block. Seems to > be ~10ms faster w/o variables. I re-profiled today and saw a more significant difference between w/ and w/o using CSS variables. Perhaps it's the way we are using them, or just that they're an immature feature. Either way I'd really appreciate :dbaron or :heycam digging into this further :)
One thing that stands out to me is that there are a bunch of assignments to element.style.transform and element.style.borderTop that are not required, since they're setting the same (or equivalent) values. We don't check that a change to a property on a .style would have no effect due to being the same value, so if you can do that check and skip the assignment, we won't need to restyle the element. The unnecessary changes to .style.transform are being done here: https://github.com/gaia-components/gaia-fast-list/blob/f3004cb6e6acdc2081919830dfd97800eee07f2e/bower_components/fast-list/fast-list.js#L599-L611 Note that if you want to compare the existing value from item.style.transform before doing the assignment (rather than keeping your own state around), you should build your transform value string to look like the canonical computed style representation: 'translate3d(0px, ' + position + 'px, 0px)'. The other example of this is the setting of borderTop here: https://github.com/gaia-components/gaia-fast-list/blob/f3004cb6e6acdc2081919830dfd97800eee07f2e/gaia-fast-list.js#L523-L525 Incidentally you might want to consider making the "no border" case be "1px solid transparent", so that you don't need to reflow and all your items have a consistent height. I haven't tried profiling but making these changes did remove a lot of the restyles. Can you let me know if that helps sufficiently? There is also still some restyling happening due to updating overflow-{x,y} in the .style of the element despite having other !important rules that fix overflow to hidden. Even though the computed values won't change from hidden, we still need to restyle the element in response to touching .style.overflow{X,Y} in fast-list.js.
Flags: needinfo?(wilsonpage)
(In reply to Cameron McCormack (:heycam) from comment #22) > One thing that stands out to me is that there are a bunch of assignments to > element.style.transform and element.style.borderTop that are not required, > since they're setting the same (or equivalent) values. We don't check that > a change to a property on a .style would have no effect due to being the > same value, so if you can do that check and skip the assignment, we won't > need to restyle the element. That's really good to know, thanks! Hopefully we'll get those fixes in soon.
Thanks Cameron, this is super valuable feedback! (In reply to Cameron McCormack (:heycam) from comment #22) > There is also still some restyling happening due to updating overflow-{x,y} > in the .style of the element despite having other !important rules that fix > overflow to hidden. Even though the computed values won't change from > hidden, we still need to restyle the element in response to touching > .style.overflow{X,Y} in fast-list.js. Our aim here was to try to get a cheap first-paint, before upgrading the list to the fully scrollable, layerized component. We found removing `will-change: transform` and `overflow-y: scroll` slashed the time spent doing display-list/layerization work allowing us to get something on the screen ASAP. We're open to other suggestions here :)
Flags: needinfo?(wilsonpage)
Also, I'm interested to know if there is a performance win by settings styles directly on an element via `.styles` over setting a class? For example, which is preferable from Gecko perspective: A. element.style.border = '1px solid'; B. element.classList.add('has-border'); --- From my perspective I would have thought 'A' would be faster as it would hit fewer code paths.
Flags: needinfo?(cam)
(In reply to Wilson Page [:wilsonpage] from comment #25) > Also, I'm interested to know if there is a performance win by settings > styles directly on an element via `.styles` over setting a class? > > For example, which is preferable from Gecko perspective: > > A. element.style.border = '1px solid'; > B. element.classList.add('has-border'); > > --- > > From my perspective I would have thought 'A' would be faster as it would hit > fewer code paths. The difference is probably going to be pretty small, and how much work B needs depends on what other rules that involve "has-border" classes are in the style sheet and where the class name appears. In both cases, we need to re-serialize the DOM attribute (style="" or class=""). In both cases we need to parse a string (using the CSS parser or as a class name). For A, we need to construct a new style rule to represent the element's inline style; for B we don't need to do that. For A we can quickly create a new rule node to point to the new style rule, without having to run selector matching on the element. For B, we might have to run selector matching on the element, if there are rules that involve "has-border" somewhere in the middle of the selector.
Flags: needinfo?(cam)
(In reply to Wilson Page [:wilsonpage] from comment #24) > Our aim here was to try to get a cheap first-paint, before upgrading the > list to the fully scrollable, layerized component. We found removing > `will-change: transform` and `overflow-y: scroll` slashed the time spent > doing display-list/layerization work allowing us to get something on the > screen ASAP. > > We're open to other suggestions here :) My suggestion here is just to avoid setting those non-!important .style.overflow{X,Y} values on the element if you know there are !important rules that set overflow and would prevent your inline styles from applying. We can't know this at the time .style.overflow{X,Y} is assigned to and need to restyle the element.
(In reply to Wilson Page [:wilsonpage] from comment #24) > Our aim here was to try to get a cheap first-paint, before upgrading the > list to the fully scrollable, layerized component. We found removing > `will-change: transform` and `overflow-y: scroll` slashed the time spent > doing display-list/layerization work allowing us to get something on the > screen ASAP. > > We're open to other suggestions here :) In example/caching app, it's using caching mechanism to get first-paint faster. However, from the profiling results as follows, it seems not giving much help on that. [1] w/ caching : http://goo.gl/6P1Amv [2] w/o caching & load first 10 items : http://goo.gl/PaXc7k In [1], the time from marker firstLoad to firstPaint takes 873ms, reading back from cache (injectItemFromCache) takes 144ms. In [2], no caching and try to load first 10 items instead, it takes 863ms. In terms of first-paint performance, they are almost no difference, however, caching incurs a problem on data consistency so that user might not be seeing up to date data presented on first-paint. In addition to that, user is not able to scroll the listview even it's shown out there. My suggestion is just to render the items which are visible in first-paint and after first frame is drawn, render remaining items based on maxItemCount. With this approach, user experience and performance can be balanced. I'll be further looking into the time spending on Refresh#1 and see what kinds of improvements we can take more.
(In reply to Astley Chen [:astley] from comment #28) > My suggestion is just to render the items which are visible in first-paint > and after first frame is drawn, render remaining items based on > maxItemCount. With this approach, user experience and performance can be > balanced. This was, and still is my recommendation also (see comment #14).
(In reply to Astley Chen [:astley] from comment #28) > (In reply to Wilson Page [:wilsonpage] from comment #24) > > Our aim here was to try to get a cheap first-paint, before upgrading the > > list to the fully scrollable, layerized component. We found removing > > `will-change: transform` and `overflow-y: scroll` slashed the time spent > > doing display-list/layerization work allowing us to get something on the > > screen ASAP. > > > > We're open to other suggestions here :) > > In example/caching app, it's using caching mechanism to get first-paint > faster. > However, from the profiling results as follows, it seems not giving much > help on that. > > [1] w/ caching : http://goo.gl/6P1Amv > [2] w/o caching & load first 10 items : http://goo.gl/PaXc7k > > In [1], the time from marker firstLoad to firstPaint takes 873ms, reading > back from cache (injectItemFromCache) takes 144ms. > In [2], no caching and try to load first 10 items instead, it takes 863ms. > > In terms of first-paint performance, they are almost no difference, however, > caching incurs a problem on data consistency so that user might not be > seeing up to date data presented on first-paint. > In addition to that, user is not able to scroll the listview even it's shown > out there. > My suggestion is just to render the items which are visible in first-paint > and after first frame is drawn, render remaining items based on > maxItemCount. With this approach, user experience and performance can be > balanced. > I'll be further looking into the time spending on Refresh#1 and see what > kinds of improvements we can take more. Thank you for your feedback astley! We found that caching backed by localStorage was unacceptably slow on B2G. I think this slowness was being masked by the app's splash-screen being held up longer than normal. The result appeared better with the caching as there was no white flash when the splash screen was taken down. The goal of the caching feature is to provide content in a faster time than the app would otherwise be able to fetch the real content (from indexeddb or network). This takes the burden off app developers to implement their own caching solutions allowing them to focus on fetching the real data. There is a risk cached data will become out-of-date, but we do provide an API for users to clear the cache. Also cached content should normally be displayed for only 500-1000ms before being replaced.
I'm working on a patch [1] to switch out localStorage for the async Cache API. This should prevent blocking that is masking the benefits of the caching feature. From my tests it seems it can take 100-200ms to get content from the cache, but as I said, this is not blocking. [1] https://github.com/gaia-components/gaia-fast-list/pull/22
(In reply to Wilson Page [:wilsonpage] from comment #31) > I'm working on a patch [1] to switch out localStorage for the async Cache > API. This should prevent blocking that is masking the benefits of the > caching feature. From my tests it seems it can take 100-200ms to get content > from the cache, but as I said, this is not blocking. > > [1] https://github.com/gaia-components/gaia-fast-list/pull/22 Sync or async to get content from cache seems not a big concern for first-paint performance since the Refresh#1 for both can be done by loadEnd which means the first frame is rendered before app splash-screen is fading out. Incidentally, injectItemFromCache is invoked during the construction of gaia-fast-list, to ensure developer can have a chance to discard the cache and using real data, we probably need a callback or some way to keep client the ability to do so. Caching feature is good for certain cases if the data is rarely changed or updated, however, for those apps whose data changed often or cant determine whether data is outdated or not, it would be not helpful for them, the value of this feature is all depends on which use cases are the most. In any case, cache feature is able to provide help for 2nd launch first paint performance, however for 1st launch(cold launch) or cleaned-cache launch, we still need to figure out how to boost the performance across the pipeline of refresh(re-styling, re-flow, rasterization..). Let me know if anything I can help. Thanks.
I've spent today auditing initialization of the list within Music app. After discussing with Etienne we have agreed on the following action items: 1. Render just enough items to fill the viewport when FastList is first created, filling in the remaining off viewport items later. 2. Don't call populateItemDetail() on first render to prevent expesive detail rendering (images) getting in the way of first paint. 3. Don't layerize images on first render. 4. Use document fragments when attaching sections and list-items to the document. 5. Only cache enough section to fill the viewport 6. Attempt to set cached scroll-height on initial cached render to make list interactive (depends on cost). 7. Remove the layerization step and render layers on first pass (cheaper after 1. implemented)
Firefox OS is not being worked on
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.