1280772 - cache Servo PropertyDeclarationBlocks for style="" attributes in nsAttrValue objects

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Cameron McCormack (:heycam)]

Currently stylo parses style="" attributes every time style for a given element is computed.  We should store the resulting PropertyDeclarationBlock in the nsAttrValue like we do with the Gecko StyleRule.

Comment 1

[Cameron McCormack (:heycam)]

https://github.com/servo/servo/pull/11787 is the Servo-side change.

Comment 2

[Cameron McCormack (:heycam)]

Attachment: patch

(awaiting bholley's review acceptance to be flipped back on)

Comment 3

[Cameron McCormack (:heycam)]

(In reply to Cameron McCormack (:heycam) (away Jun 25 – Jul 10) from comment #0)
> We should store the resulting PropertyDeclarationBlockin the nsAttrValue like we do with the Gecko StyleRule.

(Well, it's the Declaration rather than the StyleRule, but you know what I mean.)

Comment 4

[Bobby Holley (:bholley)]

Comment on attachment 8763343 [details] [diff] [review]
patch

Review of attachment 8763343 [details] [diff] [review]:
-----------------------------------------------------------------

In general it seems like we could benefit from more unified handling of the servo and gecko cases here, which seems like it would unlock additional optimizations that we probably want.

::: dom/base/nsAttrValue.cpp
@@ +345,5 @@
>      }
> +    case eServoDeclarationBlock:
> +    {
> +      cont->mValue.mServoDeclarationBlock =
> +        Servo_CloneDeclarationBlock(otherCont->mValue.mServoDeclarationBlock);

Seems like we should put these in an Arc and the refcount them from the gecko side, which would remove the need for a clone routine and make the ownership handling similar to what we do for the gecko type.

@@ +1693,5 @@
>                 "This is unexpected");
>  
> +  if (ownerDoc->GetStyleBackendType() == StyleBackendType::Servo) {
> +    // For a Servo-backed style system, we don't share MiscContainers
> +    // for identical style="" attribute values, but we do get Servo

Why do we skip that? It seems like this is probably an important optimization, and on face I don't see why we can't leverage the same machinery.

Comment 5

[Cameron McCormack (:heycam)]

(In reply to Bobby Holley (busy) from comment #4)
> Seems like we should put these in an Arc and the refcount them from the
> gecko side, which would remove the need for a clone routine and make the
> ownership handling similar to what we do for the gecko type.

OK, doing that should unblock sharing in MiscContainers.  I think I was also put off by the fact that these parsed style attributes are cached on the nsHTMLCSSStyleSheet, which we won't use in stylo, but we can still use it for caching purposes even if we never call RulesMatching etc. on it like non-stylo does.

Comment 6

[Cameron McCormack (:heycam)]

Attachment: patch v2

I rebased the mUTF8String hacks patch on top of this one to get:

https://github.com/heycam/gecko-dev/commit/a3a093d9cdab17534a9b59964a5d2f15f6de6ee1

which includes a couple of changes in addition to the rebasing, specifically:

https://github.com/heycam/gecko-dev/commit/a3a093d9cdab17534a9b59964a5d2f15f6de6ee1#diff-631934b00181e7a2235d37056eef2a47R1803

and:

https://github.com/heycam/gecko-dev/commit/a3a093d9cdab17534a9b59964a5d2f15f6de6ee1#diff-631934b00181e7a2235d37056eef2a47R1816

Comment 7

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=427258346f6c

Comment 8

[Cameron McCormack (:heycam)]

Attachment: patch v2.1

Missed a couple of crashing Servo_* stubs for non-MOZ_STYLO.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=40c748443713

Comment 9

[Bobby Holley (:bholley)]

Comment on attachment 8764180 [details] [diff] [review]
patch v2.1

Review of attachment 8764180 [details] [diff] [review]:
-----------------------------------------------------------------

So, the big thing I didn't realize before was the MiscContainer doesn't wrap refcounted types, but rather is refcounted itself. In that case, we effectively have duplicate reference-counting with the current scheme, which isn't great. Additionally, we'd actually need Arc rather than Rc to be type-kosher on the servo side (since we'd be accessing it on arbitrary threads), and we're likely to run into trouble with CSSOM trying to mutate things.

So instead, I think we revert the refcounting on the servo side, which gets rid of the RefCell and lets us mutate the struct directly, and then use MiscContainer to handle the reference counting. Does that seem sensible? Are there any situations where we'd end up with suboptimal behavior with the MiscContainer owning the declaration block?

Would be nice to chat about this sync, but with jet lag I'm going to bed early and our schedules aren't overlapping so much. :\

::: dom/base/nsAttrValue.cpp
@@ +84,5 @@
> +    case nsAttrValue::eCSSDeclaration:
> +      sheet = mValue.mCSSDeclaration->GetHTMLCSSStyleSheet();
> +      break;
> +    case nsAttrValue::eServoDeclarationBlock:
> +      sheet = Servo_GetDeclarationBlockCache(mValue.mServoDeclarationBlock);

I really don't like stashing this random gecko class on the servo declaration block. How about we just pass it as an argument to Cache/Evict, since the only callers there are nsAttrValue methods, which can get the cache off the owner doc?

In the worst case, we could stash the point in the extra string bits on the MiscContainer instead. But hopefully we can avoid doing even that.

@@ +686,5 @@
>      {
> +      // XXXheycam Once we support CSSOM access to them, we should
> +      // probably serialize eServoDeclarationBlocks like this too.
> +      // For now, we will return the string from the MiscContainer
> +      // at the top of this function.

Can you file a bug for CSSOM and make a note there that we need to fix this up?

@@ +1761,5 @@
> +    RefPtr<css::Declaration> declaration =
> +      cssParser.ParseStyleAttribute(aString, docURI, baseURI,
> +                                    aElement->NodePrincipal());
> +    if (declaration) {
> +      parsed = true;

Instead of this, how about just early-returning false if ParseStyleAttribute returns null?

@@ +1886,5 @@
>          {
>            delete cont->mValue.mIntMargin;
>            break;
>          }
> +        case eServoDeclarationBlock:

Maybe use the same block for both the eGeckoDeclaration and eServoDeclaration cases, and then just use an if/else for the different release logic?

@@ +2008,5 @@
>          //n += container->mCSSDeclaration->SizeOfIncludingThis(aMallocSizeOf);
> +      } else if (Type() == eServoDeclarationBlock &&
> +                 container->mValue.mServoDeclarationBlock) {
> +        // As with eCSSDeclaration, but if we do measure we'll need a way
> +        // to call the Servo heap_size_of function for the declaration block.

Can you file a bug on hooking up memory reporters, and mention this line in the comments?

::: dom/base/nsAttrValue.h
@@ +114,5 @@
>      ,eSVGStringList =          0x1F
>      ,eSVGTransformList =       0x20
>      ,eSVGViewBox =             0x21
>      ,eSVGTypesEnd =            eSVGViewBox
> +    ,eServoDeclarationBlock =  0x22

Can you move this under eCSSDeclaration, and remove all of the explicit hex values? We still need the one for 0x10 as well as the one for eSVGAngle and eSVGViewBox, but the rest can be omitted I think.

Also, there are about 20 uses of eCSSDeclaration in the tree. I think it would be worth a patch underneath this one to rename that to eGeckoCSSDeclaration for clarity. Once we do that, we can rename this enum to eServoCSSDeclaration for consistency.

@@ +166,5 @@
>               const nsAString* aSerialized);
>    void SetTo(const mozilla::SVGTransformList& aValue,
>               const nsAString* aSerialized);
>    void SetTo(const nsSVGViewBox& aValue, const nsAString* aSerialized);
> +  void SetToAlreadyAddrefed(ServoDeclarationBlock* aDeclarationBlock,

I was going to suggest making this SetTo(already_AddRefed<ServoDeclarationBlock> ...), but given my suggestion at the top of the review I think this can just be SetTo.

Also, can you move this right under the SetTo for css::Declaration?

@@ +204,5 @@
>    inline mozilla::css::URLValue* GetURLValue() const;
>    inline mozilla::css::ImageValue* GetImageValue() const;
>    inline double GetDoubleValue() const;
>    bool GetIntMarginValue(nsIntMargin& aMargin) const;
> +  inline ServoDeclarationBlock* GetServoDeclarationBlock() const;

Can you move this right under GetCSSDeclarationValue?

::: dom/base/nsAttrValueInlines.h
@@ +47,5 @@
>          const mozilla::SVGAnimatedPreserveAspectRatio* mSVGPreserveAspectRatio;
>          const mozilla::SVGStringList* mSVGStringList;
>          const mozilla::SVGTransformList* mSVGTransformList;
>          const nsSVGViewBox* mSVGViewBox;
> +        ServoDeclarationBlock* mServoDeclarationBlock;

Can you move this under mCSSDeclaration?

@@ +85,5 @@
>    inline bool IsRefCounted() const
>    {
>      // Nothing stops us from refcounting (and sharing) other types of
>      // MiscContainer (except eDoubleValue types) but there's no compelling
>      // reason to 

While you're here - this space should be a period.

::: layout/style/nsHTMLCSSStyleSheet.cpp
@@ +40,5 @@
>      // we're iterating the hashtable.
> +    switch (value->mType) {
> +      case nsAttrValue::eCSSDeclaration: {
> +        css::Declaration* declaration = value->mValue.mCSSDeclaration;
> +        declaration->SetHTMLCSSStyleSheet(nullptr);

If we pass the arguments to Cache and Evict like I propose, then can we nix all this entirely? Or at the very least we could unify this handling on the Miscontainer and stop the declaration itself from needing to know about it.

The fix for this stuff should probably be a dependent patch.

Comment 10

[Cameron McCormack (:heycam)]

(In reply to Bobby Holley (busy) from comment #9)
> So, the big thing I didn't realize before was the MiscContainer doesn't wrap
> refcounted types, but rather is refcounted itself. In that case, we
> effectively have duplicate reference-counting with the current scheme, which
> isn't great. Additionally, we'd actually need Arc rather than Rc to be
> type-kosher on the servo side (since we'd be accessing it on arbitrary
> threads), and we're likely to run into trouble with CSSOM trying to mutate
> things.

Looking a bit closer at the reason css::Declaration objects are reference counted (in addition to their MiscContainers being):

* we can store strong pointers to css::Declarations (through nsIStyleRule) in:
** nsComputedDOMStyle::GetStyleContextForElementNoFlush (for the "UA rules only" API that devtools uses)
** various nsStyleSet APIs
** the nsRuleNode::mRule pointer
* in nsHTMLCSSUtils::GetCSSInlinePropertyBase we have a RefPtr<css::Declaration> assigned with element.GetInlineStyleDeclaration()
* we share css::Declarations when calling nsAttrValue::SetTo(const nsAttrValue&)

I'm not sure yet how we will deal with nsComputedDOMStyle, but it surely won't be by implementing nsIStyleRule on the Servo object.  The other two nsCOMPtr<nsIStyleRule> cases aren't relevant for Stylo, and the RefPtr<> in editor is not necessary.

The SetTo(const nsAttrValue&) is the only one to worry about.  In my first patch I was just cloning the ServoDeclarationBlock.  We do actually run into this, at least here:

https://dxr.mozilla.org/mozilla-central/source/dom/base/Element.cpp#2371

We get in here even during document parsing of style="" attributes, so it would be good to avoid the ServoDeclarationBlock cloning I think.  I'm not sure what's a good way to do that without refcounting the ServoDeclarationBlock.  Maybe sharing the MiscContainer would work here?  (Not in general in SetTo, but at least for the purpose of keeping the old value for the AfterSetAttr call.)

> ::: dom/base/nsAttrValue.cpp
> @@ +84,5 @@
> > +    case nsAttrValue::eCSSDeclaration:
> > +      sheet = mValue.mCSSDeclaration->GetHTMLCSSStyleSheet();
> > +      break;
> > +    case nsAttrValue::eServoDeclarationBlock:
> > +      sheet = Servo_GetDeclarationBlockCache(mValue.mServoDeclarationBlock);
> 
> I really don't like stashing this random gecko class on the servo
> declaration block. How about we just pass it as an argument to Cache/Evict,
> since the only callers there are nsAttrValue methods, which can get the
> cache off the owner doc?

Oh, great point.

Comment 11

[Cameron McCormack (:heycam)]

(In reply to Cameron McCormack (:heycam) (away Jun 25 – Jul 10) from comment #10)
> > I really don't like stashing this random gecko class on the servo
> > declaration block. How about we just pass it as an argument to Cache/Evict,
> > since the only callers there are nsAttrValue methods, which can get the
> > cache off the owner doc?
> 
> Oh, great point.

We call Evict in nsAttrValue::ClearMiscContainer, from which we don't have access to the owner doc.  This can be called from many of the nsAttrValue methods (all the SetTo methods, operator=, and the Parse* methods, at least).  We would have to pass the owner doc through to all of those, which doesn't seem great.

I don't think we can store the nsHTMLCSSStyleSheet pointer in mStringBits since we do also store the Declaration's serialization in there (contradicting the comment in nsAttrValueInlines.h above mStringBits) and so we will want to do that for ServoDeclarationBlocks too.  See the SetMiscAtomOrString call in nsAttrValue::SetTo(css::Declaration*, ...).

Comment 12

[Cameron McCormack (:heycam)]

So maybe the best thing is just to have:

  struct ServoStyleAttribute {
    ServoDeclarationBlock* mDeclarations;
    nsHTMLCSSStyleSheet* mSheet;
    bool mImmutable;
  };

and create an owned one of those to store in the MiscContainer.

Comment 13

[Cameron McCormack (:heycam)]

And then we could make that struct refcounted, avoiding the need to add refcounting to ServoDeclarationBlock itself or cloning it in SetTo.

Comment 14

[Cameron McCormack (:heycam)]

As discussed on IRC http://logs.glob.uno/?c=mozilla%23layout#c16786 we can avoid cloning the ServoDeclarationBlock altogether because we can just share the MiscContainer.  Also, we will go ahead with storing the nsHTMLCSSStyleSheet/bool on the Servo side (although we don't need to wrap them in RefCells like I do in my current PR).

Comment 15

[Cameron McCormack (:heycam)]

Attachment: Part 1: Remove explicit numbering of MiscContainer-residing nsAttrValue types.

Comment 16

[Cameron McCormack (:heycam)]

Attachment: Part 2: Rename eCSSDeclaration to eGeckoCSSDeclaration.

Comment 17

[Cameron McCormack (:heycam)]

Attachment: Part 3: Store Servo-parsed style="" attributes in nsAttrValues. (v3)

Comment 18

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=68bfc9f1f48e756b8557e1bd7b5bfc71a85af0ba

Comment 19

[Bobby Holley (:bholley)]

Comment on attachment 8764474 [details] [diff] [review]
Part 2: Rename eCSSDeclaration to eGeckoCSSDeclaration.

Review of attachment 8764474 [details] [diff] [review]:
-----------------------------------------------------------------

Nice. Sorry this turned out to be bigger than I thought.

Comment 20

[Bobby Holley (:bholley)]

Comment on attachment 8764475 [details] [diff] [review]
Part 3: Store Servo-parsed style="" attributes in nsAttrValues. (v3)

Review of attachment 8764475 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comments addressed, but please make sure to look at the ReparseStyleAttribute thing.

::: dom/base/nsAttrValue.cpp
@@ +601,5 @@
>    SetSVGType(eSVGViewBox, &aValue, aSerialized);
>  }
>  
>  void
> +nsAttrValue::SetTo(ServoDeclarationBlock* aValue,

Can you move this below the css::Declaration overload?

@@ +1080,5 @@
>      case eIntMarginValue:
>      {
>        return thisCont->mValue.mIntMargin == otherCont->mValue.mIntMargin;
>      }
> +    case eServoCSSDeclaration:

Can you hoist this below the eGeckoCSSDeclaration case?

@@ +2000,5 @@
>          //n += container->mGeckoCSSDeclaration->SizeOfIncludingThis(aMallocSizeOf);
> +      } else if (Type() == eServoCSSDeclaration &&
> +                 container->mValue.mServoCSSDeclaration) {
> +        // As with eGeckoCSSDeclaration, but if we do measure we'll need a way
> +        // to call the Servo heap_size_of function for the declaration block.

Reference the bug # requested in the previous review?

::: dom/base/nsAttrValueInlines.h
@@ +187,5 @@
>    return true;
>  }
>  
> +inline ServoDeclarationBlock*
> +nsAttrValue::GetServoCSSDeclarationValue() const

Can you hoist this next to the corresponding gecko getter?

::: dom/base/nsStyledElement.cpp
@@ +131,5 @@
>    }
>    const nsAttrValue* oldVal = mAttrsAndChildren.GetAttr(nsGkAtoms::style);
>    
> +  if (oldVal && oldVal->Type() != nsAttrValue::eGeckoCSSDeclaration &&
> +      OwnerDoc()->GetStyleBackendType() == StyleBackendType::Gecko) {

Don't we need to handle the servo case here?

::: layout/style/ServoBindings.h
@@ +186,5 @@
>                                    RawServoStyleSet* set);
>  bool Servo_StyleSheetHasRules(RawServoStyleSheet* sheet);
>  RawServoStyleSet* Servo_InitStyleSet();
>  void Servo_DropStyleSet(RawServoStyleSet* set);
> +ServoDeclarationBlock* Servo_ParseStyleAttribute(const uint8_t* bytes,

Maybe group all these together with a comment?

Comment 21

[Cameron McCormack (:heycam)]

(In reply to Bobby Holley (busy) from comment #20)
> ::: dom/base/nsStyledElement.cpp
> @@ +131,5 @@
> >    }
> >    const nsAttrValue* oldVal = mAttrsAndChildren.GetAttr(nsGkAtoms::style);
> >    
> > +  if (oldVal && oldVal->Type() != nsAttrValue::eGeckoCSSDeclaration &&
> > +      OwnerDoc()->GetStyleBackendType() == StyleBackendType::Gecko) {
> 
> Don't we need to handle the servo case here?

Ah, yes we do.  Will make it check against the expected nsAttrValue::ValueType for the document's style backend type.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b83dbe4071af

Comment 22

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e567bf2c3342
https://hg.mozilla.org/integration/mozilla-inbound/rev/e64637e1c4b2
https://hg.mozilla.org/integration/mozilla-inbound/rev/ddd91b2e949d

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e567bf2c3342
https://hg.mozilla.org/mozilla-central/rev/e64637e1c4b2
https://hg.mozilla.org/mozilla-central/rev/ddd91b2e949d