Closed
Bug 1329877
Opened 8 years ago
Closed 8 years ago
Consider improving the TreeMatchContext handling under CreateNeededFrames
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla54
People
(Reporter: bzbarsky, Assigned: emilio)
References
(Blocks 1 open bug)
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.
Updated•8 years ago
|
Blocks: FastReflows
Comment 1•8 years ago
|
||
(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.
Reporter | ||
Comment 2•8 years ago
|
||
Ah, if the styles are all computed before CreateNeededFrames ever runs, then yes, I agree.
Assignee | ||
Comment 3•8 years ago
|
||
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)
Assignee | ||
Comment 4•8 years ago
|
||
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.
Reporter | ||
Comment 5•8 years ago
|
||
So that patch basically fixes this bug for the specific case of stylo, where we never use the TreeMatchContext?
Flags: needinfo?(bzbarsky)
Comment 6•8 years ago
|
||
I don't understand - TreeMatchContext is totally orthogonal to stylo.
Comment 7•8 years ago
|
||
(Or rather, it is unnecessary / unused in stylo)
Reporter | ||
Comment 8•8 years ago
|
||
Do we avoid the work of initing the bloom filter with stylo right now?
Assignee | ||
Comment 9•8 years ago
|
||
(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.
Assignee | ||
Comment 10•8 years ago
|
||
(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.
Assignee | ||
Comment 11•8 years ago
|
||
(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).
Reporter | ||
Comment 12•8 years ago
|
||
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.
Assignee | ||
Comment 13•8 years ago
|
||
Right, I think it shouldn't be extremely hard, I might poke at it if I have the time.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 15•8 years ago
|
||
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.
Assignee | ||
Comment 16•8 years ago
|
||
Try looks green, needinfoing boris who offered to review this.
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 17•8 years ago
|
||
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.
Reporter | ||
Comment 18•8 years ago
|
||
mozreview-review |
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+
Reporter | ||
Updated•8 years ago
|
Flags: needinfo?(bzbarsky)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 20•8 years ago
|
||
mozreview-review-reply |
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.
Comment 21•8 years ago
|
||
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ae0efe63cb9b
Optimize AncestorFilter usage in lazy frame construction. r=bz
Comment 22•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Updated•8 years ago
|
Assignee: nobody → emilio+bugs
Reporter | ||
Comment 23•8 years ago
|
||
> 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.
Description
•