[Messages] investigate scoping CSS rules

RESOLVED FIXED in 2.2 S8 (20mar)

Status

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: julienw, Assigned: steveck)

Tracking

unspecified
2.2 S8 (20mar)
x86_64
Linux
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.2+, b2g-v2.1 wontfix, b2g-v2.2 fixed, b2g-master fixed)

Details

(Whiteboard: [p(2.2S5)=1][p(2.2S4)=1])

Attachments

(2 attachments)

Comment hidden (empty)
(Reporter)

Comment 1

5 years ago
Posted file wip github PR
Not sure this is efficient.

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

Comment 2

5 years ago
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" ?
Flags: needinfo?(dbaron)
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.)
Flags: needinfo?(dbaron)
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+
(Assignee)

Comment 6

4 years ago
I'll take some time on removing the nth-child rules
(Reporter)

Updated

4 years ago
Whiteboard: [p=1]
(Reporter)

Updated

4 years ago
Target Milestone: --- → 2.2 S4 (23jan)
(Assignee)

Comment 7

4 years ago
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.
(Assignee)

Comment 9

4 years ago
(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
(Assignee)

Comment 10

4 years ago
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.
(Reporter)

Comment 11

4 years ago
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/
(Reporter)

Comment 12

4 years ago
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?
(Assignee)

Comment 13

4 years ago
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.

Updated

4 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 14

4 years ago
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+ → ---
(Reporter)

Updated

4 years ago
Whiteboard: [p=1] → [p(2.2S5)=1][p(2.2S4)=1]
Target Milestone: 2.2 S4 (23jan) → 2.2 S5 (6feb)
(Assignee)

Comment 16

4 years ago
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.
Attachment #8551058 - Attachment description: Test patch for classes removal → patch for classes replacement and clean up
Attachment #8551058 - Flags: feedback?(azasypkin)
(Assignee)

Comment 17

4 years ago
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.
(Assignee)

Comment 18

4 years ago
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+
(Assignee)

Comment 20

4 years ago
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?
Attachment #8551058 - Flags: feedback?(pivanov)
I think we should modify the example to appy the new gaia-header component. This make more sense to me :)
(Reporter)

Comment 22

4 years ago
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
(Assignee)

Comment 23

4 years ago
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)
Attachment #8551058 - Flags: feedback?(pivanov)
(Assignee)

Updated

4 years ago
Attachment #8551058 - Flags: feedback?(wilsonpage) → feedback?(wilsonpage)
Can we update the target milestone? Thanks.
Flags: needinfo?(whuang)
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-
(Assignee)

Comment 26

4 years ago
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.
Attachment #8551058 - Flags: review?(pivanov)
Attachment #8551058 - Flags: feedback?(wilsonpage)
Attachment #8551058 - Flags: feedback-
Flags: needinfo?(whuang)
Target Milestone: 2.2 S5 (6feb) → 2.2 S8 (20mar)
(Assignee)

Comment 27

4 years ago
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)
(Reporter)

Comment 28

4 years ago
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+
(Assignee)

Comment 30

4 years ago
Thanks!

In master: https://github.com/mozilla-b2g/gaia/commit/a0c1cf5892218d73054d53448ec1d06a2b846bef
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
(Assignee)

Comment 31

4 years ago
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.