527977 - Always allocate space for inherit structs in style contexts

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Boris Zbarsky [:bzbarsky]]

I did some measurement of what sorts of cached style data we allocate for style contexts and rulenodes (and the fallback data we allocate in style sets, of course, but that always has both reset and inherited structs).  Results over our Tp4 run:

Rulenodes:
9846 HAVE BOTH
7700 HAVE INHERITED ONLY
193469 HAVE RESET ONLY
127117 HAVE NEITHER

Style contexts:
151088 HAVE BOTH
550099 HAVE INHERITED ONLY

Which makes sense when you think about it: If a style context inherits _anything_ (that is, if there's any inherit struct someone cares about that's not fully specified with non-parent-dependent values), then the code in the

  if ((!startStruct && !isReset &&
       (detail == eRuleNone || detail == eRulePartialInherited)) ||
      detail == eRuleFullInherited) {

block in nsRuleNode::WalkRuleTree will grab that struct from the parent style context and set it on the child.  So you have to try _really_ hard to create a set of rules that can lead to a style context that doesn't have any inherit structs cached on it.

Given that, I think we should just have an nsInheritedStyleData member in nsStyleContext (not a pointer) and an nsResetStyleData* member for those cases when we have reset structs.  Should save us one more allocation and some branches, etc.

Patches coming up.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 1: remove some dead code

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 2. Make nsInheritedStyleData destruction more flexible

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Real part 2

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 3: main guts. Change the storage, inline GetStyle*/PeekStyle*

This seems to give me about a 10% win on a benchmark that mostly measures a situation where we reparent style contexts and then compute the difference...  No obvious changes on benchmarks that measure frame construction and the like.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 4: always cache inherit structs on the style context, even when caching on the rulenode

No reason to not do this, imo.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 5: make PeekStyleData on inherit structs very fast

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 5 actually passing tests (not sure we want this)

There turned out to be cases that didn't cache inherited structs on the style context.  This patch fixes that, but I'm not entirely sure it's worth it....

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: This is what I used to measure, by the way

Comment 9

[Zack Weinberg (:zwol)]

Comment on attachment 411737 [details] [diff] [review]
Real part 2

Please put a blank line between the last } of DestroyStructs and the new beginning of Destroy().

Comment 10

[Zack Weinberg (:zwol)]

Comment on attachment 411742 [details] [diff] [review]
Part 3: main guts.  Change the storage, inline GetStyle*/PeekStyle*


I really wish we didn't need all this mucking about with offsets and reinterpret_cast<>, but that's neither here nor there as far as this patch.  The overall concept looks good; some nits:

> const void* nsStyleContext::GetStyleData(nsStyleStructID aSID)
> {
>-  const void* cachedData = mCachedStyleData.GetStyleData(aSID); 
>+  const void* cachedData = GetCachedStyleData(aSID);;

Double semicolon at the end of the new line.

>+  if (nsCachedStyleData::IsReset(aSID)) {
>+    if (!mCachedResetData) {
>+      mCachedResetData = new (mRuleNode->GetPresContext()) nsResetStyleData;
>+      // XXXbz And if that fails?
>     }

Probably not worth worrying about?  I don't know where we are on infallible malloc, though.

>+  /**
>+   * PeekStyle* is like GetStyle* but doesn't have the rulenode
>+   * compute the data.
>+   *
>+   * Strictly speaking, we don't need to do this for structs no one's
>+   * called GetStyle* or GetStyleData on for this particular style
>+   * context.  But we don't keep track of that right now.
>+   */

The first paragraph of this comment I find a little misleading, as the rulenode GetStyleXXX function _is_ called, just with aComputeData false.  Maybe "... but does not trigger style computation if the data is not cached on either the style context or the rule node"?

The second paragraph of this comment is opaque to me.  It might not need changing but I'd appreciate an elaboration.

>+  // Helper function that GetStyleData and GetUniqueStyleData use.  Only
>+  // returns the structs we cache ourselves; never consults the ruletree.
>+  inline const void* GetCachedStyleData(nsStyleStructID aSID);

Function body isn't available, so this shouldn't be inline.

>+  // Helper functions for GetStyle* and PeekStyle*
>+  #define STYLE_STRUCT_INHERITED(name_, checkdata_cb_, ctor_args_)      \
>+    const nsStyle##name_ * DoGetStyle##name_(PRBool aComputeData) {     \

I'm not a big fan of the name "DoGetStyle", but I can't think of anything better.

>+  // 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.
>+  nsResetStyleData*       mCachedResetData; // Cached reset style data.
>+  nsInheritedStyleData    mCachedInheritedData; // Cached inherited style data

I wonder, do we know at the time a style context is created whether it should cache reset data?  If so, as a follow-up we could turn that case into a derived type that embeds both structures, saving an allocation and some pointer chasing.

Comment 11

[Zack Weinberg (:zwol)]

Comment on attachment 411820 [details] [diff] [review]
Part 4: always cache inherit structs on the style context, even when caching on the rulenode

The code looks fine, but for the record, could you explain the rationale for this change in a little more detail?

Comment 12

[Zack Weinberg (:zwol)]

Comment on attachment 415411 [details] [diff] [review]
Part 5 actually passing tests (not sure we want this)

I'm confused by this one.  Part 4 makes rule nodes unconditionally cache inherited data on their style contexts as they compute them.  Why then is it necessary to re-cache stuff in nsStyleContext::GetStyleData?

Comment 13

[Boris Zbarsky [:bzbarsky]]

> Please put a blank line between the last } of DestroyStructs and the new
> beginning of Destroy().

Done.

> Double semicolon at the end of the new line.

Fixed.

> Probably not worth worrying about?  

None of the rest of this code does; I think it's ok.

> Maybe "... but does not trigger style computation if the data is not cached
> on either the style context or the rule node"?

Done.

> The second paragraph of this comment is opaque to me. 

I've removed it altogether and replaced it with:

   * Perhaps this shouldn't be a public nsStyleContext API.

The comment I had there makes sense for the nsStyleContext-internal uses of PeekStyle* (though yes, it was far too cryptic), but doesn't make sense for external callers.

> Function body isn't available, so this shouldn't be inline.

Hmm.  It's available at all the callsites, right?  Is that not enough?

> I wonder, do we know at the time a style context is created whether it should
> cache reset data?  

Unfortunately, no.  It needs to do that if any of its reset structs has any property whose computed value depends on the parent... and since we compute structs lazily, we don't have this information at stylecontext-creation time.

Comment 14

[Boris Zbarsky [:bzbarsky]]

> for the record, could you explain the rationale for this change in a little
> more detail?

Since there is now no memory cost to caching on the style context, it seems like we might as well save some cycles and not have to go to the ruletree for those inherit structs (particularly because in the ruletree we might need to walk several rulenodes to get to the right struct).

> Why then is it necessary to re-cache stuff in nsStyleContext::GetStyleData?

Unfortunately, the code in part 4 only caches the struct on the context if the rulenode ends up computing it.  If the struct is instead found on some ancestor rulenode in WalkRuleTree, it doesn't get cached on the style context.  I considered changing WalkRuleTree to do the caching itself, but that turned out to be more complicated than changing GetStyleData....

Comment 15

[Zack Weinberg (:zwol)]

(In reply to comment #13)
> > The second paragraph of this comment is opaque to me. 
> I've removed it altogether and replaced it with:
>
>    * Perhaps this shouldn't be a public nsStyleContext API.
> 
> The comment I had there makes sense for the nsStyleContext-internal uses of
> PeekStyle* (though yes, it was far too cryptic), but doesn't make sense for
> external callers.

Ok.

> > Function body isn't available, so this shouldn't be inline.
> 
> Hmm.  It's available at all the callsites, right?  Is that not enough?

Yeah, it is.  Maybe make it a protected method, if it isn't already, to hint to readers that the definition is visible to all callers?

> > I wonder, do we know at the time a style context is created whether it should
> > cache reset data?  
> 
> Unfortunately, no.  It needs to do that if any of its reset structs has any
> property whose computed value depends on the parent... and since we compute
> structs lazily, we don't have this information at stylecontext-creation time.

Oh well.

(In reply to comment #14)
> > for the record, could you explain the rationale for this change in a little
> > more detail?
> 
> Since there is now no memory cost to caching on the style context, it seems
> like we might as well save some cycles and not have to go to the ruletree for
> those inherit structs (particularly because in the ruletree we might need to
> walk several rulenodes to get to the right struct).

Thanks, that makes sense.

Comment 16

[Zack Weinberg (:zwol)]

Comment on attachment 415411 [details] [diff] [review]
Part 5 actually passing tests (not sure we want this)

(In reply to comment #14)
> > Why then is it necessary to re-cache stuff in nsStyleContext::GetStyleData?
> 
> Unfortunately, the code in part 4 only caches the struct on the context if the
> rulenode ends up computing it.  If the struct is instead found on some ancestor
> rulenode in WalkRuleTree, it doesn't get cached on the style context.  I
> considered changing WalkRuleTree to do the caching itself, but that turned out
> to be more complicated than changing GetStyleData....

Got it.  I'll approve then.

Comment 17

[Boris Zbarsky [:bzbarsky]]

> Maybe make it a protected method, if it isn't already

It already is.

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 411742 [details] [diff] [review]
Part 3: main guts.  Change the storage, inline GetStyle*/PeekStyle*

>+  const void* cachedData = GetCachedStyleData(aSID);;

want only one ; rather than two

>    * The typesafe functions below are preferred to the use of this
>-   * function.
>+   * function, if nothing else because they're faster.

Both because they're easier to read and because they're faster.

sr=dbaron

Sorry for the delay.

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 411820 [details] [diff] [review]
Part 4: always cache inherit structs on the style context, even when caching on the rulenode

You need to update the comment:

>  // context owns a reference to its parent).  If the bit in |mBits| is
>  // set for a struct, that means that the pointer for that struct is
>  // owned by an ancestor rather than by this style context.

and:

>  PRUint32                mBits; // Which structs are inherited from the
>                                 // parent context.

to reflect that mBits can be set for cached-on-the-rule-node as well.

sr=dbaron with that

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 415411 [details] [diff] [review]
Part 5 actually passing tests (not sure we want this)

sr=dbaron, but probably worth landing separately, and backing out if it doesn't provide any additional benefit (since it is added complexity)

Comment 21

[Boris Zbarsky [:bzbarsky]]

Checked in those first 4 parts:
  http://hg.mozilla.org/mozilla-central/rev/bc964be10f48
  http://hg.mozilla.org/mozilla-central/rev/c8777f7a4b3d
  http://hg.mozilla.org/mozilla-central/rev/ac2b160e89d5
  http://hg.mozilla.org/mozilla-central/rev/49ffeef99f25

and a fix for resulting build bustage:

  http://hg.mozilla.org/mozilla-central/rev/124702f2b6bc

Doesn't seem to move the talos needle much, if at all.

I doubt part 5 would be noticeable on talos, or really anywhere that doesn't involve a bunch of restyling.  It sounds like it might not be worth it in general, perhaps.  So not going to land that; marking fixed for the rest.

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So I'd always thought this patch was intended to make us obey the invariant that if you'd asked for an inherited struct on a style context, it would be cached on the context.  But it actually only did that in the case where the request to the rule node resulted in a new struct; it didn't do so if the struct needed was already cached in the rule tree (common for nsStyleColor and nsStyleQuotes and *maybe* nsStyleFont and nsStyleList, probably very rare otherwise since it requires specifying all properties in the struct).

I'm fixing this in bug 1209603 patch 9 (at least assuming I don't renumber the patches again).