Closed Bug 1372061 Opened 7 years ago Closed 7 years ago

stylo: StyleChildrenIterator::IsNeeded may be hot, and it's slow.

Categories

(Core :: CSS Parsing and Computation, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: emilio, Assigned: heycam)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 3 obsolete files)

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
Depends on: 1372068
Priority: -- → P1
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?
Flags: needinfo?(cam)
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).
Blocks: 1373416
Sure, I'll look at this over the weekend.
Assignee: nobody → cam
Status: NEW → ASSIGNED
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.
Flags: needinfo?(cam)
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
Attachment #8881199 - Flags: review?(bobbyholley)
Attachment #8881438 - Flags: review?(bobbyholley)
Attachment #8881439 - Flags: review?(bobbyholley)
Attachment #8881200 - Flags: review?(bobbyholley)
Attachment #8881201 - Flags: review?(bobbyholley)
Attachment #8881202 - Flags: review?(bobbyholley)
(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
Attachment #8881199 - Flags: review?(bobbyholley) → review+
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.
Attachment #8881438 - Flags: review?(bobbyholley) → review+
Comment on attachment 8881439 [details]
style: Remove unused TElement::children_and_traversal_children_might_differ.

https://reviewboard.mozilla.org/r/152518/#review157722
Attachment #8881439 - Flags: review?(bobbyholley) → review+
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.
Attachment #8881200 - Flags: review?(bobbyholley) → review+
Comment on attachment 8881201 [details]
Bug 1372061 - Change StyleChildrenIterator FFI functions to use placement new/delete.

https://reviewboard.mozilla.org/r/152472/#review157736
Attachment #8881201 - Flags: review?(bobbyholley) → review+
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
Attachment #8881202 - Flags: review?(bobbyholley) → review+
Blocks: 1374999
Attachment #8881439 - Attachment is obsolete: true
Attachment #8881200 - Attachment is obsolete: true
Attachment #8881202 - Attachment is obsolete: true
Blocks: 1376647
Comment on attachment 8881662 [details]
Bug 1372061 - Part 4: Test expectation adjustment.

https://reviewboard.mozilla.org/r/152828/#review157952
Attachment #8881662 - Flags: review?(cam) → review+
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
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.
Flags: needinfo?(cam)
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bcfb78bcc781
Shuffle some fields around to avoid bindgen issues. r=emilio
Flags: needinfo?(cam)
You need to log in before you can comment on or make changes to this bug.