If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Always allocate space for inherit structs in style contexts

RESOLVED FIXED

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
8 years ago
2 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

Trunk
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 2 obsolete attachments)

2.89 KB, patch
zwol
: review+
Details | Diff | Splinter Review
1.32 KB, patch
zwol
: review+
Details | Diff | Splinter Review
15.69 KB, patch
zwol
: review+
dbaron
: superreview+
Details | Diff | Splinter Review
5.06 KB, patch
zwol
: review+
dbaron
: superreview+
Details | Diff | Splinter Review
7.37 KB, patch
zwol
: review+
dbaron
: superreview+
Details | Diff | Splinter Review
6.11 KB, patch
Details | Diff | Splinter Review
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.
Created attachment 411734 [details] [diff] [review]
Part 1: remove some dead code
Attachment #411734 - Flags: review?(zweinberg)
Created attachment 411735 [details] [diff] [review]
Part 2.  Make nsInheritedStyleData destruction more flexible
Attachment #411735 - Flags: review?(zweinberg)
Created attachment 411737 [details] [diff] [review]
Real part 2
Attachment #411735 - Attachment is obsolete: true
Attachment #411737 - Flags: review?(zweinberg)
Attachment #411735 - Flags: review?(zweinberg)
Created attachment 411742 [details] [diff] [review]
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.
Attachment #411742 - Flags: superreview?(dbaron)
Attachment #411742 - Flags: review?(zweinberg)
Created attachment 411820 [details] [diff] [review]
Part 4: always cache inherit structs on the style context, even when caching on the rulenode

No reason to not do this, imo.
Attachment #411820 - Flags: superreview?(dbaron)
Attachment #411820 - Flags: review?(zweinberg)
Created attachment 411821 [details] [diff] [review]
Part 5: make PeekStyleData on inherit structs very fast
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #411821 - Flags: superreview?(dbaron)
Attachment #411821 - Flags: review?(zweinberg)
Created attachment 415411 [details] [diff] [review]
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....
Attachment #415411 - Flags: superreview?(dbaron)
Attachment #415411 - Flags: review?(zweinberg)
Created attachment 416243 [details] [diff] [review]
This is what I used to measure, by the way
Attachment #411821 - Attachment is obsolete: true
Attachment #411821 - Flags: superreview?(dbaron)
Attachment #411821 - Flags: review?(zweinberg)

Updated

8 years ago
Attachment #411734 - Flags: review?(zweinberg) → review+

Updated

8 years ago
Attachment #411737 - Flags: review?(zweinberg) → review+
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 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.
Attachment #411742 - Flags: review?(zweinberg) → review+
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?
Attachment #411820 - Flags: review?(zweinberg) → review+
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?
> 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.
> 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....
(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 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.
Attachment #415411 - Flags: review?(zweinberg) → review+
> Maybe make it a protected method, if it isn't already

It already is.
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.
Attachment #411742 - Flags: superreview?(dbaron) → superreview+
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
Attachment #411820 - Flags: superreview?(dbaron) → superreview+
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)
Attachment #415411 - Flags: superreview?(dbaron) → superreview+
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
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).
You need to log in before you can comment on or make changes to this bug.