Not sure this is efficient. We should look at some nth-child rules we have though.
Hey David, we have such a selector in our CSS: #messages-new-message-notice > div:nth-child(2) Does it make restyles show for any "div" ?
So I'm not quite sure what you mean. A rule like that does have disadvantages, though. It causes us to set the NODE_HAS_SLOW_SELECTOR_LATER_SIBLINGS flag on the div's parent -- and in fact on the parent of any div that fails the bloom filter (that is probabilistically filtering out most things that don't have #messages-new-message-notice) and that we try to match. This, in turn, causes us to do more restyling anytime we insert or remove a child of that parent, other than the last child. (We restyle all siblings after the inserted/removed one.)
We should consider making it a feature blocker if this improves launch time.
feature-b2g: --- → 2.2?
I'm marking feature-b2g:2.2+, as it's blocking performance meta bug. If it turns out the approach doesn't improve, then just close the bug or remove flag.
feature-b2g: 2.2? → 2.2+
I'll take some time on removing the nth-child rules
I've made an experiment about removing pseudo class like first/last/nth-child in message app css(18 selectors). The average time for visuallyLoaded on flame only dereased about ~10ms (from 1436ms to 1428ms). Not a impressive number but maybe it could reduce more on buri. Another note is there are still lots pseudo class selectors in building block, and it seems unable to scope BB css properly.
Who can work on this bug? Thanks.
(In reply to Kevin Hu [:khu] from comment #8) > Who can work on this bug? Thanks. I'm profling the css issue, maybe you could count me in. After removing the related class in shared BB, the average visuallyLoaded time is 1365ms. But please note that it's simple stlying removing, not replace by other class selector. So it means the replacing the stying will increase the time because of stying applied.
Assignee: nobody → schung
I've done some profiling on buri. The bad news is it seems didn't improve if we merely clean up the pseudo class in message. The time on buri varied a lot, and the avearge time even increased a little(~20ms) within limited tests. However, removing the pseudo class both in shared BB and message also works on buri and it could reduce around 30ms in average. There are 4 shared css stying in messae app(list, confirm, edit_mode and action_menu) which used pseudo class and they are removed temporary for this profiling.
Hey Steve, just want to note that using a "median" time is more meaningful than using a "average" time, because it's less sensitive to testing bias. Personally I use mathjs  in a jsbin so that I can reuse the test-perf output.  http://mathjs.org/
Steve, wondering how much decrease you see on Flame after removing all rules? Also, can you please share the patch you used to remove these rules? Last but not least, and I know it's a tedious task, do you know if especially one of the rules was especially bad for performance?
After applying this patch, here is some data for 20+ rounds for average and median time from loaded to first view ready(hope this could reproduce more accurate data): Before(ms) After(ms) Median:498.07 Median:465.47 Average:501.2 Average:473.09 There are some ugly selectors in shared action menu and edit mode, will investigate there css files first.
Here is some result after removing the selectors in action menu/edit mode : Remove selectors Remove selectors (ms) in action menu in edit mode Median:490.06 Median:488.79 Average:493.07 Average:494.54 Looks like we could reduce ~10ms from each css. Will do more test to replace them with simplified selectors.
[changing flag for the performance improvement task]
blocking-b2g: --- → 2.2+
feature-b2g: 2.2+ → ---
Whiteboard: [p=1] → [p(2.2S5)=1][p(2.2S4)=1]
Target Milestone: 2.2 S4 (23jan) → 2.2 S5 (6feb)
Comment on attachment 8551058 [details] [review] patch for classes replacement and clean up Hi Oleg, it's a patch contains 3 purpose: - unused class clean up - pseudo class replacement - set display to none for checkbox label The average visual ready time could be reduced 30~40ms on flame. I'll try other app at the mean time because BB styling changed.
Some discussion with Oleg offline: Since he didn't found any improvement from test-perf result, I also profiling again automatically(by make test-perf with 20 runs) and manully(from Performance Entry console log). It's weird that the modification in action_menu cause made that result slower than original :/ It's hard to trace which part will cause the regresion since the result is quite unstable. If I can't find a clear pattern for improvement, I might simply remove the unused selector in the end.
Comment on attachment 8551058 [details] [review] patch for classes replacement and clean up Hi Oleg, I've tried many combinations and it seems difficult to find a pattern that "must" improve performance. However this patch might improve slightly per average and has higher chance that produce better result then master. The patch is updated based on your suggestions, and with some adjustment: - Revert the action menu changes, because I can't not proof all the time, and this changes will need update most of the apps. - Revert the checkbox changes, because have this chages will sometimes lead the visual complete time increased in test-pref
Attachment #8551058 - Flags: feedback?(azasypkin) → review?(azasypkin)
Comment on attachment 8551058 [details] [review] patch for classes replacement and clean up Thanks for the patch, Steve! Even that I don't see much perf improvement, I still think it's good to have such cleanup. I left one optional nit and one question at github.
Attachment #8551058 - Flags: review?(azasypkin) → review+
Comment on attachment 8551058 [details] [review] patch for classes replacement and clean up Hi Pavel, I made some css selector clean up for edit_mode.css since all the apps already migrated the original header to new gaia-header component. The only problem is the example html still used the header element for edit form. Do you think we should keep the selector for header element for compatibility, or modify the example to apply the gaia-header as well?
I think we should modify the example to appy the new gaia-header component. This make more sense to me :)
Be careful as I'm not sure all apps are using gaia-header... see  for example. A patch will remove this one soon (I don't have the bug number in my head right now) but other cases can exist.  https://github.com/mozilla-b2g/gaia/blob/master/apps/bookmark/save.html#L38-L47
Comment on attachment 8551058 [details] [review] patch for classes replacement and clean up At least the apps using edit mode applied gaia-header, so I think it's fine to clean up the header related selector in edit mode. I changes the edit mode example to gaia-header, but it seems doesn't work as expected. Hi Wilson, is there anything missed in the example?
Attachment #8551058 - Flags: feedback?(wilsonpage) → feedback?(wilsonpage)
Can we update the target milestone? Thanks.
Comment on attachment 8551058 [details] [review] patch for classes replacement and clean up The urls are broken for file://. You need to use relative urls: <script defer src="../../elements/gaia-header/dist/gaia-header.js"></script> Also `component_utils.js` not required.
Attachment #8551058 - Flags: feedback?(wilsonpage) → feedback-
Comment on attachment 8551058 [details] [review] patch for classes replacement and clean up Thanks for the suggestion! Patch updated and hope this changes for edit mode example is appropriate.
Target Milestone: 2.2 S5 (6feb) → 2.2 S8 (20mar)
Comment on attachment 8551058 [details] [review] patch for classes replacement and clean up Hi Julien, do you have any thought or suggestion about the edit mode example applied gaia-header(since Wilson is PTO now)? Thanks!
Attachment #8551058 - Flags: feedback?(wilsonpage) → feedback?(felash)
Comment on attachment 8551058 [details] [review] patch for classes replacement and clean up yep, the example looks good: same output than on master, I didn't see anything weird in the network view. So lgtm.
Attachment #8551058 - Flags: feedback?(felash) → feedback+
Comment on attachment 8551058 [details] [review] patch for classes replacement and clean up LGTM :)
Attachment #8551058 - Flags: review?(pivanov) → review+
Thanks! In master: https://github.com/mozilla-b2g/gaia/commit/a0c1cf5892218d73054d53448ec1d06a2b846bef
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Comment on attachment 8551058 [details] [review] patch for classes replacement and clean up [Approval Request Comment] [Bug caused by] (feature/regressing bug #):Original behavior before 2.0 [User impact] if declined: Small startup time penalty because of bad/unused css [Testing completed]: N/A [Risk to taking this patch] (and alternatives if risky): Low [String changes made]: N/A
Attachment #8551058 - Flags: approval-gaia-v2.2?(bbajaj)
Attachment #8551058 - Flags: approval-gaia-v2.2?(bbajaj) → approval-gaia-v2.2+
You need to log in before you can comment on or make changes to this bug.