1452214 - https://jrmuizel.github.io/perf-tests/moving-balls.html spends a long time > 110ms in style

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P3)

Description

[Jeff Muizelaar [:jrmuizel]]

https://perfht.ml/2H2fPnn

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

5k elements are traversed and styled, which is exactly expected, but it shouldn't generally take that much time I suppose...

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

This is a profile with a single stylo thread: https://perfht.ml/2KD1NXB

It seems the majority of time happens in StrongRuleNode::ensure_child. This is a case where one element has tons of children which don't share style at all, so the cost of rule tree becomes significant. We are probably experiencing O(N^2) performance issue here that we are scanning all the child nodes under the parent element for each children.

I don't immediately have idea how should this be avoided. Maybe we should exclude rule nodes with only style attribute from rule tree iterating.

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #2)
> Maybe we should exclude rule nodes with only style attribute from rule tree iterating.

I mean, maybe each rule node can have two subtree, one for rule nodes of style attributes (maybe also animations / transitions?), the other for those from style rules, so that we don't waste time on checking reusability of the former.

Comment 4

[Cameron McCormack (:heycam)]

The old nsRuleNode switched its children representation from a linked list to a hash table when we reached a threshold of a certain number of children, which I think would have helped this test.  We'd need a lock free way to do that, which isn't impossible, but would complicate the RuleNode implementation even more.

Separating style attribute rule nodes out into a separate child list might be reasonable, at the expense of increasing RuleNode's size, though there's probably something clever we could do to avoid that.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

It's unclear to me why we spend so much time in ensure_child. That means that the rule node was replaced completely, right? One would think we'd hit[1] often instead.

Are we copying the declaration block all the time? If so the bug is in CSSOM I'd think...

[1]: https://searchfox.org/mozilla-central/rev/2aa42f2cab4a110edf21dd7281ac23a1ea8901f9/servo/components/style/rule_tree/mod.rs#432

Comment 6

[Emilio Cobos Álvarez (:emilio)]

So I'm moderately sure this was regressed by bug 1402218... We still have other bugs related to first-line and such. I think we need to fix the first-line reparenting code instead of that fix, that would allow us to reuse the node.

Will take a look when I have the time.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

To be clear, the issue here is that we reparent first-line styles even from frames that are about to go away. I'd love to get rid of the first-line reparenting code, but we can presumably do better instead...

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Hey emilio - where do you feel this falls in terms of priority? Given that moving-balls.html is a tech demo, we're not sure how representative it is for the web in general... would you consider this a nice to have (qf:p3), or an important-to-fix (qf:p1)?

Comment 9

[Emilio Cobos Álvarez (:emilio)]

I'd say it's somewhere in between, p2 probably.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

This looks much better now I think. We still thrash the rule tree way more than we should due to ::first-line shittyness, but bug 1493420 should've made it faster. Mind confirming you also don't see such huge style times anymore?

Comment 11

[Jeff Muizelaar [:jrmuizel]]

Style is still a big chunk of the time, but it's improved enough for us to mark this as fixed: https://perfht.ml/35HvWQD