804975 - store more style data in the rule tree by allowing multiple reset structs on a rule node, based on conditions

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

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

I was thinking about this as a followup to bug 780341 comment 38 and bug 780341 comment 40.

My initial idea was that we could actually store some of conditions that
currently prevent styles from being stored in the rule tree as nodes in
the rule tree (that has a condition instead of a rule).  I'd start with
element's font size and root's font size as the first conditions.

Then I started to think it would be preferable to, instead of having
additional rule nodes, to just have a mechanism for storing multiple
copies of the same struct on a particular rule node.

This also has the nice characteristic that it improves a performance
penalty that we currently have for good design practices ('em' units).

List of reset structs:
  Background
  Position
  TextReset
  Display
  Content
  UIReset
  Table
  Margin
  Padding
  Border
  Outline
  XUL
  SVGReset
  Column

Potential conditions (these all depend only on inherited structs, I
think!), though we wouldn't necessarily need to do this for all of these
conditions:
 - 'em' size
 - 'ex' size
 - 'rem' size
 - 'ch' size
 - 'color'
 - body's 'color'
 - 'direction'
 - min font size

I mentioned this to bzbarsky, and he wrote in email:
> If we can do it without making rulenodes too big and slowing down
> access to structs on them too much, this second would be good, yes.
> Right now, the walk up the rule tree looking for reset structs is
> pretty hot in many cases.


I'm thinking this may well be worth trying.

Comment 1

[Kevin Grandon :kgrandon]

Would like to have, but I suppose this is not a requirement for 2.0.

Comment 2

[Florian Bender]

(In reply to Benoit Girard (:BenWa) from Bug 862276 comment #13)
> If bug 862274 is a dupe of bug 931668 then there should only be bug 804975
> left. We should try to wrap it up ASAP since this bug is causing significant
> delays all over FFOS.

Is anybody currently looking into this issue?

Comment 3

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

It's not clear how much this bug would help; the first step would be to get some numbers on whether it would actually be a significant win (in speed and space).

I don't think anyone is looking into it right now, though sounds like heycam might at some point.

Comment 4

[Cameron McCormack (:heycam)]

David suggested to me to measure how often we fault out of storing a struct in the rule tree due to a single reason -- using em units is probably going to be one of the more common ones, so would be a good candidate -- and then to count how many times we end up recomputing that struct due to it not being cached on the rule tree.

Comment 5

[Cameron McCormack (:heycam)]

Attachment: measurements

I did some measurements and looked at four pages: cnn.com, facebook.com (after logging in), twitter.com (also after logging in), and html.spec.whatwg.org.

The measurements show:
  (a) the counts of unique numbers of font-size values being relied upon to
      compute em values (overall, and per struct) for a given rule node, which
      gives us an idea of how many copies of a struct we would need to store in
      a rule node; and

  (b) per struct, how many times we recompute a struct due to faulting out of
      the rule tree because of em units being used and no other reason, how
      many times we recompute a struct due to faulting out of the rule tree for
      any reason, and how many times we compute a struct in total.

In terms of how many unique font-size values are needed, overwhelmingly we only need one struct.  This is not exceptionally surprising; only relative font-size values (em, ex, %, smaller, larger) could cause different font-size values to be used for a given rule node.

In terms of how many struct computations we would save if we would otherwise fault out of the rule tree caching due to em unit usage, it isn't much of a help on cnn.com, twitter.com and facebook.com.  It is however a big help on the HTML spec, and that I guess is due to many elements matching rules with em-united margins and padding.  For example, of the 13315 Margin struct computations that occur, 10322 of them are due to missing the rule tree cache, and 7189 of those are due solely to em unit usage.

It would be interesting to record the counts of other reasons for computing a struct due to not caching in the rule tree.  Other conditions (or combinations of conditions) might be more profitable.

Comment 6

[Cameron McCormack (:heycam)]

In terms of memory savings on the html.space.whatwg.org page, it would be 4,278,712 bytes out of 50,890,588 bytes of style data if you only count the structs themselves and not whatever they might point to (and ignoring any memory needed to store multiple structs on a rule node of course).

Comment 7

[Cameron McCormack (:heycam)]

Attachment: more detailed measurements

Here are measurements for html.spec.whatwg.org that show more detail about the reasons for missing the rule tree cache.  I've just restricted it to reset structs.

A bunch of the remaining Margin/Padding cases are due to the use of logical properties.  Given the use of logical properties in the UA style sheet, I think we should caching structs for different writing mode values.

Most of the rest are due to explicit inheritance of properties.  I'm counting just the number of inherited properties on a given struct, not specifically which properties they are (as it would have been a lot harder to thread through the property ID through the various {Calc,Set}Blah functions in nsRuleNode).

Comment 8

[Cameron McCormack (:heycam)]

Attachment: measurement patch

Comment 9

[Cameron McCormack (:heycam)]

Attachment: script to produce measurement output summary

This script takes as input all of Firefox's stdout with the attachment 8577890 [details] [diff] [review] applied and outputs a summary like attachment 8577888 [details].

Comment 10

[Cameron McCormack (:heycam)]

Attachment: measurements from a day's browser usage

Here are measurements from about 8 hours of my normal browser usage.  Still a lot of cache misses due to explicit inherit -- an awful lot on TextReset and Display, in particular.  Apart from that, em unit values and use of logical properties still stand out as the most common reasons.

Comment 11

[Cameron McCormack (:heycam)]

Attachment: PoC for caching structs with different font-size and writing-mode values

Comment 12

[Cameron McCormack (:heycam)]

I did a try both with and without attachment 8585322 [details] [diff] [review] applied:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=96058c6fe928
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b4590c3a7b18

Unfortunately it doesn't look like we get a performance benefit from this on our talos tests.

I also did some manual testing that loads the HTML spec and measure time from the start of the document loading until the load event is dispatched (and an extra layout flush done for good measure), but got similar looking values.

We do get some memory savings, though.  Here is about:memory for the HTML spec without the patch applied:

│  │  │  ├──216.99 MB (56.63%) -- window(file:///tmp/HTML%20Standard.html)
│  │  │  │  ├──107.45 MB (28.04%) -- layout
│  │  │  │  │  ├───41.60 MB (10.86%) -- frames
│  │  │  │  │  │   ├──16.37 MB (04.27%) ── nsTextFrame
│  │  │  │  │  │   ├──10.52 MB (02.75%) ── nsInlineFrame
│  │  │  │  │  │   ├───7.67 MB (02.00%) ── nsBlockFrame
│  │  │  │  │  │   └───7.04 MB (01.84%) ++ (16 tiny)
│  │  │  │  │  ├───29.38 MB (07.67%) ── text-runs
│  │  │  │  │  ├───20.58 MB (05.37%) ── pres-contexts
│  │  │  │  │  ├────6.91 MB (01.80%) ── line-boxes
│  │  │  │  │  ├────4.92 MB (01.28%) ── pres-shell
│  │  │  │  │  ├────3.83 MB (01.00%) ── style-contexts
│  │  │  │  │  └────0.24 MB (00.06%) ++ (2 tiny)
│  │  │  │  ├───99.85 MB (26.06%) -- dom
│  │  │  │  │   ├──61.21 MB (15.98%) ── element-nodes
│  │  │  │  │   ├──26.28 MB (06.86%) ── text-nodes
│  │  │  │  │   ├──12.36 MB (03.22%) ── other [2]
│  │  │  │  │   └───0.00 MB (00.00%) ── event-targets
│  │  │  │  ├────6.00 MB (01.57%) ── property-tables
│  │  │  │  └────3.69 MB (00.96%) ++ (2 tiny)

and with the patch applied:

│  │  │  ├──187.43 MB (50.69%) -- window(file:///tmp/HTML%20Standard.html)
│  │  │  │  ├───99.85 MB (27.00%) -- dom
│  │  │  │  │   ├──61.21 MB (16.55%) ── element-nodes
│  │  │  │  │   ├──26.28 MB (07.11%) ── text-nodes
│  │  │  │  │   ├──12.36 MB (03.34%) ── other [2]
│  │  │  │  │   └───0.00 MB (00.00%) ── event-targets
│  │  │  │  ├───77.88 MB (21.06%) -- layout
│  │  │  │  │   ├──41.60 MB (11.25%) -- frames
│  │  │  │  │   │  ├──16.37 MB (04.43%) ── nsTextFrame
│  │  │  │  │   │  ├──10.52 MB (02.85%) ── nsInlineFrame
│  │  │  │  │   │  ├───7.67 MB (02.07%) ── nsBlockFrame
│  │  │  │  │   │  └───7.04 MB (01.90%) ++ (16 tiny)
│  │  │  │  │   ├──20.58 MB (05.57%) ── pres-contexts
│  │  │  │  │   ├───6.91 MB (01.87%) ── line-boxes
│  │  │  │  │   ├───4.72 MB (01.28%) ── pres-shell
│  │  │  │  │   ├───3.83 MB (01.04%) ── style-contexts
│  │  │  │  │   └───0.24 MB (00.07%) ++ (3 tiny)

So we're saving around 30 MB of memory.

Comment 13

[Cameron McCormack (:heycam)]

I have a patch queue that works, but I'm not seeing the kinds of memory improvements that I mention in comment 12.  I assumed that style structs weren't being counted anywhere specific, but now I see that they are attributed to the pres-shell entry.  I'm not sure where the 30 MB different was coming from.

Instrumenting to measure the number of Margin/Padding/Position structs that exist after the HTML spec is loaded (these are the structs with the best usage of the conditional rule node caching), I get:

* With patches:
    Margin:    320 ( 20,480 bytes)
    Padding:   286 ( 18,304 bytes)
    Position:  531 (237,888 bytes)
* Without patches:
    Margin:   2864 (183,296 bytes)
    Padding:  2143 (137,152 bytes)
    Position: 1830 (819,840 bytes)

So in total that's 843 KB.  Not an amazing amount compared to the ~200 MB for the page as a whole.

I also adjusted the memory reporters to get an explicit style-structs entry, and get a similar value in difference (where the total style struct memory usage was in the order of a few megabytes).

A talos try run on my current patches still reveals no performance improvements on those tests:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=da3da51db4ef (without patches)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=63a7186f005a (with patches)

So overall I'm not sure whether we should take the patches.  I'll attach what I have.  WDYT David?

Comment 14

[Cameron McCormack (:heycam)]

about:memory without patches applied:

│  ├──195.07 MB (54.01%) -- top(file:///tmp/html.html, id=8)/active
│  │  ├──193.80 MB (53.66%) -- window(file:///tmp/html.html)
│  │  │  ├──104.27 MB (28.87%) -- dom
│  │  │  │  ├───62.84 MB (17.40%) ── element-nodes
│  │  │  │  ├───29.07 MB (08.05%) ── text-nodes
│  │  │  │  ├───12.36 MB (03.42%) ── other [2]
│  │  │  │  └────0.00 MB (00.00%) ── event-targets
│  │  │  ├───79.81 MB (22.10%) -- layout
│  │  │  │   ├──41.81 MB (11.58%) -- frames
│  │  │  │   │  ├──16.38 MB (04.54%) ── nsTextFrame
│  │  │  │   │  ├──10.53 MB (02.91%) ── nsInlineFrame
│  │  │  │   │  ├───7.68 MB (02.13%) ── nsBlockFrame
│  │  │  │   │  └───7.21 MB (02.00%) ++ (16 tiny)
│  │  │  │   ├──20.64 MB (05.71%) ── pres-contexts
│  │  │  │   ├───7.66 MB (02.12%) ── line-boxes
│  │  │  │   ├───5.61 MB (01.55%) -- (5 tiny)
│  │  │  │   │   ├──3.07 MB (00.85%) ── style-structs
│  │  │  │   │   ├──2.23 MB (00.62%) ── pres-shell
│  │  │  │   │   ├──0.17 MB (00.05%) ── style-sets
│  │  │  │   │   ├──0.14 MB (00.04%) ── rule-nodes
│  │  │  │   │   └──0.01 MB (00.00%) ── text-runs
│  │  │  │   └───4.10 MB (01.13%) ── style-contexts
│  │  │  ├────6.00 MB (01.66%) ── property-tables
│  │  │  └────3.72 MB (01.03%) ++ (2 tiny)
│  │  └────1.27 MB (00.35%) ++ (4 tiny)

about:memory with patches applied:

│  ├──194.20 MB (52.34%) -- top(file:///tmp/html.html, id=8)
│  │  ├──193.97 MB (52.28%) -- active
│  │  │  ├──192.70 MB (51.94%) -- window(file:///tmp/html.html)
│  │  │  │  ├──104.27 MB (28.10%) -- dom
│  │  │  │  │  ├───62.84 MB (16.94%) ── element-nodes
│  │  │  │  │  ├───29.07 MB (07.84%) ── text-nodes
│  │  │  │  │  ├───12.36 MB (03.33%) ── other [2]
│  │  │  │  │  └────0.00 MB (00.00%) ── event-targets
│  │  │  │  ├───78.70 MB (21.21%) -- layout
│  │  │  │  │   ├──41.81 MB (11.27%) -- frames
│  │  │  │  │   │  ├──16.38 MB (04.42%) ── nsTextFrame
│  │  │  │  │   │  ├──10.53 MB (02.84%) ── nsInlineFrame
│  │  │  │  │   │  ├───7.68 MB (02.07%) ── nsBlockFrame
│  │  │  │  │   │  └───7.22 MB (01.94%) ++ (16 tiny)
│  │  │  │  │   ├──20.59 MB (05.55%) ── pres-contexts
│  │  │  │  │   ├───7.66 MB (02.07%) ── line-boxes
│  │  │  │  │   ├───4.53 MB (01.22%) -- (5 tiny)
│  │  │  │  │   │   ├──2.19 MB (00.59%) ── style-structs
│  │  │  │  │   │   ├──2.04 MB (00.55%) ── pres-shell
│  │  │  │  │   │   ├──0.17 MB (00.04%) ── style-sets
│  │  │  │  │   │   ├──0.14 MB (00.04%) ── rule-nodes
│  │  │  │  │   │   └──0.00 MB (00.00%) ── text-runs
│  │  │  │  │   └───4.11 MB (01.11%) ── style-contexts
│  │  │  │  ├────6.00 MB (01.62%) ── property-tables
│  │  │  │  └────3.72 MB (01.00%) ++ (2 tiny)
│  │  │  └────1.27 MB (00.34%) ++ (4 tiny)
│  │  └────0.23 MB (00.06%) ++ js-zone(0x7fffc2fe5800)

Comment 15

[Cameron McCormack (:heycam)]

Attachment: Part 2: Add a RuleNodeCacheKey class and use it instead of a boolean canStoreInRuleTree during style computation.

Comment 16

[Cameron McCormack (:heycam)]

Attachment: Part 3: Support conditional cached reset structs on rule nodes.

Comment 17

[Cameron McCormack (:heycam)]

Attachment: Part 4: Cache reset structs on rule nodes for different font-size or writing mode values.

Comment 18

[Cameron McCormack (:heycam)]

Filed bug 1168299 for getting about:memory to report style struct memory usage.

Comment 19

[Cameron McCormack (:heycam)]

Attachment: Part 1: Expose WritingMode bits.

Comment 20

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

Is there a limit to the length of the chains for the conditions?  It seems like there ought to be -- and I wonder if that would change the performance characteristics.  I didn't see one so far, though.

Otherwise I tend to think this is pretty reasonable.

Comment 21

[Cameron McCormack (:heycam)]

No, I don't have a limit currently.  I'll add one then request review.

Comment 22

[Cameron McCormack (:heycam)]

Attachment: Part 2: Add a RuleNodeCacheKey class and use it instead of a boolean canStoreInRuleTree during style computation.

Comment 23

[Cameron McCormack (:heycam)]

Attachment: Part 3: Support conditional cached reset structs on rule nodes.

Comment 24

[Cameron McCormack (:heycam)]

Attachment: Part 3.1: Limit the number of conditional style structs we cache on rule nodes.

Comment 25

[Cameron McCormack (:heycam)]

Attachment: Part 4: Cache reset structs on rule nodes for different font-size or writing mode values.

Comment 26

[Cameron McCormack (:heycam)]

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

Comment 27

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

What do new performance numbers look like?  (Both for something general, and maybe also for something that you'd expect to be faster just to confirm that it actually is?)

Comment 28

[Cameron McCormack (:heycam)]

FYI, in my testing last quarter I found that it was rare to need to store more than one struct in the rule node, so I don't think we would generally get a performance improvement from limiting the number of structs.

(In reply to David Baron [:dbaron] ⏰UTC-7 from comment #27)
> What do new performance numbers look like?  (Both for something general, and
> maybe also for something that you'd expect to be faster just to confirm that
> it actually is?)

How about I measure total time spent under nsRuleNode::WalkRuleTree for a selection of pages, with and without the patches applied?

Comment 29

[Cameron McCormack (:heycam)]

Opening the browser, loading the full HTML spec (with scripts stripped) from a file, then quitting the browser, averaging over 5 runs, I get around 308ms of time spent under nsRuleNode::WalkRuleTree just for that document without the patches applied (σ=18ms), and around 252ms with the patches applied (σ=9ms).

Comment 30

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

Comment on attachment 8615761 [details] [diff] [review]
Part 2: Add a RuleNodeCacheKey class and use it instead of a boolean canStoreInRuleTree during style computation.

I'm not sure about the use of "CacheKey" as a term here.  We've usually
used that for something used to look something up in a cache or check
whether a cached result is still valid.  This seems a little broader than
that (although I suppose I've already taken CacheKey a little bit out
of context -- I think this takes it farther).  Might
RuleNodeCacheability be better?  Any other ideas?  (Search-replacing
the patch should be doable.)


The definition of mFontSize in RuleNodeCacheKey.h should say that it's
the font size from which em units are derived.  (Otherwise there's
ambiguity in the case of text zoom or minimum font size.)  If you want
to work it out, it might even be worth saying what that means in the case
of text zoom and minimum font size (though it might not be a bad idea for
me to review that).

In RuleNodeCacheKey.cpp:

Perhaps RuleNodeCacheKey::Matches should assert that the eUncacheable
bit is not set?  (Or, otherwise, maybe it should return false if it is?)


nsRuleNode.cpp:

>-      // aCanStoreInRuleTree to false yet, so we don't want to introduce
>+      // aCacheKey to false yet, so we don't want to introduce

s/to false/have dependencies or be uncacheable/, or something like that

r=dbaron with that

Comment 31

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

Comment on attachment 8615762 [details] [diff] [review]
Part 3: Support conditional cached reset structs on rule nodes.

In nsConditionalResetStyleData::Entry, it might make sense to make
the mCacheKey and mStyleStruct members const.  (It looks like they
never change -- and it makes sense for them never to change.)

I'm a little concerned about the presence of the GetStyleData function
on nsCachedStyleData and nsConditionalResetStyleData that doesn't take a
style context.  Same for the GetStyle##name_ on nsCachedStyleData.  Is
that temporary?  What still calls it at the end of the patch series?
(I see the assertion in COMPUTE_END_RESET... though that use could be
replaced by a HasStyleData call, i.e., you could add a HasStyleData
that just returns boolean.)

At some point we might want to integrate marking of these structs into
the rule tree GC.

COMPUTE_END_RESET:
>+  } else if (cacheKey.Cacheable()) {                                          \
>+    if (!mStyleData.mResetData) {                                             \
>+      mStyleData.mResetData = new (mPresContext) nsConditionalResetStyleData; \
>+    }                                                                         \
>+    mStyleData.mResetData->                                                   \
>+      SetStyleData(eStyleStruct_##type_, mPresContext, data_, cacheKey);      \
>+    /* Tell the style context that it doesn't own the data */                 \
>+    aContext->                                                                \
>+      AddStyleBit(nsCachedStyleData::GetBitForSID(eStyleStruct_##type_));     \
>+    aContext->SetStyle(eStyleStruct_##type_, data_);                          \

Here you're missing out on quite a bit of caching opportunity.  You're
using mStyleData rather than aHighestNode->mStyleData, which means you're
storing the data at the most specific point in the rule tree rather than
at the least specific point in the rule tree that's possible.

I think that's worth doing.  It will make the optimization here
substantially stronger.

However, you'll need to decide what it means about the dependent bits,
and the invariants related to IsUsedDirectly (which are maintained by
both PropagateDependentBit and SetUsedDirectly).  You clearly can't use
PropagateDependentBit as it is today.  So you'll need to come up with
new invariants and adjust WalkRuleTree as appropriate.

I agree that also storing the result on the style context is the right
thing to do here.


And to make that cross-rule-node optimization worthwhile, you'll also
need to adjust WalkRuleTree so that it finds the cached data on an
ancestor rule node.  That might mean that you have to set dependent
bits, which will mean a slight change in their invariants (since for
these structs you probably don't want them on all IsUsedDirectly() rule
nodes, but you probably do want them cached on the style context).

I think that adjustment might be easier to do if you don't limit the
number of conditional structs you store -- although maybe it's not a big
deal.  If so, I'm ok with dropping patch 3.1.  (Sorry, should have
realized that before I told you to limit it.)  But if you do drop patch
3.1, it might make sense to make the getter for the conditional structs
pull the result it got to the front of the linked list.


I'm inclined to think this adjustment is worth doing now -- although I
suppose it's not crazy to try landing the patch without it and then
doing it later.

Comment 32

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

Comment on attachment 8615764 [details] [diff] [review]
Part 4: Cache reset structs on rule nodes for different font-size or writing mode values.

>     default:
>+      aCacheKey.SetUncacheable();
>       NS_NOTREACHED("unexpected unit");
>       break;

Skip this; there's no actual dependency here if we were ever to hit this code, and it might make the compiler optimize the code less well.

r=dbaron with that

Comment 33

[Cameron McCormack (:heycam)]

(In reply to David Baron [:dbaron] ⏰UTC-7 (mostly busy, back June 29) from comment #31)
> Here you're missing out on quite a bit of caching opportunity.  You're
> using mStyleData rather than aHighestNode->mStyleData, which means you're
> storing the data at the most specific point in the rule tree rather than
> at the least specific point in the rule tree that's possible.
> 
> I think that's worth doing.  It will make the optimization here
> substantially stronger.

I did consider this, but...

> However, you'll need to decide what it means about the dependent bits,
> and the invariants related to IsUsedDirectly (which are maintained by
> both PropagateDependentBit and SetUsedDirectly).  You clearly can't use
> PropagateDependentBit as it is today.  So you'll need to come up with
> new invariants and adjust WalkRuleTree as appropriate.

...yes, working out what the exact invariants are here wasn't straightforward, so I figured I'd look at that later.  We need to ensure that the font-size and writing mode values that we rely on do not have different values in any of the rule nodes we visit in PropagateDependentBit (or its new analogue).

> I'm inclined to think this adjustment is worth doing now -- although I
> suppose it's not crazy to try landing the patch without it and then
> doing it later.

I'll probably not have time to make those changes before the end of the quarter, so I suggest getting the current patches in first and I'll look at adjusting dependent bits next month.

Comment 34

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

(In reply to Cameron McCormack (:heycam) from comment #33)
>  We need to ensure
> that the font-size and writing mode values that we rely on do not have
> different values in any of the rule nodes we visit in PropagateDependentBit
> (or its new analogue).

I don't see why.  The whole point is that we allow these values to vary now, no?

Comment 35

[Cameron McCormack (:heycam)]

Ah yes, of course.  Well I will see if I get time to try this, but I'll still do it as a followup.

Comment 36

[Cameron McCormack (:heycam)]

(In reply to David Baron [:dbaron] ⏰UTC-4 (mostly busy, back June 29) from comment #31)
> Comment on attachment 8615762 [details] [diff] [review]
> Part 3: Support conditional cached reset structs on rule nodes.
...
> I'm a little concerned about the presence of the GetStyleData function
> on nsCachedStyleData and nsConditionalResetStyleData that doesn't take a
> style context.  Same for the GetStyle##name_ on nsCachedStyleData.  Is
> that temporary?  What still calls it at the end of the patch series?
> (I see the assertion in COMPUTE_END_RESET... though that use could be
> replaced by a HasStyleData call, i.e., you could add a HasStyleData
> that just returns boolean.)

There are a few uses of the nsStyleContext parameter-less GetStyleData method left at the end of the patch queue:

* nsRuleNode::NodeHasCachedData -- this is only used by StyleAnimationValue::ComputeValue, and I think it's correct to check for non-conditional cached data, here.  Maybe the method should be renamed to NodeHasUnconditionalCachedData.

* nsRuleNode::SetUsedDirectly -- should this actually be copying conditional structs, too, even if we leave the stronger optimisation mentioned in comment 31 to a followup?  I think it doesn't need to, since the only uses of IsUsedDirectly() are in PropagateDependentBit, which we're not yet doing for conditional structs, and in WalkRuleTree, where we're currently only looking for unconditional structs.

* nsRuleNode::WalkRuleTree -- again, before we do the stronger optimisation, I think we want to explicitly look up the unconditional structs

* the aforementioned assertion in COMPUTE_END_RESET

Unless there is something wrong about their usage, I will leave them; hopefully after doing the stronger optimisation it can be removed.

However, the GetStyle##name_ methods are not used, so I will remove them.

Comment 37

[Cameron McCormack (:heycam)]

(In reply to David Baron [:dbaron] ⏰UTC-4 (mostly busy, back June 29) from comment #30)
> Comment on attachment 8615761 [details] [diff] [review]
> Part 2: Add a RuleNodeCacheKey class and use it instead of a boolean
> canStoreInRuleTree during style computation.
> 
> I'm not sure about the use of "CacheKey" as a term here.  We've usually
> used that for something used to look something up in a cache or check
> whether a cached result is still valid.  This seems a little broader than
> that (although I suppose I've already taken CacheKey a little bit out
> of context -- I think this takes it farther).  Might
> RuleNodeCacheability be better?  Any other ideas?  (Search-replacing
> the patch should be doable.)

The object does two things -- recording whether a struct is cacheable (and if so, what its conditions are), and being able to match a style context to the conditions.  Maybe "RuleNodeCacheConditions"?

> The definition of mFontSize in RuleNodeCacheKey.h should say that it's
> the font size from which em units are derived.  (Otherwise there's
> ambiguity in the case of text zoom or minimum font size.)  If you want
> to work it out, it might even be worth saying what that means in the case
> of text zoom and minimum font size (though it might not be a bad idea for
> me to review that).

I don't think it's that that important to mention what value we're actually resolve em units against given text zoom and minimum font size settings.

Comment 38

[Cameron McCormack (:heycam)]

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

Comment 39

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

Comment on attachment 8615762 [details] [diff] [review]
Part 3: Support conditional cached reset structs on rule nodes.

re comment 36:

>* nsRuleNode::NodeHasCachedData -- this is only used by
>StyleAnimationValue::ComputeValue, and I think it's correct to check
>for non-conditional cached data, here.  Maybe the method should be
>renamed to NodeHasUnconditionalCachedData.

Yeah, renaming sounds good.

Probably plus a comment of caution.

>However, the GetStyle##name_ methods are not used, so I will remove
>them.

Yes, please.

Re comment 37:

>The object does two things -- recording whether a struct is cacheable
>(and if so, what its conditions are), and being able to match a style
>context to the conditions.  Maybe "RuleNodeCacheConditions"?

Sounds good.


Please do file the followup on the remaining caching part here.

r=dbaron with that and the other minor things in comment 31.

Comment 40

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

Comment on attachment 8615763 [details] [diff] [review]
Part 3.1: Limit the number of conditional style structs we cache on rule nodes.

After thinking more about the followup bug (see comment 31), I'm
inclined to think we should skip this after all.

Comment 41

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1e215a36545b
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7a0c7012a37
https://hg.mozilla.org/integration/mozilla-inbound/rev/547f41558cd5
https://hg.mozilla.org/integration/mozilla-inbound/rev/733b4adb4140

Comment 42

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1e215a36545b
https://hg.mozilla.org/mozilla-central/rev/b7a0c7012a37
https://hg.mozilla.org/mozilla-central/rev/547f41558cd5
https://hg.mozilla.org/mozilla-central/rev/733b4adb4140