1089154 - [Messages] investigate scoping CSS rules

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::SMS, defect)

Comment 1

[Julien Wajsberg [:julienw]]

Attachment: wip github PR

Not sure this is efficient.

We should look at some nth-child rules we have though.

Comment 2

[Julien Wajsberg [:julienw]]

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" ?

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.)

Comment 4

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

We should consider making it a feature blocker if this improves launch time.

Comment 5

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

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.

Comment 6

[Steve Chung [:steveck]]

I'll take some time on removing the nth-child rules

Comment 7

[Steve Chung [:steveck]]

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.

Comment 8

[Kai-Chih Hu [:khu]]

Who can work on this bug? Thanks.

Comment 9

[Steve Chung [:steveck]]

(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.

Comment 10

[Steve Chung [:steveck]]

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.

Comment 11

[Julien Wajsberg [:julienw]]

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 [1] in a jsbin so that I can reuse the test-perf output.

[1] http://mathjs.org/

Comment 12

[Julien Wajsberg [:julienw]]

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?

Comment 13

[Steve Chung [:steveck]]

Attachment: patch for classes replacement and clean up

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.

Comment 14

[Steve Chung [:steveck]]

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.

Comment 15

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

[changing flag for the performance improvement task]

Comment 16

[Steve Chung [:steveck]]

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.

Comment 17

[Steve Chung [:steveck]]

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 18

[Steve Chung [:steveck]]

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

Comment 19

[Oleg Zasypkin [: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.

Comment 20

[Steve Chung [:steveck]]

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?

Comment 21

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

I think we should modify the example to appy the new gaia-header component. This make more sense to me :)

Comment 22

[Julien Wajsberg [:julienw]]

Be careful as I'm not sure all apps are using gaia-header...

see [1] 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.

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/bookmark/save.html#L38-L47

Comment 23

[Steve Chung [:steveck]]

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?

Comment 24

[Kai-Chih Hu [:khu]]

Can we update the target milestone? Thanks.

Comment 25

[Wilson Page [:wilsonpage]]

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.

Comment 26

[Steve Chung [:steveck]]

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.

Comment 27

[Steve Chung [:steveck]]

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!

Comment 28

[Julien Wajsberg [:julienw]]

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.

Comment 29

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

Comment on attachment 8551058 [details] [review]
patch for classes replacement and clean up

LGTM :)

Comment 30

[Steve Chung [:steveck]]

Thanks!

In master: https://github.com/mozilla-b2g/gaia/commit/a0c1cf5892218d73054d53448ec1d06a2b846bef

Comment 31

[Steve Chung [:steveck]]

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

Comment 32

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/42477300e99391ad652051f4a862e7ab49e07fe3