So I was re-profiling bug 1213122, and tl;dr: Bug 1357583 is worth it, and makes us restyle way less (also, it's plausible that mozreview has changed stuff{). But still, we spend a bunch of time in the function added there. What I didn't expect to find is more time in Gecko_MaybeCreateStyleChildrenIterator than doing work: * https://perfht.ml/2rQkm3L

This function doesn't seem to be very optimizable... It seems most of the time is taken inside the function itself, but the function itself doesn't look very expensive at all, just some condition check... There are several virtual calls, though, from IsInAnonymousSubtree(), which calls nsINode::GetBindingParent() in common cases, and do_QueryFrame. Maybe we can try devirtualizing them... which doesn't seem to be trivial, though.

I've been profiling and this keeps coming up. The profile I'm looking at now has us spending 1.6 ms on this when restyling wikipedia. The overhead seems to be about 30% the GetProperty (which is out-of-line), and presumably most of the rest is the QueryFrame. The worst part is that this number doesn't improve with style sharing, since it's overhead in the traversal itself. It seems to me that what we should do is to just set a bit on the element whenever one of the conditions happens that would require a StyleChildIterator. bz recently freed up some node bits, so we should grab them while they're hot. We also spend a smaller (but measurable) amount of time heap-allocating the StyleChildrenIterators. It occurs to me that this is dumb - we should just stack-allocate the iterators on the Rust side, and then call into C++ to placement-new/destroy like we do with the style structs. The overhead of this is significantly less than that of IsNeeded though, so it's less urgent if it ends up being a pain somehow. Cam, do you have cycles to knock this one out?

Note that this is one of the things that causes restyling to be slower than the initial styling pass, because with the initial styling pass we don't have a frame, and so we skip the QueryFrame (and I also suspect we're less likely to have the "might have node properties" node bit).

Sure, I'll look at this over the weekend.

Green try run with the first two patches applied: https://treeherder.mozilla.org/#/jobs?repo=try&revision=53221a0d748d850d2b741b092053053dd5e354c9 Stack-based StyleChildrenIterator creation not working yet though.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4473e3b6d0c18758ebf04da63a9e4331aa0de1b Haven't profiled yet to see how much difference it makes; will do that tonight. Also, one downside of this approach is that once an element gets anonymous content (from ::before/::after, or from NAC or XBL bindings), we'll forever choose to use a StyleChildrenIterator. It seems like we should be able to tell from the result of using the StyleChildrenIterator whether we no longer need to, by keeping a track of whether it vends any anonymous content, and updating the flag if it got to the end and didn't. We can only do that when we have exclusive access to the element, which we do indeed have in, say, preprocess_children, but I'm still thinking of the best way to interact with the LayoutIterator at that point to tell it to update the flag without it breaking our TElement abstraction too badly.

On the Obama wikipedia page on my laptop, with a full page restyle: Without patches: 64.6 ms in Servo_TraverseSubtree 0.8 ms in Gecko_MaybeCreateStyleChildrenIterator With patches: 64.2 ms - Servo_TraverseSubtree 0.2 ms - Gecko_MaybeConstructStyleChildrenIterator

(In reply to Cameron McCormack (:heycam) from comment #17) > On the Obama wikipedia page on my laptop, with a full page restyle: > > Without patches: > > 64.6 ms in Servo_TraverseSubtree > 0.8 ms in Gecko_MaybeCreateStyleChildrenIterator We did another run, and the that one had 2.2 ms in MaybeCreateStyleChildrenIterator. So it fluctuates a bit, but this patch probably saves us at least 1ms on average.

Comment on attachment 8881199 [details] Bug 1372061 - Add node flag recording whether we might have anonymous children. https://reviewboard.mozilla.org/r/152468/#review157708

Comment on attachment 8881438 [details] Bug 1372061 - Remove unused StyleChildrenIterator::IsNeeded. https://reviewboard.mozilla.org/r/152516/#review157720 ::: servo/components/style/gecko/wrapper.rs:389 (Diff revision 1) > + fn get_binding_with_content<'a>(&self) -> Option<GeckoXBLBinding<'a>> { > + unsafe { Fix the lifetimes here per IRL discussion. ::: servo/components/style/gecko/wrapper.rs:392 (Diff revision 1) > + loop { > + if !(*binding).mContent.raw::<nsIContent>().is_null() { Add a comment saying this mirrors GetBindingWithContent.

Comment on attachment 8881439 [details] style: Remove unused TElement::children_and_traversal_children_might_differ. https://reviewboard.mozilla.org/r/152518/#review157722

Comment on attachment 8881200 [details] style: Use faster check to determine whether a StyleChildrenIterator is needed. https://reviewboard.mozilla.org/r/152470/#review157732 ::: servo/components/style/gecko/wrapper.rs:527 (Diff revision 2) > + // FIXME(heycam): Having trouble with bindgen on nsXULElement, > + // where the binding parent is stored in a member variable > + // rather than in slots. So just get it through FFI for now. Elaborate on this comment? ::: servo/components/style/gecko/wrapper.rs:533 (Diff revision 2) > + self.get_dom_slots() > + .and_then(|slots| { > + unsafe { (*slots.__bindgen_anon_1.mBindingParent.as_ref()).as_ref() } > + }) Let's separate out the part that returns the pointer and then use that directly from our caller? That will avoid this as_element overhead.

Comment on attachment 8881201 [details] Bug 1372061 - Change StyleChildrenIterator FFI functions to use placement new/delete. https://reviewboard.mozilla.org/r/152472/#review157736

Comment on attachment 8881201 [details] Bug 1372061 - Change StyleChildrenIterator FFI functions to use placement new/delete. https://reviewboard.mozilla.org/r/152472/#review157740 ::: layout/style/ServoBindings.cpp:216 (Diff revision 2) > - const Element* el = aNode->AsElement(); > - return StyleChildrenIterator::IsNeeded(el) ? new StyleChildrenIterator(el) > - : nullptr; > + if (!StyleChildrenIterator::IsNeeded(aElement)) { > + return false; > + } Per discussion, let's remove this check, and remove the boolean return value of this function.

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #24) > Per discussion, let's remove this check, and remove the boolean return value > of this function. Oh, and I guess also remove the "Maybe" from the name.

Comment on attachment 8881202 [details] style: Use placement new/delete StyleChildrenIterator FFI functions. https://reviewboard.mozilla.org/r/152474/#review157746

Comment on attachment 8881662 [details] Bug 1372061 - Part 4: Test expectation adjustment. https://reviewboard.mozilla.org/r/152828/#review157952

Pushed by cmccormack@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a91a72d500c2 Add node flag recording whether we might have anonymous children. r=bholley https://hg.mozilla.org/integration/autoland/rev/d0e5ada9e9cf Change StyleChildrenIterator FFI functions to use placement new/delete. r=bholley https://hg.mozilla.org/integration/autoland/rev/1056830def2d Remove unused StyleChildrenIterator::IsNeeded. r=bholley https://hg.mozilla.org/integration/autoland/rev/efba066ec857 Part 4: Test expectation adjustment. r=heycam

https://hg.mozilla.org/mozilla-central/rev/a91a72d500c2 https://hg.mozilla.org/mozilla-central/rev/d0e5ada9e9cf https://hg.mozilla.org/mozilla-central/rev/1056830def2d https://hg.mozilla.org/mozilla-central/rev/efba066ec857

Pushed by cmccormack@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/39f4723df4a3 Shuffle some fields around to avoid bindgen issues. r=emilio

Backed out for build bustage: ChildIterator.h:43:7: field 'mIndexInInserted' will be initialized after field 'mIsFirst': https://hg.mozilla.org/integration/autoland/rev/5b7416327dc2320efc4d9f690b25354108569d87 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=39f4723df4a3d02b41af6e406152e5756bf1b031&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=110987918&repo=autoland > /home/worker/workspace/build/src/dom/base/ChildIterator.h:43:7: error: field 'mIndexInInserted' will be initialized after field 'mIsFirst' [-Werror,-Wreorder] etc.

Pushed by cmccormack@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/bcfb78bcc781 Shuffle some fields around to avoid bindgen issues. r=emilio