Closed
Bug 1368290
Opened 7 years ago
Closed 7 years ago
stylo: Implement ComputedValues sharing for text and anonymous boxes
Categories
(Core :: CSS Parsing and Computation, enhancement, P1)
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox55 | --- | wontfix |
firefox56 | --- | wontfix |
firefox57 | --- | fixed |
People
(Reporter: bzbarsky, Assigned: emilio)
References
(Blocks 3 open bugs)
Details
Attachments
(1 file, 1 obsolete file)
Right now we have ComputedValues sharing (via using the same style context) for sibling textnodes, but not across cousins. In practice that means we can end up with lots of textnodes that could be sharing ComputedValues but are not; see data in bug 1367862. We should probably figure out a sane way to do cousin sharing here.
Comment 1•7 years ago
|
||
I think the best would probably be to have an ephemeral nsRefPtrHashtable mapping from element style to text style on the nsFrameConstructorState. That would get sharing at least within a given restyle generation without bloating ComputedValues with another pointer.
Reporter | ||
Comment 2•7 years ago
|
||
Do we have nsFrameConstructorState while doing restyling or the equivalent of ReparentStyleContext?
Reporter | ||
Comment 3•7 years ago
|
||
So... I was digging into this more today. Do we even have effective sharing for siblings? For the restyle post-traversal we do, but if we're doing frame construction, don't we just end up doing a separate nsStyleContext and hence separate ComputedValues for every textnode? As for the restyle case, we definitely don't have an nsFrameConstructorState in ProcessPostTraversalForText or its callees.
Comment 4•7 years ago
|
||
(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #2) > Do we have nsFrameConstructorState while doing restyling or the equivalent > of ReparentStyleContext? Well, no. That would be ServoRestyleManager::ProcessPostTraversal, which is its own thing. We could add an equivalent hashtable there. (In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #3) > So... I was digging into this more today. Do we even have effective sharing > for siblings? > > For the restyle post-traversal we do, but if we're doing frame construction, > don't we just end up doing a separate nsStyleContext and hence separate > ComputedValues for every textnode? I think that is accurate, unfortunately. Needs fixing.
Updated•7 years ago
|
Assignee: nobody → bzbarsky
Priority: -- → P1
Updated•7 years ago
|
Blocks: stylo-memory
Comment 5•7 years ago
|
||
We decided on IRC to hang these in a linked list off the ServoStyleContext. Same for anonymous boxes (bug 1368291).
Updated•7 years ago
|
Summary: stylo: Implement ComputedValues sharing across cousin textnodes → stylo: Implement ComputedValues sharing for text and anonymous boxes
Comment 6•7 years ago
|
||
Emilio said he had time to knock this out this week before 56 goes to beta.
Assignee: bzbarsky → emilio+bugs
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 9•7 years ago
|
||
mozreview-review |
Comment on attachment 8889659 [details] Bug 1368290: Cache anon box styles in ServoStyleContext. https://reviewboard.mozilla.org/r/160718/#review165948
Attachment #8889659 -
Flags: review?(bobbyholley) → review+
Comment 10•7 years ago
|
||
mozreview-review |
Comment on attachment 8889660 [details] Bug 1368290: Cache text styles in ServoStyleContext. https://reviewboard.mozilla.org/r/160720/#review165958 per IRC discussion I think we can do this without the extra heap allocation.
Attachment #8889660 -
Flags: review?(bobbyholley) → review-
Assignee | ||
Comment 11•7 years ago
|
||
So, I got a patch, but it leaks the world, and trying to understand how this all works and why my patch doesn't work (which I don't know why immediately), I became really confused about how the fusing stuff works. So when creating a new ServoStyleContext, we create it with an mRefCnt == 0. And we store that in the rust side of things, and then return that from the multiple _Foo_Inherit functions. Which means that if I add this assertion to autoland (without my patches), when we create, e.g., text styles, we fail the assertion (we have zero references): diff --git a/layout/style/ServoStyleSet.cpp b/layout/style/ServoStyleSet.cpp index a1e6295075e4..df5f52ade61b 100644 --- a/layout/style/ServoStyleSet.cpp +++ b/layout/style/ServoStyleSet.cpp @@ -365,6 +365,7 @@ ServoStyleSet::ResolveStyleForText(nsIContent* aTextNode, nsCSSAnonBoxes::mozText, aParentContext, InheritTarget::Text).Consume(); + MOZ_ASSERT(computedValues->HasSingleReference()); return computedValues.forget(); } Manish, Bobby, how's this supposed to work? I'm _super_ confused right now.
Flags: needinfo?(manishearth)
Flags: needinfo?(bobbyholley)
Assignee | ||
Comment 12•7 years ago
|
||
Oh, of course we don't even use the mRefCnt field at all, which makes sense. But that means that then we never arrive to ServoStyleContext::Destroy, which is supposed to call the destructor of ServoStyleContext. Which explains my leak. But how are we supposed to call ~nsStyleContext to decrement the refcount of mParent? I'm still really confused, this is a huge mess :(
Assignee | ||
Comment 13•7 years ago
|
||
But then there's Gecko_ServoStyleContext_Destroy, which _should_ call the ServoStyleContext destructor...
Assignee | ||
Comment 14•7 years ago
|
||
Ok, I think I know why my patch causes the leak, but I still don't understand why don't we leak already.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 17•7 years ago
|
||
So now I find obvious why this leaks... And it's because of the cyclic parent/child reference. Best idea to fix it is getting rid of mParent so far...
Comment 18•7 years ago
|
||
I think Emilio figured it out, but for posterity: the design here is that ServoStyleContext is arc-managed. So if you have a RefPtr<ServoStyleContext> or RefPtr<nsStyleContext>, dropping that will call into Servo_StyleContext_Release, which will loop back into Gecko_ServoStyleContext_Destroy. nsStyleContext::mRefCnt was unused, and bug 1384114 is right to move it into GeckoStyleContext. As for the cyclic reference: that is indeed a problem. Eliminating mParent would be great, at least for style contexts with a non-null mPseudoTag. Do you think that's tractable?
Flags: needinfo?(bobbyholley)
Comment 19•7 years ago
|
||
mozreview-review |
Comment on attachment 8889660 [details] Bug 1368290: Cache text styles in ServoStyleContext. https://reviewboard.mozilla.org/r/160720/#review166370 Canceling review for now because we need to deal with the cyclic reference thing, but this generally looks to me like the right approach.
Attachment #8889660 -
Flags: review?(bobbyholley)
Comment 20•7 years ago
|
||
I think we can eliminate mparent for the simple cases. Style contexts in NAC may still leak though? IIRC sometimes the pseudo is non null for notrivial stuff in NAC. Ideally mparent should go away completely for servo. I tried doing this in my patches but it was a lot of extra unnecessary stuff. Might be simpler post all the fix-ups.
Flags: needinfo?(manishearth)
Reporter | ||
Comment 21•7 years ago
|
||
The only comment I have on the patches is that not caching for kids of anon box styles could come back to bite us in some cases (e.g. pages that have lots of "display: table" bits with block kids or something).
Reporter | ||
Comment 22•7 years ago
|
||
Notes from IRC: 1) We should just do the linked list like in the attached patch for now. 2) If we run into it being a problem, we could use two words: one for linked list head, one for next link. 3) We could also try to change where anon boxes inherit from (e.g. to the nearest non-anon-box ancestor). That would mean nothing ever inherits from anon boxes. It's not clear whether it's OK to do this, necessarily. For example, if an anon box can ever have a scrollframe, we really do want the scrolledcontent to inherit from the anon box, not from some ancestor of the anon box.
Updated•7 years ago
|
Priority: P1 → --
Reporter | ||
Updated•7 years ago
|
Priority: -- → P1
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8889660 -
Attachment is obsolete: true
Assignee | ||
Comment 24•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f9bd862221520afa39a6cb80c9f7ae47f339122
Assignee | ||
Comment 25•7 years ago
|
||
Bobby, was this version of the patch reviewed? I'm pretty confused.
Flags: needinfo?(bobbyholley)
Assignee | ||
Comment 26•7 years ago
|
||
Comment on attachment 8889659 [details] Bug 1368290: Cache anon box styles in ServoStyleContext. Well, just in case it wasn't... If it was, please re-stamp it.
Flags: needinfo?(bobbyholley)
Attachment #8889659 -
Flags: review+ → review?(bobbyholley)
Comment 27•7 years ago
|
||
mozreview-review |
Comment on attachment 8889659 [details] Bug 1368290: Cache anon box styles in ServoStyleContext. https://reviewboard.mozilla.org/r/160718/#review170366 This looks great, thank you! ::: layout/style/ServoStyleContext.h:45 (Diff revision 3) > + if (IsInheritingAnonBox()) { > + return; > + } Add a comment here? ::: layout/style/nsCSSAnonBoxes.h:62 (Diff revision 3) > #undef CSS_NON_INHERITING_ANON_BOX > #undef CSS_ANON_BOX > false; > } > > +#ifdef DEBUG Nit: Given that this is inline, probably don't need to make it debug-only. That will avoid the footgun where somebody tries to use it, and it compiles locally, but breaks release builds.
Attachment #8889659 -
Flags: review?(bobbyholley) → review+
Reporter | ||
Comment 28•7 years ago
|
||
> probably don't need to make it debug-only
It does a slow linear search. People really shouldn't be using it for anything outside of assertions. I had in fact been going to object to it being in the patch when I first read it a while back, until I realized it _was_ debug-only.
Comment 29•7 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #28) > > probably don't need to make it debug-only > > It does a slow linear search. People really shouldn't be using it for > anything outside of assertions. I had in fact been going to object to it > being in the patch when I first read it a while back, until I realized it > _was_ debug-only. Ok fair enough. In that case probably add a comment indicating why it shouldn't be used in release builds.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 32•7 years ago
|
||
Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/autoland/rev/7dcc0236da00 Cache anon box styles in ServoStyleContext. r=bholley
Comment 33•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7dcc0236da00
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Updated•7 years ago
|
status-firefox55:
--- → wontfix
status-firefox56:
--- → wontfix
status-firefox-esr52:
--- → unaffected
Comment 34•7 years ago
|
||
https://hg.mozilla.org/projects/date/rev/7dcc0236da0039ac33b270579853a8de3f491c80 Bug 1368290: Cache anon box styles in ServoStyleContext. r=bholley
You need to log in
before you can comment on or make changes to this bug.
Description
•