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

RESOLVED FIXED in Firefox 56

Status

()

enhancement
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: emilio, Assigned: heycam)

Tracking

(Blocks 1 bug)

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(4 attachments, 3 obsolete attachments)

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
Reporter

Updated

2 years ago
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
Assignee

Comment 4

2 years ago
Sure, I'll look at this over the weekend.
Assignee: nobody → cam
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Assignee

Comment 9

2 years ago
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.
Assignee

Comment 10

2 years ago
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)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 17

2 years ago
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
Assignee

Updated

2 years ago
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 19

2 years ago
mozreview-review
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 20

2 years ago
mozreview-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 21

2 years ago
mozreview-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 22

2 years ago
mozreview-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 23

2 years ago
mozreview-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 24

2 years ago
mozreview-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 26

2 years ago
mozreview-review
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+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Blocks: 1374999
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Attachment #8881439 - Attachment is obsolete: true
Assignee

Updated

2 years ago
Attachment #8881200 - Attachment is obsolete: true
Assignee

Updated

2 years ago
Attachment #8881202 - Attachment is obsolete: true
Assignee

Updated

2 years ago
Blocks: 1376647
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 41

2 years ago
mozreview-review
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+

Comment 42

2 years ago
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

Comment 44

2 years ago
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)

Comment 46

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

Updated

2 years ago
Flags: needinfo?(cam)
You need to log in before you can comment on or make changes to this bug.