Closed Bug 1329877 Opened 4 years ago Closed 4 years ago

Consider improving the TreeMatchContext handling under CreateNeededFrames

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox53 --- affected
firefox54 --- fixed

People

(Reporter: bzbarsky, Assigned: emilio)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file)

The profile in bug 1329601 shows us spending 35ms creating/initializing TreeMatchContexts under the CreateNeededFrames stacks.  I expect that what's happening is that we have a deep tree and a node in it that has a bunch of kids needing construction or something.  We walk down the tree and construct them all; that needs an nsFrameConstructorState each time and hence a TreeMatchContext.  So we create tons of TreeMatchContexts.

We could do better: at the initial entrypoint to CreateNeededFrames we could create a TreeMatchContext, then maintain it as we go down the tree, and have the nsFrameConstructorState use our existing TreeMatchContext when we need it.  This should be cheaper if there are multiple things to construct in there, I would think.

The only question is whether this is worth doing at all, given that stylo may remove TreeMatchContext anyway.  But note that stylo may want some sort of equivalent optimization, though it would be a bit harder to do just because our tree traversal is on the gecko side but the bloom filter maintenance needs to be on the servo side.
Blocks: FastReflows
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #0)
> The only question is whether this is worth doing at all, given that stylo
> may remove TreeMatchContext anyway.  But note that stylo may want some sort
> of equivalent optimization, though it would be a bit harder to do just
> because our tree traversal is on the gecko side but the bloom filter
> maintenance needs to be on the servo side.

Why would stylo need this? All the styles (modulo NAC and XBL styles) are computed eagerly, so there's no styling work to be done in CreateNeededFrames.
Ah, if the styles are all computed before CreateNeededFrames ever runs, then yes, I agree.
See also that I have a patch that does this (still not during CreateNeededFrames, but should be trivially doable), under bug 1301859.

Boris, would you be interested in getting it rebased and review it?
Flags: needinfo?(bzbarsky)
Well, doesn't do this exactly (didn't read the whole story), but should help with restyling. I'm also sort of confident with how it works now, so I could try to look at this too.
So that patch basically fixes  this bug for the specific case of stylo, where we never use the TreeMatchContext?
Flags: needinfo?(bzbarsky)
I don't understand - TreeMatchContext is totally orthogonal to stylo.
(Or rather, it is unnecessary / unused in stylo)
Do we avoid the work of initing the bloom filter with stylo right now?
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #5)
> So that patch basically fixes  this bug for the specific case of stylo,
> where we never use the TreeMatchContext?

Nope, that's a patch for Gecko's style system, that avoids populating the bloom filter for stuff like transitions/animations and restyle attributes.

In stylo we avoid the bloom filter during styling, but not during frame construction yet.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #9)
> (In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #5)
> > So that patch basically fixes  this bug for the specific case of stylo,
> > where we never use the TreeMatchContext?
> 
> Nope, that's a patch for Gecko's style system, that avoids populating the
> bloom filter for stuff like transitions/animations and restyle attributes.
> 
> In stylo we avoid the bloom filter during styling, but not during frame
> construction yet.

Stylo also doesn't repopulate the bloom filter if it doesn't need to run selector matching, in case that was your question.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #9)
> In stylo we avoid the bloom filter during styling, but not during frame
> construction yet.

And to be clear, with this I mean we avoid _Gecko's_ bloom filter.

(In reply to Emilio Cobos Álvarez [:emilio] from comment #10)
> Stylo also doesn't repopulate the bloom filter if it doesn't need to run
> selector matching, in case that was your question.

And with this I mean Servo's bloom filter (http://searchfox.org/mozilla-central/source/servo/components/style/bloom.rs).
OK.  So that patch doesn't really seem related to this bug, since under CreateNeededFrames in the _non-stylo_ case we definitely need to do matching.

It might still be worth doing, of course.
Right, I think it shouldn't be extremely hard, I might poke at it if I have the time.
I had the time to hack that up. Passing it through try now (https://treeherder.mozilla.org/#/jobs?repo=try&revision=0ae0fd0060e5af1e648381c18ad0d700301eb719).

The cool part is that with this and the patch in bug 1301859, recreating text frames (which can be quite frequent due to stuff like textContent) should be mostly free in this aspect, since we shouldn't need to do any selector matching there.
Try looks green, needinfoing boris who offered to review this.
Flags: needinfo?(bzbarsky)
Oh, btw, if someone wonders about the constructor tag instead of a helper frame constructor function or similar, is because TreeMatchContext is not copy-constructible now, and even though we can make it so, it seems that keeping it this way could be good to avoid dumb mistakes that involve copying such a large struct.
Comment on attachment 8841765 [details]
Bug 1329877: Optimize AncestorFilter usage in lazy frame construction.

https://reviewboard.mozilla.org/r/115886/#review118760

::: layout/base/nsCSSFrameConstructor.h:90
(Diff revision 1)
>    // Note: It's the caller's responsibility to make sure to wrap a
>    // CreateNeededFrames call in a view update batch and a script blocker.
>    void CreateNeededFrames();
>  
>  private:
> -  void CreateNeededFrames(nsIContent* aContent);
> +  void CreateNeededFrames(nsIContent* aContent, TreeMatchContext&);

Please give the second arg a name.

::: layout/base/nsCSSFrameConstructor.cpp:7204
(Diff revision 1)
>    for (nsIContent* child = iter.GetNextChild(); child; child = iter.GetNextChild()) {
>      if (child->HasFlag(NODE_DESCENDANTS_NEED_FRAMES)) {
> -      CreateNeededFrames(child);
> +      TreeMatchContext::AutoAncestorPusher insertionPointPusher(
> +          aTreeMatchContext);
> +
> +      if (child->GetParent() != aContent && child->GetParent()->IsElement()) {

This is worth a comment explaining why it's needed.

::: layout/base/nsCSSFrameConstructor.cpp:7206
(Diff revision 1)
> -      CreateNeededFrames(child);
> +      TreeMatchContext::AutoAncestorPusher insertionPointPusher(
> +          aTreeMatchContext);
> +
> +      if (child->GetParent() != aContent && child->GetParent()->IsElement()) {
> +        insertionPointPusher.PushAncestorAndStyleScope(
> +            child->GetParent()->AsElement());

I guess we always have a filter here?  Or is there a reason that's not being checked and just the PushStyleScope() bit done if we don't have one?

::: layout/base/nsCSSFrameConstructor.cpp:7514
(Diff revision 1)
>  
>    // Create some new frames
> +  //
> +  // We use the provided tree match context, or create a new one on the fly
> +  // otherwise.
> +  Maybe<TreeMatchContext> matchContext;

We could put the `Maybe<TreeMatchContext>` in the FrameConstructionState, have the ctor take a `TreeMatchContext*`, and have the member people are supposed to use be a `TreeMatchContext*` that points to the provided one if not null, or the Maybe otherwise.

This would allow us to put the (mDocument, TreeMatchContext::ForFrameConstruction) boilerplate in one spot instead of duplicating it....  Thoughts?
Attachment #8841765 - Flags: review+
Flags: needinfo?(bzbarsky)
Comment on attachment 8841765 [details]
Bug 1329877: Optimize AncestorFilter usage in lazy frame construction.

https://reviewboard.mozilla.org/r/115886/#review118760

> Please give the second arg a name.

Done

> This is worth a comment explaining why it's needed.

Done

> I guess we always have a filter here?  Or is there a reason that's not being checked and just the PushStyleScope() bit done if we don't have one?

Yes, we call InitAncestors with the root element when we begin lazy frame construction, so there's always a filter there. I added an assertion.

> We could put the `Maybe<TreeMatchContext>` in the FrameConstructionState, have the ctor take a `TreeMatchContext*`, and have the member people are supposed to use be a `TreeMatchContext*` that points to the provided one if not null, or the Maybe otherwise.
> 
> This would allow us to put the (mDocument, TreeMatchContext::ForFrameConstruction) boilerplate in one spot instead of duplicating it....  Thoughts?

That sounds fine, though it'll be a relatively noisy update of all the uses of `mTreeMatchContext`. I think at that point we could also add an accessor and what not. If you think it's worth I can do it as a followup.
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ae0efe63cb9b
Optimize AncestorFilter usage in lazy frame construction. r=bz
https://hg.mozilla.org/mozilla-central/rev/ae0efe63cb9b
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Assignee: nobody → emilio+bugs
> If you think it's worth I can do it as a followup.

It probably is...
You need to log in before you can comment on or make changes to this bug.