Closed Bug 1136893 Opened 10 years ago Closed 9 years ago

Slow subtree restyling in this testcase

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: mstange, Assigned: heycam)

References

Details

(Keywords: perf)

Attachments

(4 files, 2 obsolete files)

testcase
(deleted), text/html
Details
proof-of-concept patch
(deleted), patch
Details | Diff | Splinter Review
proof-of-concept patch part 2
(deleted), patch
Details | Diff | Splinter Review
testcase-restyle.html
(deleted), text/html
Details
Attached file testcase (obsolete) (deleted) —
This is a very simplified testcase inspired by one problem from bug 1125224. On my machine, Firefox takes about 770ms to flush, and Chrome takes 4ms.
Attached file testcase (obsolete) (deleted) —
I removed the duplicate rules and instead increased the number of child elements. Chrome wasn't affected by the duplicate rules, so it seems to have some kind of duplicate rule detection, and that's not something I want us to imitate as part of fixing this bug. New numbers: - Firefox: 260ms - Chrome: 4ms - Safari: 36ms
Attachment #8569410 - Attachment is obsolete: true
So apparently what's happening is that we detect that the selector "body.nav-open .wrapper" involves descendants of the body, so we do a subtree restyle on the body. Can we, when we encounter that selector, instead make a list of all the elements that match the rightmost selector ".wrapper", and then do a self restyle on those? I assume we have fast paths for getting all the elements that match a simple class selector.
Flags: needinfo?(cam)
Keywords: perf
Attached file testcase (deleted) —
with Date.now() fallback
Attachment #8569420 - Attachment is obsolete: true
The current setup, as you've probably deduced, is that we store a table of selectors that use a given class name, a table of those that use a given ID, etc. When we detect an attribute change (including class="") in RestyleManager::Attribute{WillChange,Changed}, we ask the style set 'please tell me what kind of restyling I need to do on this element with the changed attribute, given all the rules we have'. We see that we had a changed class="" attribute and look up the selectors that use that class somewhere in the selector; that's nsCSSRuleProcessor::HasAttributeDependentStyle. In the table (mClassTable) we store a pointer to the part of the selector that has the class name match. So here we have a pointer to the nsCSSSelector that represents the body.nav-open bit. We then check that the element we had the class="" attribute change on actually matches the the selector starting from body.nav-open and moving left (in case we had any ancestor selectors). We do, so we look at what the combinator is just to the right of our point in the selector -- it's a " " descendant combinator (between the body.nav-open and the .wrapper) which means we return eRestyle_Subtree meaning we need to restyle the subtree, not just the element with the class="" on it. We don't really have a fast path for getting all descendant elements that match a given class name. We do have stuff in dom/base/nsContentList.{h,cpp} which are used for (among other things) the return value of getElementsByClassName(), which starts by traversing the entire subtree beneath the root you pass in, and then watches for DOM mutations and updates itself when changes occur. And I feel like it would be heavy to keep one of these around for selector matching here. So we're going to have to traverse the subtree under the <body>. Can we avoid restyling those descendant elements unless they match .wrapper? We could have a cheap trick where we have HasAttributeDependentStyle somehow return that the element itself doesn't need restyling, but only descendants that match .wrapper do, and do that traversal in RestyleManager::Attribute{WillChange,Changed}, calling PostRestyleEvent on each descendant that does match .wrapper. I'm assuming here that the restyling of each descendant is where the time is being taken, rather than the traversal over the descendants.
Flags: needinfo?(cam)
I slightly misread the problem here. I was thinking we had a DOM structure like: <body> <div> <div> <div class=wrapper></div> where we know that we want to restyle the div.wrapper and that we can avoid restyling the other divs. The structure is actually: <body> <div class=container> <div class=wrapper> <div> <div> ... where the bulk of the elements we want to avoid restyling are under the div.wrapper. What we can still do is detect this case where the selector has |.wrapper| at the end, traverse the DOM under the <body> to find elements that have that class, and ensure that the previously reported restyle hint of eRestyle_Subtree (which means to re-run selector matching on the entire subtree) is replaced with eRestyle_Self (which means re-run selector matching on the element, but not on descendants) when restyling the div.wrapper instead. Here we should trigger the bug 931668 optimisations, but it still means that each child of div.wrapper needs to get a new style context computed for it so that we can verify there are no style changes. This lets us avoid recomputing style for the grandchildren of the div.wrapper, at least. I wonder if we could record on style contexts whether they explicitly inherit any properties on a reset style struct (like nsStyleDisplay, here, which is where clip is stored) so that we could know that the div.wrapper's children will not be affected by a style change to a non-inherited property on div.wrapper, without needing to actually recompute their styles.
Attached patch proof-of-concept patch (deleted) — — Splinter Review
Here's a hacked up proof-of-concept of the approach in comment 4 plus converting eRestyle_Subtree into eRestyle_Self. On my machine this reduces the time reported from your test from 260ms to 46ms.
(In reply to Cameron McCormack (:heycam) from comment #6) > Created attachment 8571117 [details] [diff] [review] > proof-of-concept patch > > Here's a hacked up proof-of-concept of the approach in comment 4 plus > converting eRestyle_Subtree into eRestyle_Self. On my machine this reduces > the time reported from your test from 260ms to 46ms. Do you know how Chrome manages to do this quickly?
I'm not familiar with how Chrome does things, I'd have to take a look.
Attached patch proof-of-concept patch part 2 (deleted) — — Splinter Review
However, if I "record on style contexts whether they explicitly inherit any properties on a reset style struct" per the last para of comment 5, I get down to ~6ms.
These two PoC patches are independent, btw. On this test case, the former will skip the restyling of the body and its child div (the one that's the parent of the div.wrapper), while the latter will ensure we don't restyle any children of the div.wrapper.
I think we really need this. It's probably killing us in bug 1172239.
With the test case from bug 1172239, the PoC patch doesn't quite work yet. First, we end up checking to see if it's safe to do this "restyle individual descendants rather than restyle the subtree of the element whose class is changing" optimisation for all six class names on the element, even though we really only need to care about the presence of .s-item-container-height-auto. At least one of the selectors falls off the quite narrow conditions the patch checks for (that the class is present only in selectors that consist of more than one simple selector, and where the last selector has a " " combinator on its lhs and at least one class selector on the rhs). I'll work on loosening this up. Maybe HasAttributeDependentStyle can return not just a list of class names to check for, but actual selectors. After removing those class names from that element, we do trigger the new behaviour, but I think we're spending too much time inside AddPendingRestyle checking up the tree to find an existing restyle root to piggy back off. It ends up making the test 10x as slow. :-) In the patch's PostRestyleEventForClasses, since we're traversing down the tree anyway, we should just check for a restyle root that's an ancestor of the element we're toggling the class on once, update that knowledge as we traverse down the tree, and then pass that closest ancestor restyle root into AddPendingRestyle so that it doesn't need to find it itself.
Status: NEW → ASSIGNED
Depends on: 1180118
Depends on: 1180120
I split off the two ideas in this bug to dependents and will continue work there.
> Do you know how Chrome manages to do this quickly? That's a good question. In particular, it manages it a lot faster than Safari... and last I'd looked at WebKit's style code they just always restyled the whole subtree on class changes. But that was a while back; they might have changed it since then. Patrick might know more about what Blink does here, and https://docs.google.com/document/d/1vEW86DaeVs4uQzNFI5R-_xS9TcS1Cs_EUsHRSgCHGu8/edit?pli=1#heading=h.xa3ovcncd2vp might be useful (I haven't read it yet, so can't claim it definitely is).
Flags: needinfo?(pwalton)
With unpatched mozill-central on this machine I get 525ms. With my current patches for bug 1180118 and bug 1180120 applied I get 12ms. (Chrome is 4ms.)
FWIW I think this may improve my workload with the SMS app. I'll test a build with the patches tomorrow and I'll report. If there are still issues I'll do a simpler testcase.
Attached file testcase-restyle.html (deleted) —
When changing the class, it seems to trigger a restyle on its children even that I only change "transform" as a result of the class change. I tried this with the patches for bug 1180120 and bug 1180118. There are more children than what I have in the SMS application, but the SMS application has also a lot more CSS.
TBH I'm not completely sure of what I see with this testcase -- the profiler I use in Firefox seems to get interferences from other sources.
Thanks for the test case Julien. It looks like the patches in those two bugs help with most of the restyles that occur but not for the final restyle at the end of the animation. Please try applying the patch from bug 1191600 on top of those from the other two bugs.
With all 3 patches on top of latest mozilla-central, the testcase is perfectly smooth, while it is lagging enormously on Aurora. This is a really impressive improvement !
Flags: needinfo?(pwalton)
Declaring this bug done. There is one regression bug still open but I've got patches for it.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: