Closed Bug 1188721 Opened 6 years ago Closed 3 years ago

consider making style structs refcounted

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED WORKSFORME
Tracking Status
firefox42 --- affected

People

(Reporter: heycam, Assigned: heycam)

References

Details

Attachments

(21 files)

58 bytes, text/x-review-board-request
dbaron
: review+
Details
58 bytes, text/x-review-board-request
dbaron
: review+
Details
58 bytes, text/x-review-board-request
dbaron
: review+
Details
58 bytes, text/x-review-board-request
dbaron
: review+
Details
58 bytes, text/x-review-board-request
dbaron
: review+
Details
58 bytes, text/x-review-board-request
dbaron
: review+
Details
58 bytes, text/x-review-board-request
dbaron
: review+
Details
58 bytes, text/x-review-board-request
dbaron
: review+
Details
58 bytes, text/x-review-board-request
dbaron
: review+
Details
58 bytes, text/x-review-board-request
dbaron
: review+
Details
58 bytes, text/x-review-board-request
dbaron
: review+
Details
58 bytes, text/x-review-board-request
dbaron
: review+
Details
58 bytes, text/x-review-board-request
dbaron
: review+
Details
58 bytes, text/x-review-board-request
dbaron
: review+
Details
58 bytes, text/x-review-board-request
dbaron
: review+
Details
58 bytes, text/x-review-board-request
dbaron
: review+
Details
58 bytes, text/x-review-board-request
dbaron
: review+
Details
58 bytes, text/x-review-board-request
dbaron
: review+
Details
58 bytes, text/x-review-board-request
dbaron
: review+
Details
58 bytes, text/x-review-board-request
dbaron
: review+
Details
58 bytes, text/x-review-board-request
dbaron
: review+
Details
I think we should consider making style structs refcounted.  Management of ownership of structs shared in the style context tree (and to a lesser degree, the rule node tree) is tricky.  If we did not have to ensure that an ancestor style context retains ownership of a struct when performing bug 931668 optimizations, we could remove the restriction on those optimizations not working when encountering shared style contexts.
Style structs are allocated from a pres arena (mostly -- there is at least one instance of a style struct being allocated on the stack: an nsStyleFont in nsRuleNode::CalcLengthWithInitialFont).  Unless we want to store pres context pointers on every style struct (and that seems a little wasteful), we won't be able to use the standard refcount machinery, as Release would need to have a pres context passed in in case the object needs deleting.
It's not as easy to allow eRestyleResult_Stop when encountering shared style contexts than I thought in comment 0.  While making style structs refcounted does let us stop doing struct swapping, we can of course still have not-yet-computed style differences between the old and new style contexts due to changes in what ancestor style contexts we have.

Refcounting style structs still does let us avoid the complicated struct swapping and faulting due to mismatched struct pointers that we do now, though.
When we want to stop restyling but are at a shared style context, we could switch to a restyling mode where we don't actually recompute descendant style structs, but instead just copy the struct pointers from the old style contexts (which would be safe now that they're refcounted).
Blocks: 1258332
Assignee: nobody → cam
Status: NEW → ASSIGNED
Comment on attachment 8733714 [details]
MozReview Request: Bug 1188721 - Part 1: Add an nsStyleStruct superclass that stores a refcount. r?dbaron

https://reviewboard.mozilla.org/r/41885/#review61738

::: layout/style/nsStyleStruct.h:108
(Diff revision 1)
>  
> -struct nsStyleFont
> +class nsStyleStruct
> +{
> +public:
> +  MozRefCountType AddRef() {
> +    if (mRefCnt == UINT32_MAX) {

This (and the same in the next function) should probably be MozRefCountType(-1), given that MozRefCountType is pointer-sized.

Alternatively, we could add a #define MOZ_REFCOUNT_MAX UINTPTR_MAX to mfbt/RefCountType.h and use that here.  That might be slightly preferable, although it's more work.

::: layout/style/nsStyleStruct.h:125
(Diff revision 1)
> +    return --mRefCnt;
> +  }
> +
> +protected:
> +  nsStyleStruct() : mRefCnt(0) {}
> +  nsStyleStruct(const nsStyleStruct& aOther) {}

Initialize mRefCnt to 0?

That said, perhaps better to make the copy constructor and the operator= be "= delete" instead of defining them.
Attachment #8733714 - Flags: review?(dbaron) → review+
Comment on attachment 8733715 [details]
MozReview Request: Bug 1188721 - Part 2: Replace void* with nsStyleStruct*. r?dbaron

https://reviewboard.mozilla.org/r/41887/#review61740

::: layout/style/nsRuleNode.h:134
(Diff revision 1)
>    mozilla::RangedArray<void*,
>                         nsStyleStructID_Reset_Start,
>                         nsStyleStructID_Reset_Count> mEntries;

Why not change this from void* to nsStyleStruct* and then avoid the casts below in the GetStyleData methods?

::: layout/style/nsRuleNode.h:163
(Diff revision 1)
> -                     nsStyleContext* aStyleContext,
> +                               nsStyleContext* aStyleContext,
> -                     bool aCanComputeData) const {
> +                               bool aCanComputeData) const {

this re-indentation looks off by one
Attachment #8733715 - Flags: review?(dbaron) → review+
(In reply to David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) from comment #28)
> ::: layout/style/nsRuleNode.h:134
> (Diff revision 1)
> >    mozilla::RangedArray<void*,
> >                         nsStyleStructID_Reset_Start,
> >                         nsStyleStructID_Reset_Count> mEntries;
> 
> Why not change this from void* to nsStyleStruct* and then avoid the casts
> below in the GetStyleData methods?

Er, never mind.  I see why not now. :-)
Attachment #8733716 - Flags: review?(dbaron) → review+
Comment on attachment 8733716 [details]
MozReview Request: Bug 1188721 - Part 3: Update refcounts of style structs stored in style contexts and rule nodes. r?dbaron

https://reviewboard.mozilla.org/r/41889/#review61742

::: layout/style/nsStyleContext.cpp
(Diff revision 1)
> -  nsStyleStruct** dataSlot;
>    if (nsCachedStyleData::IsReset(aSID)) {
>      if (!mCachedResetData) {
>        mCachedResetData = new (mRuleNode->PresContext()) nsResetStyleData;
>      }
> -    dataSlot = &mCachedResetData->mStyleStructs[aSID];
> +    mCachedResetData->SetStyleData(aSID, aStruct);
>    } else {
> -    dataSlot = &mCachedInheritedData.mStyleStructs[aSID];
> +    mCachedInheritedData.SetStyleData(aSID, aStruct);
>    }
> -  NS_ASSERTION(!*dataSlot || (mBits & nsCachedStyleData::GetBitForSID(aSID)),
> -               "Going to leak style data");
> -  *dataSlot = aStruct;

I'm a little bit sad about this, since I think this was one of the optimizations I made that *almost* got the generic codepath fast enough that we didn't need to spit out a function for each struct using macros in a whole bunch of places.  But I guess that project failed..
Attachment #8733717 - Flags: review?(dbaron) → review+
Comment on attachment 8733717 [details]
MozReview Request: Bug 1188721 - Part 4: Destroy style structs when their refcounts reach zero. r?dbaron

https://reviewboard.mozilla.org/r/41891/#review61816

::: layout/style/nsRuleNode.h:47
(Diff revision 1)
>        NS_ASSERTION(!mStyleStructs[aSID], "Going to leak style data");
>        aStyleStruct->AddRef();
>        mStyleStructs[aSID] = aStyleStruct;
>      }
>    }
> -  void ClearStyleData(nsStyleStructID aSID) {
> +  bool ClearStyleData(nsStyleStructID aSID) {

Probably want MOZ_MUST_USE on this boolean result as well?

::: layout/style/nsRuleNode.h:110
(Diff revision 1)
>        NS_ASSERTION(!mStyleStructs[aSID], "Going to leak style data");
>        aStyleStruct->AddRef();
>        mStyleStructs[aSID] = aStyleStruct;
>      }
>    }
> -  void ClearStyleData(nsStyleStructID aSID) {
> +  bool ClearStyleData(nsStyleStructID aSID) {

same here

::: layout/style/nsStyleStruct.h:123
(Diff revision 1)
> +  static void Destroy(void* aData,
> +                      nsStyleStructID aSID,
> +                      nsPresContext* aPresContext) {
> +    typedef void (*DestroyFn)(void*, nsPresContext*);
> +    static DestroyFn sDestroyFn[] = {
> +#define STYLE_STRUCT(name_, checkdata_cb_) &Destroy##name_,
> +#include "nsStyleStructList.h"
> +#undef STYLE_STRUCT
> +    };
> +    sDestroyFn[aSID](aData, aPresContext);
> +  }

Did you check what effect this (and having it inline) has on generated code size?

::: layout/style/nsStyleStruct.h:127
(Diff revision 1)
>  
> +  static void Destroy(void* aData,
> +                      nsStyleStructID aSID,
> +                      nsPresContext* aPresContext) {
> +    typedef void (*DestroyFn)(void*, nsPresContext*);
> +    static DestroyFn sDestroyFn[] = {

const, please
Comment on attachment 8733718 [details]
MozReview Request: Bug 1188721 - Part 5: Destroy structs replaced in GetUniqueStyleData. r?dbaron

https://reviewboard.mozilla.org/r/41893/#review61820

::: layout/style/nsRuleNode.h:56
(Diff revision 1)
>      return shouldDelete;
>    }
>    void SwapStyleData(nsStyleStructID aSID, nsInheritedStyleData& aOther) {
>      std::swap(mStyleStructs[aSID], aOther.mStyleStructs[aSID]);
>    }
> +  bool ReplaceStyleData(nsStyleStructID aSID, nsStyleStruct* aStyleStruct) {

MOZ_MUST_USE, here and in nsResetStyleData?

::: layout/style/nsStyleContext.cpp:467
(Diff revision 1)
> +    if (unreferenced) {                                                       \
> +      /* if currentData, which ReplaceStyleData just released, went down */   \
> +      /* to a zero refcount, we must destroy it */                            \
> +      currentData->Destroy(presContext);                                      \
> +    }                                                                         \

Seems like there should be a FIXME here, since it seems like a waste of work to do this if our goal is to get a unique struct, but we already had a unique struct.

Maybe file a followup bug (both for this FIXME and the one higher up in the function)?
Attachment #8733718 - Flags: review?(dbaron) → review+
Comment on attachment 8733719 [details]
MozReview Request: Bug 1188721 - Part 6: Remove cached struct pointer equality requirement for stopping restyles. r?dbaron

https://reviewboard.mozilla.org/r/41895/#review62462
Attachment #8733719 - Flags: review?(dbaron) → review+
Comment on attachment 8733720 [details]
MozReview Request: Bug 1188721 - Part 7: Stop swapping structs between style contexts when restyling. r?dbaron

https://reviewboard.mozilla.org/r/41897/#review62464
Attachment #8733720 - Flags: review?(dbaron) → review+
Comment on attachment 8733721 [details]
MozReview Request: Bug 1188721 - Part 8: Remove the ability to swap structs between style contexts. r?dbaron

https://reviewboard.mozilla.org/r/41899/#review62466
Attachment #8733721 - Flags: review?(dbaron) → review+
Attachment #8733722 - Flags: review?(dbaron) → review+
Comment on attachment 8733722 [details]
MozReview Request: Bug 1188721 - Part 9: Stop marking style contexts ineligible for sharing since we no longer swap structs. r?dbaron

https://reviewboard.mozilla.org/r/41901/#review62468
Comment on attachment 8733723 [details]
MozReview Request: Bug 1188721 - Part 10: Stop clearing cached struct pointers in style context descendants. r?dbaron

https://reviewboard.mozilla.org/r/41903/#review62470
Attachment #8733723 - Flags: review?(dbaron) → review+
Comment on attachment 8733724 [details]
MozReview Request: Bug 1188721 - Part 11: Stop holding strong references to parent style contexts when stopping restyles, since we no longer swap structs. r?dbaron

https://reviewboard.mozilla.org/r/41905/#review62472
Attachment #8733724 - Flags: review?(dbaron) → review+
Comment on attachment 8733725 [details]
MozReview Request: Bug 1188721 - Part 12: Report style struct refcounts in restyle logs. r?dbaron

https://reviewboard.mozilla.org/r/41907/#review62474

::: layout/style/nsStyleStruct.h:136
(Diff revision 1)
>      };
>      sDestroyFn[aSID](aData, aPresContext);
>    }
>  
> +#ifdef DEBUG
> +  MozRefCountType GetRefCnt() const { return mRefCnt; }

Maybe worth having "Debug" in the method name?
Attachment #8733725 - Flags: review?(dbaron) → review+
Attachment #8733726 - Flags: review?(dbaron) → review+
Comment on attachment 8733726 [details]
MozReview Request: Bug 1188721 - Part 13: Use style struct refcount rather than mBits to determine whether a cached struct is safe to return from GetUniqueStyleData. r?dbaron

https://reviewboard.mozilla.org/r/41909/#review62476

::: layout/style/nsStyleContext.cpp:440
(Diff revision 1)
> -  // just return it.  (We leak in this case if there are kids -- and this
> +  // just return it.
> -  // function really shouldn't be called for style contexts that could
> -  // have kids depending on the data.  ClearStyleData would be OK, but
> -  // this test for no mChild or mEmptyChild doesn't catch that case.)

I think it's worth leaving the parenthetical that this function shouldn't be called if the style struct has kids (since they'd have inherited the wrong data!)
Comment on attachment 8733727 [details]
MozReview Request: Bug 1188721 - Part 14: Stop storing style struct ownership data in nsStyleContext::mBits. r?dbaron

https://reviewboard.mozilla.org/r/41911/#review62480

::: layout/style/nsStyleContext.h
(Diff revision 1)
> -  // Since style contexts typically have some inherited data but only sometimes
> -  // have reset data, we always allocate the mCachedInheritedData, but only
> -  // sometimes allocate the mCachedResetData.

Please retain this comment.

::: layout/style/nsStyleContext.h:580
(Diff revision 1)
> -  // have reset data, we always allocate the mCachedInheritedData, but only
> -  // sometimes allocate the mCachedResetData.
>    nsResetStyleData*       mCachedResetData; // Cached reset style data.
>    nsInheritedStyleData    mCachedInheritedData; // Cached inherited style data
>  
> -  // mBits stores a number of things:
> +  // mBits stores the additional bits listed at the top of nsStyleStruct.h.

Please add a comment to that list of bits to say that all the low bits are now free.
Attachment #8733727 - Flags: review?(dbaron) → review+
Comment on attachment 8733728 [details]
MozReview Request: Bug 1188721 - Part 15: Remove unused NS_STYLE_INELIGIBLE_FOR_SHARING. r?dbaron

https://reviewboard.mozilla.org/r/41913/#review62482
Attachment #8733728 - Flags: review?(dbaron) → review+
Comment on attachment 8733729 [details]
MozReview Request: Bug 1188721 - Part 16: Remove unused NS_STYLE_IS_GOING_AWAY. r?dbaron

https://reviewboard.mozilla.org/r/41915/#review62484
Attachment #8733729 - Flags: review?(dbaron) → review+
Comment on attachment 8733730 [details]
MozReview Request: Bug 1188721 - Part 17: Remove unused nsStyleContext::ClearCachedInheritedStyleData. r?dbaron

https://reviewboard.mozilla.org/r/41917/#review62486
Attachment #8733730 - Flags: review?(dbaron) → review+
Comment on attachment 8733731 [details]
MozReview Request: Bug 1188721 - Part 18: Remove unused ns{Inherited,Reset}StyleData::ClearStyleData. r?dbaron

https://reviewboard.mozilla.org/r/41919/#review62488
Attachment #8733731 - Flags: review?(dbaron) → review+
Comment on attachment 8733732 [details]
MozReview Request: Bug 1188721 - Part 19: Remove unused nsStyleContext::HasCachedDependentStyleData. r?dbaron

https://reviewboard.mozilla.org/r/41921/#review62490
Attachment #8733732 - Flags: review?(dbaron) → review+
Comment on attachment 8733733 [details]
MozReview Request: Bug 1188721 - Part 20: Remove unused nsStyleContext::HasSingleReference. r?dbaron

https://reviewboard.mozilla.org/r/41923/#review62492
Attachment #8733733 - Flags: review?(dbaron) → review+
https://reviewboard.mozilla.org/r/41909/#review62494

::: layout/style/nsStyleStruct.h:135
(Diff revision 1)
>  #undef STYLE_STRUCT
>      };
>      sDestroyFn[aSID](aData, aPresContext);
>    }
>  
> +  bool HasSingleReference() const { return mRefCnt == 1; }

Probably good to assert here that mRefCnt is not 0 (like the assertion removed in part 20).
Comment on attachment 8733734 [details]
MozReview Request: Bug 1188721 - Part 21: Remove style struct ownership assertions. r?dbaron

https://reviewboard.mozilla.org/r/41925/#review62496
Attachment #8733734 - Flags: review?(dbaron) → review+
We don't need this now that we use Arcs to hold style structs in ComputedValuesInner.
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → WONTFIX
I think WFM is a bit more accurate for posterity, since they they're refcounted now.
Resolution: WONTFIX → WORKSFORME
You need to log in before you can comment on or make changes to this bug.