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)

Product:
Core
Component:
CSS Parsing and Computation
Version:
53 Branch
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
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)

Bug 1368290: Cache anon box styles in ServoStyleContext.
59 bytes, text/x-review-board-request
bholley
: review+
Details 
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.
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.
Do we have nsFrameConstructorState while doing restyling or the equivalent of ReparentStyleContext?
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.
(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.
Blocks: 1367862
Assignee: nobody → bzbarsky
Priority: -- → P1
Blocks: 1373430
We decided on IRC to hang these in a linked list off the ServoStyleContext. Same for anonymous boxes (bug 1368291).
Depends on: 1373018
Summary: stylo: Implement ComputedValues sharing across cousin textnodes → stylo: Implement ComputedValues sharing for text and anonymous boxes
Blocks: 1368291
Emilio said he had time to knock this out this week before 56 goes to beta.
Assignee: bzbarsky → emilio+bugs
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 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-
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)
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 :(
Depends on: 1384114
But then there's Gecko_ServoStyleContext_Destroy, which _should_ call the ServoStyleContext destructor...
Ok, I think I know why my patch causes the leak, but I still don't understand why don't we leak already.
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...
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 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)
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)
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).
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.
Depends on: 1384542
Blocks: 1384945
Depends on: 1385896
Priority: P1 → --
Priority: -- → P1
Blocks: 1369902
Attachment #8889660 - Attachment is obsolete: true
Blocks: 1387120
Bobby, was this version of the patch reviewed? I'm pretty confused.
Flags: needinfo?(bobbyholley)
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 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+
> 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.
(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.
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/7dcc0236da00
Cache anon box styles in ServoStyleContext. r=bholley
https://hg.mozilla.org/mozilla-central/rev/7dcc0236da00
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: